molecule-ai/molecule-ci
34
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(validator): handle abstract intermediates + class-aliasing + lock GITHUB_TOKEN scope (#21)

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-04-28 12:27:09 -07:00 committed by GitHub
parent 30e094220a
commit 2e40916b57
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 194 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
.github/workflows/validate-workspace-template.yml vendored
View File

@ -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

143
scripts/test_validate_workspace_template.py
View File

@ -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

57
scripts/validate-workspace-template.py
View File

@ -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."
)