Independent code review of #17 (adapter runtime-load) and #18 (schema
versioning) surfaced four Required and three Optional findings worth
fixing before the patterns harden into the codebase.
Required:
R1: Delete .molecule-ci/scripts/{validate-workspace-template,
migrate-template}.py — dead-vendored mirror. The new validator
workflow invokes .molecule-ci-canonical/scripts/ (the canonical
clone), not .molecule-ci/scripts/. The mirror was the exact drift
class #90 is supposed to eliminate: next contributor would edit
one copy and silently diverge. Other workflows (validate-plugin,
validate-org-template) still use the legacy path and keep their
own scripts there — so removing OUR two files is asymmetric but
correct, and the legacy path can phase out organically.
R2: validate-workspace-template.yml's `cache-dependency-path` pointed
at the validator's own deps file (just `pyyaml>=6.0`). Pip cache
key never invalidated when the template added crewai/langgraph/
etc. Repoint to the calling repo's `requirements.txt`, which is
the file the heavy install actually uses one step later.
R3: `_check_schema_v1` looped `SCHEMA_V1_REQUIRED_KEYS` and re-emitted
"missing required key `template_schema_version`" — but the
dispatcher already verified the field is present + int before
reaching v1, so that branch was dead defensive code. Skip it
explicitly with a comment, but keep the field in the constant for
contract documentation + the unknown-keys filter.
R4: `_template_adapter_under_validation` was a fixed sys.modules key,
meaning back-to-back invocations in the same Python process
shared the slot. Use a per-call-unique name keyed on the absolute
path's hash. No observed bug today; defensive-only.
Optional:
O1: Class-discovery filter now also requires `__module__ == module_name`.
Without this, an `from molecule_runtime.adapters.base import
AbstractCLIAdapter` re-export would count as a "real" adapter,
masking the genuine "no concrete subclass" case the gate exists
to catch. Cheap and forward-proofs against any future abstract
intermediate the runtime might expose. Added a sibling test
pinning the new behavior.
O2: migrate-template.py's docstring claimed "uses ruamel.yaml when
available" but the implementation only ever calls `yaml.safe_dump`.
Replaced the lie with a clearer caveat block + a forward-pointer
to ruamel-when-comments-detected as a future enhancement.
O3: Reordered the workflow so the secret-scan step runs BEFORE
`pip install -r requirements.txt`. Same threat surface as the
Docker build smoke (which already runs first), but cheap defense-
in-depth: a malicious template PR adding a malicious dep to
requirements.txt now has its post-install hook execute AFTER the
secret scanner has already inspected the diff.
Test changes:
- test_adapter_with_no_baseadapter_subclass_errors updated for the
new error message ("no concrete class inheriting from").
- New test_only_imported_baseadapter_subclass_does_not_count pins
the O1 __module__-filter behavior.
- 43/43 tests pass (was 42/42 before the new test).
- Real langgraph template still passes the full validator end-to-end.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f125d68910