From 2e40916b57308798a79d5fce8d664eb5a1470606 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Tue, 28 Apr 2026 12:27:09 -0700 Subject: [PATCH] fix(validator): handle abstract intermediates + class-aliasing + lock GITHUB_TOKEN scope (#21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent post-merge review of #19 surfaced two more findings. Both shipped here. Q3 — abstract intermediates + multiple-concrete-classes. The class-discovery filter from O1 (#19) only excluded BaseAdapter itself. Two failure modes slipped through: (a) A locally-defined abstract intermediate `class FrameworkAdapter(BaseAdapter): @abstractmethod ...` passed the filter, falsely satisfying "at least one concrete subclass" while still being non-instantiable at workspace boot. (b) A template defining BOTH `class FrameworkAdapter(BaseAdapter)` AND `class ConcreteAdapter(FrameworkAdapter)` had both pass the filter, producing a silent ambiguity where the runtime's class-discovery picks one per its resolution rules — wrong class loaded after a future runtime refactor. Fixes: - Add `not inspect.isabstract(obj)` to the discovery filter so abstract intermediates are excluded. - Hard-error if `len(adapter_classes) > 1` listing both names so the contributor knows exactly which classes are competing. Three new tests pin the behaviors: - test_abstract_intermediate_alone_does_not_count - test_abstract_plus_concrete_passes_with_concrete_only - test_multiple_concrete_baseadapter_subclasses_errors Identity-based deduplication. Caught against the real langgraph template during smoke-testing the Q3 fix: production adapters often do `Adapter = ConcreteAdapter` as a module-level alias for the runtime's discovery convention. `vars(mod)` returns BOTH bindings pointing at the same class object, so the new multiple-concrete-classes error fired falsely on every aliased template. Fix: deduplicate by `id(obj)` BEFORE counting, so the same class object under multiple bindings counts once. New regression test test_aliased_concrete_class_is_deduplicated pins this against any future filter regression. Existing tests updated to use fully-concrete BaseAdapter subclasses (matching production templates) since the new abstract-filter correctly rejects partial stubs that don't override every abstract method BaseAdapter declares (5 methods: name, display_name, description, setup, create_executor). Q5 — GITHUB_TOKEN scope lockdown. validate-workspace-template.yml runs untrusted-by-design code from the calling template repo: pip post-install hooks, adapter.py imports, Dockerfile RUN steps. Each of those primitives executes with GITHUB_TOKEN in env. The workflow had no `permissions:` block, defaulting to whatever the calling repo grants — often contents: write. Add `permissions: contents: read` at the workflow level. Worst- case-with-token now drops to "read public repo state" — no write to issues, no push to branches, no comment-spam, no workflow re-trigger. Partial mitigation; the deeper `pull_request_target` discipline is bigger scope (tracked separately). Verification: - 47/47 tests pass (was 43; +3 abstract/multi-concrete + +1 alias) - All 8 production templates pass the full updated validator end-to-end with 0 warnings / 0 errors Co-authored-by: Claude Opus 4.7 (1M context) --- .../workflows/validate-workspace-template.yml | 16 ++ scripts/test_validate_workspace_template.py | 143 +++++++++++++++++- scripts/validate-workspace-template.py | 57 +++++-- 3 files changed, 194 insertions(+), 22 deletions(-) diff --git a/.github/workflows/validate-workspace-template.yml b/.github/workflows/validate-workspace-template.yml index 2e4c494..27f0a3d 100644 --- a/.github/workflows/validate-workspace-template.yml +++ b/.github/workflows/validate-workspace-template.yml @@ -2,6 +2,22 @@ name: Validate Workspace Template on: workflow_call: +# Defense-in-depth on the GITHUB_TOKEN scope. This workflow runs +# untrusted-by-design code from the calling template repo — pip +# installs the template's requirements.txt (post-install hooks), +# imports adapter.py, and `docker build`s the Dockerfile (RUN +# steps). Each of those primitives can execute arbitrary code with +# the token in env. Pinning `contents: read` means the worst a +# malicious template PR can do with the token is read public repo +# state — no write to issues, no push to branches, no comment-spam, +# no workflow re-trigger. +# +# Note: this is only a partial mitigation. The deeper lockdown is +# `pull_request_target` discipline (consume the diff but don't +# execute it from the PR's context) — tracked as a separate task. +permissions: + contents: read + jobs: validate: name: Template validation diff --git a/scripts/test_validate_workspace_template.py b/scripts/test_validate_workspace_template.py index 0bb138b..1d2733d 100644 --- a/scripts/test_validate_workspace_template.py +++ b/scripts/test_validate_workspace_template.py @@ -342,19 +342,32 @@ def test_no_adapter_skips_runtime_load_silently(validator, tmp_path, monkeypatch assert runtime_load_warnings == [], runtime_load_warnings -@_skip_no_runtime -def test_valid_baseadapter_subclass_passes(validator, tmp_path, monkeypatch): - """The happy path: adapter.py defines a class inheriting from - BaseAdapter. All 8 production templates match this shape.""" - adapter = ( +def _good_adapter_py() -> str: + """A fully concrete BaseAdapter subclass — overrides every + abstract method BaseAdapter declares. Mirrors the shape of all 8 + production templates so tests of the runtime-load check exercise + the same path the real templates do.""" + return ( "from molecule_runtime.adapters.base import BaseAdapter\n" "\n" "class MyAdapter(BaseAdapter):\n" " @staticmethod\n" - " def name():\n" - " return 'test-adapter'\n" + " def name(): return 'test-adapter'\n" + " @staticmethod\n" + " def display_name(): return 'Test'\n" + " @staticmethod\n" + " def description(): return 'fixture adapter'\n" + " def setup(self, config): pass\n" + " def create_executor(self, config): return None\n" ) - _materialise(tmp_path, adapter_py=adapter) + + +@_skip_no_runtime +def test_valid_baseadapter_subclass_passes(validator, tmp_path, monkeypatch): + """The happy path: adapter.py defines a fully concrete class + inheriting from BaseAdapter. All 8 production templates match + this shape.""" + _materialise(tmp_path, adapter_py=_good_adapter_py()) monkeypatch.chdir(tmp_path) validator.check_adapter_runtime_load() assert validator.ERRORS == [], validator.ERRORS @@ -380,6 +393,120 @@ def test_adapter_with_no_baseadapter_subclass_errors(validator, tmp_path, monkey ), validator.ERRORS +@_skip_no_runtime +def test_abstract_intermediate_alone_does_not_count(validator, tmp_path, monkeypatch): + """A locally-defined abstract subclass (e.g., a framework-level + intermediate that templates extend) must not satisfy the contract + on its own. The runtime needs a CONCRETE class to instantiate; + accepting an abstract one would let workspace boot fail at + instantiation time instead of validator time.""" + adapter = ( + "from abc import abstractmethod\n" + "from molecule_runtime.adapters.base import BaseAdapter\n" + "\n" + "class FrameworkAdapter(BaseAdapter):\n" + " @abstractmethod\n" + " def my_abstract_method(self): ...\n" + ) + _materialise(tmp_path, adapter_py=adapter) + monkeypatch.chdir(tmp_path) + validator.check_adapter_runtime_load() + assert any( + "no concrete class inheriting from" in e + for e in validator.ERRORS + ), validator.ERRORS + + +@_skip_no_runtime +def test_abstract_plus_concrete_passes_with_concrete_only(validator, tmp_path, monkeypatch): + """The legitimate factoring pattern: define an abstract framework- + level intermediate, then a concrete leaf. Only the concrete leaf + counts toward the "at least one" requirement — the framework + intermediate is filtered out by `inspect.isabstract`.""" + adapter = ( + "from abc import abstractmethod\n" + "from molecule_runtime.adapters.base import BaseAdapter\n" + "\n" + "class FrameworkAdapter(BaseAdapter):\n" + " @abstractmethod\n" + " def framework_specific_hook(self): ...\n" + "\n" + "class ConcreteAdapter(FrameworkAdapter):\n" + " def framework_specific_hook(self): pass\n" + " @staticmethod\n" + " def name(): return 'concrete'\n" + " @staticmethod\n" + " def display_name(): return 'Concrete'\n" + " @staticmethod\n" + " def description(): return 'leaf'\n" + " def setup(self, config): pass\n" + " def create_executor(self, config): return None\n" + ) + _materialise(tmp_path, adapter_py=adapter) + monkeypatch.chdir(tmp_path) + validator.check_adapter_runtime_load() + assert validator.ERRORS == [], validator.ERRORS + + +@_skip_no_runtime +def test_multiple_concrete_baseadapter_subclasses_errors(validator, tmp_path, monkeypatch): + """Two concrete BaseAdapter subclasses in the same file is a + silent ambiguity: the runtime's class-discovery picks one per + its own resolution rules, so the WRONG class might be loaded + after a future runtime refactor. Force the maintainer to either + mark intermediates abstract or split into separate modules.""" + adapter = ( + "from molecule_runtime.adapters.base import BaseAdapter\n" + "\n" + "class FirstConcreteAdapter(BaseAdapter):\n" + " @staticmethod\n" + " def name(): return 'first'\n" + " @staticmethod\n" + " def display_name(): return 'First'\n" + " @staticmethod\n" + " def description(): return 'first'\n" + " def setup(self, config): pass\n" + " def create_executor(self, config): return None\n" + "\n" + "class SecondConcreteAdapter(BaseAdapter):\n" + " @staticmethod\n" + " def name(): return 'second'\n" + " @staticmethod\n" + " def display_name(): return 'Second'\n" + " @staticmethod\n" + " def description(): return 'second'\n" + " def setup(self, config): pass\n" + " def create_executor(self, config): return None\n" + ) + _materialise(tmp_path, adapter_py=adapter) + monkeypatch.chdir(tmp_path) + validator.check_adapter_runtime_load() + multi_errors = [ + e for e in validator.ERRORS + if "multiple concrete BaseAdapter subclasses" in e + ] + assert len(multi_errors) == 1, validator.ERRORS + # Both names should appear in the error so the operator knows + # exactly which classes are competing. + assert "FirstConcreteAdapter" in multi_errors[0] + assert "SecondConcreteAdapter" in multi_errors[0] + + +@_skip_no_runtime +def test_aliased_concrete_class_is_deduplicated(validator, tmp_path, monkeypatch): + """Production templates often do `Adapter = ConcreteAdapter` as a + module-level alias for the runtime's class-discovery convention. + `vars(mod)` returns BOTH bindings pointing at the same class + object — without identity-based dedup, the multi-concrete-class + error fires falsely (regression caught against the real langgraph + template during the Q3 fix). Pin that aliased templates pass.""" + adapter = _good_adapter_py() + "\nAdapter = MyAdapter\n" + _materialise(tmp_path, adapter_py=adapter) + monkeypatch.chdir(tmp_path) + validator.check_adapter_runtime_load() + assert validator.ERRORS == [], validator.ERRORS + + @_skip_no_runtime def test_only_imported_baseadapter_subclass_does_not_count(validator, tmp_path, monkeypatch): """Re-exported imports do not satisfy the contract. If the only diff --git a/scripts/validate-workspace-template.py b/scripts/validate-workspace-template.py index 38b9472..98e9318 100644 --- a/scripts/validate-workspace-template.py +++ b/scripts/validate-workspace-template.py @@ -349,21 +349,34 @@ def check_adapter_runtime_load() -> None: sys.modules.pop(module_name, None) return - # Class discovery: only count classes DEFINED in adapter.py, not - # re-exported imports. Without the `__module__` filter, a template - # that does `from molecule_runtime.adapters.base import - # AbstractCLIAdapter` (or any future abstract intermediate the - # runtime exposes) would have that import counted as a "real" - # adapter — masking the genuine "no concrete subclass" case the - # whole check is meant to catch. - adapter_classes = [ - obj + # Class discovery: only count CONCRETE classes DEFINED in + # adapter.py, not re-exported imports and not abstract + # intermediates. Three filter axes: + # + # 1. `__module__ == module_name` — defined HERE, not imported + # from molecule_runtime or a third-party framework. + # 2. `obj is not BaseAdapter` — BaseAdapter itself doesn't count. + # 3. `not inspect.isabstract(obj)` — abstract intermediates + # defined locally don't count. Catches the + # `class Framework(BaseAdapter): pass` + `class Concrete(Framework):` + # pattern where vars(mod) has BOTH and we'd otherwise count + # both as "real" adapters. + import inspect # noqa: PLC0415 + # Deduplicate by class identity. Many production adapters do + # `Adapter = ConcreteAdapter` as a module-level alias for the + # runtime's discovery — `vars(mod)` returns both bindings + # (`Adapter` AND `ConcreteAdapter`) pointing at the same class + # object. Without dedup, the multiple-concrete-subclasses + # error fires falsely on every aliased template. + adapter_classes = list({ + id(obj): obj for name, obj in vars(mod).items() if isinstance(obj, type) and obj is not BaseAdapter and issubclass(obj, BaseAdapter) and getattr(obj, "__module__", None) == module_name - ] + and not inspect.isabstract(obj) + }.values()) sys.modules.pop(module_name, None) if not adapter_classes: @@ -373,10 +386,26 @@ def check_adapter_runtime_load() -> None: "in this file. The runtime resolves the adapter via " "class discovery on adapter.py's own definitions — " "imports of base classes from molecule_runtime do not " - "count. Without a concrete subclass DEFINED here, " - "workspace boot falls through to the default langgraph " - "executor and ignores this file silently. If that's " - "intentional, delete adapter.py." + "count, and abstract intermediates do not count. " + "Without a concrete subclass DEFINED here, workspace " + "boot falls through to the default langgraph executor " + "and ignores this file silently. If that's intentional, " + "delete adapter.py." + ) + return + + if len(adapter_classes) > 1: + names = sorted(c.__name__ for c in adapter_classes) + err( + f"adapter.py: multiple concrete BaseAdapter subclasses " + f"defined: {names}. The runtime's class-discovery picks " + f"one per its own resolution rules (typically last-defined " + f"or first-by-iteration), so shipping more than one is a " + f"silent ambiguity — the wrong class might be loaded after " + f"a future runtime refactor. Either keep exactly one " + f"concrete subclass + mark the others abstract via " + f"`abc.ABC` / abstract methods, or move them to separate " + f"importable modules." )