Address PR #111 review: per-scan registry + open type field + tighter signature gate

Four findings from PR #111 review — three P1s and one P2. All four
addressed; 7 regression tests added (now 28 total in
test_adapter_entry_point_discovery.py).

P1 #1 — --no-plugins did not disable adapters after a prior enabled scan.
Pre-fix, ``discover_third_party_adapters`` mutated the module-global
``REGISTRY``. A later in-process scan with ``plugins_enabled=False``
skipped discovery but ``_load_sources`` still resolved the
already-registered adapter through that same global. Reproduced:
``report.loaded_adapters == []`` on scan two but the adapter still
ran. Trust boundary violated.

P1 #2 — repeated scans misclassified stable third-party adapters as
``source_type_collision``. Same root cause: collision detection used
``registry._adapters.keys()`` AFTER scan-one's third-party
registration had mutated the global. Scan two thus saw the same
adapter as a "built-in" collision while still executing it.

Fix for both: per-scan registry clone.

- New ``AdapterRegistry.clone()`` returns a shallow-copy registry
  with a fresh ``_adapters`` dict. Subsequent ``register()`` calls
  on the clone do NOT propagate to the global.
- ``_load_inputs`` now builds a per-scan registry via
  ``scan_registry = REGISTRY.clone()``, runs discovery against
  ``scan_registry``, and threads ``scan_registry`` through to
  ``_load_sources`` via a new ``registry=`` kwarg.
- ``_load_sources`` accepts the per-scan registry, falling back to
  the global only for legacy callers that bypass ``_load_inputs``
  (existing ``tests/test_adapter_registry.py`` patterns).
- Global ``REGISTRY`` stays builtin-only across the process. Tests
  that monkeypatch ``REGISTRY._adapters`` keep working because the
  clone captures the global's state at scan-start (after monkeypatch).

Two new regression tests pin the fix:

- ``test_no_plugins_disables_third_party_after_prior_enabled_scan``:
  scan-one runs with plugins enabled (adapter runs once); scan-two
  in the same process with ``plugins_enabled=False`` must see
  ``loaded_adapters == []`` AND the adapter must NOT run a second
  time. Pre-fix the invocation counter incremented; post-fix it
  stays at 1.
- ``test_second_scan_does_not_misclassify_third_party_as_collision``:
  two consecutive scans with plugins enabled must both report
  ``validation_status == "valid"`` on the same adapter. Pre-fix
  scan two reported ``source_type_collision``.

The autouse ``_reset_registry_after_test`` fixture is no longer
needed — discovery doesn't mutate the global — and is replaced with
a minimal ``_populate_registry`` fixture that just triggers lazy
population.

P1 #3 — per_source third-party adapters could not be referenced in
shipgate.yaml. ``ToolSourceConfig.type`` was a closed ``Literal`` of
built-in source types, so a manifest declaring
``type: my_third_party_source`` failed Pydantic validation BEFORE
discovery had a chance to register the loader. The v0.20 third-party
adapter surface was unusable for its main advertised use case.

Fix:

- ``ToolSourceConfig.type`` relaxed from ``Literal[...]`` to ``str``.
- New module-level constant ``BUILTIN_TOOL_SOURCE_TYPES`` enumerates
  the seven built-in source types for documentation and for tests
  that need to iterate the curated set.
- New constant ``BUILTIN_PER_SCAN_ONLY_TOOL_SOURCE_TYPES`` ({n8n,
  openai_api, anthropic_api, validation}) preserves the existing
  rejection: those four built-ins are per_scan-only and ingest config
  from a dedicated top-level manifest section, so they MUST NOT
  appear in ``tool_sources``. A new ``reject_per_scan_only_builtins``
  model validator on ``ToolSourceConfig`` raises ``ValueError`` at
  manifest-load with a routable error pointing the user to the
  top-level section.
- Unknown source types (typos like ``n8N``, genuinely-third-party
  types without a registered adapter) still fail at scan time with
  ``ConfigError`` from ``AdapterRegistry.require``. Exit-2 contract
  unchanged.
- ``tests/test_n8n.py::test_n8n_top_level_config_is_accepted_and_tool_source_type_is_rejected``
  updated: the case-variation (``type: n8N``) assertion now uses
  ``run_scan`` (dispatcher-time error) instead of ``load_manifest``
  (parse-time error). Same exit code, different layer.
- ``tests/test_adapter_registry.py`` + ``tests/harness/test_overlay_renderer.py``
  updated to iterate ``BUILTIN_TOOL_SOURCE_TYPES`` instead of
  ``get_args(ToolSourceConfig.type)``.
- New regression test
  ``test_per_source_third_party_adapter_referenced_from_manifest``:
  builds a manifest with ``type: demo_source``, registers a matching
  third-party per_source adapter via entry-points, asserts ``run_scan``
  succeeds and the adapter actually runs.

The JSON manifest schema (``docs/manifest-v0.1.json``) loses the
``enum`` constraint on ``tool_sources[].type`` — schema is now
``{"type": "string"}``. IDE autocomplete for built-in names is a
follow-up; the trust gate is the dispatcher, not the schema.

P2 #4 — signature validator accepted methods that crash the dispatcher.
``_protocol_error`` rejected only >3 required positional and zero
positional. Two failure modes slipped through:

- ``def load(self, source)`` — only 1 positional slot; dispatcher's
  ``adapter.load(source, base_dir, manifest)`` call fails with
  ``TypeError: too many positional arguments``.
- ``def load(self, source, base_dir, manifest, *, must_set)`` —
  required keyword-only param; dispatcher passes no kwargs.

Both now caught at the ``bad_protocol`` gate before registration.
``_protocol_error`` now:

- Computes ``accepting_slots`` = count of (required + optional)
  positional, and ``has_var_positional`` = whether ``*args`` is
  present.
- Rejects when ``accepting_slots < 3 AND not has_var_positional``.
- Rejects any required keyword-only parameter (mirrors plugin
  validation).

Four new regression tests:

- ``test_gate_bad_protocol_load_too_few_positional``: ``load(self,
  source)`` lands in ``bad_protocol`` with "at least 3 positional"
  in the error.
- ``test_gate_bad_protocol_load_required_keyword_only``: required
  kw-only lands in ``bad_protocol`` with "keyword-only" + the param
  name in the error.
- ``test_gate_bad_protocol_accepts_var_positional``: ``load(self,
  *args)`` is valid (can bind 3 args).
- ``test_gate_bad_protocol_accepts_optional_keyword_only``: optional
  kw-only with default is valid.

Verification: 1264 tests pass; ruff clean; schema roundtrip --check
exits 0; coverage 88.02%.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: pengfei-threemoonslab

docs/manifest-v0.1.json

@@ -1502,15 +1502,6 @@
           "title": "Trust"
         },
         "type": {
-          "enum": [
-            "mcp",
-            "openapi",
-            "openai_agents_sdk",
-            "google_adk",
-            "langchain",
-            "crewai",
-            "codex_plugin"
-          ],
           "title": "Type",
           "type": "string"
         }

src/agents_shipgate/cli/scan.py

@@ -366,13 +366,25 @@ def _load_inputs(
     """
     from agents_shipgate.inputs.protocol import discover_third_party_adapters
 
+    # v0.20 (PR #111 review fix P1 #1+#2): build a per-scan registry
+    # clone so third-party discovery NEVER mutates the global
+    # ``REGISTRY``. Without this, a later ``--no-plugins`` scan would
+    # still see adapters registered by an earlier scan, and the
+    # collision check on scan-two would misclassify stable third-
+    # party adapters as ``source_type_collision`` (the global already
+    # has them from scan-one). The clone captures any monkeypatch
+    # state at this exact moment, so existing tests that
+    # ``monkeypatch.setitem(REGISTRY._adapters, …)`` still work.
+    scan_registry = REGISTRY.clone()
     loaded_adapters: list[dict[str, Any]] = []
     discover_third_party_adapters(
-        REGISTRY,
+        scan_registry,
         plugins_enabled=plugins_enabled,
         loaded_adapters=loaded_adapters,
     )
-    loaded_sources, artifact_bag = _load_sources(manifest, base_dir, verbose=verbose)
+    loaded_sources, artifact_bag = _load_sources(
+        manifest, base_dir, verbose=verbose, registry=scan_registry
+    )
     logger.debug(
         "loaded sources",
         extra={
@@ -1386,8 +1398,9 @@ def _load_sources(
     base_dir: Path,
     *,
     verbose: bool,
+    registry: Any = None,
 ) -> tuple[list[LoadedToolSource], ArtifactBag]:
-    """Dispatch every adapter through ``REGISTRY``.
+    """Dispatch every adapter through the supplied ``registry``.
 
     Returns ``(loaded_sources, artifact_bag)``. ``artifact_bag`` is a
     typed ``ArtifactBag`` with per-scan adapter artifacts keyed by
@@ -1397,7 +1410,7 @@ def _load_sources(
     Ordering is deterministic and matches the legacy run_scan order:
 
       1. per-source loaders in tool_sources declared order
-      2. per-scan adapters in REGISTRY iteration order:
+      2. per-scan adapters in registry iteration order:
          google_adk → langchain → crewai → n8n → openai_api
          → anthropic_api → codex_plugin → validation
 
@@ -1409,32 +1422,42 @@ def _load_sources(
     Per-scan source types appearing in tool_sources are ignored by
     pass 1 — they would be redundant; framework loaders already iterate
     every matching entry internally via the manifest.
+
+    v0.20 (PR #111 review fix): ``registry`` is the per-scan registry
+    built by ``_load_inputs`` (``REGISTRY.clone()`` plus any
+    third-party adapters validated in this scan). Defaults to the
+    module-global ``REGISTRY`` only for callers that bypass
+    ``_load_inputs`` (notably the legacy tests in
+    ``tests/test_adapter_registry.py``). New code should always pass
+    a per-scan registry.
     """
+    if registry is None:
+        registry = REGISTRY
     per_source_loaded: list[LoadedToolSource] = []
     per_scan_loaded: list[LoadedToolSource] = []
     bag = ArtifactBag()
 
     # Pass 1 — per-source adapters only, in tool_sources declared
     # order. Per-scan source types (langchain, crewai, etc.) are
-    # skipped here; pass 2 invokes them in canonical REGISTRY order
+    # skipped here; pass 2 invokes them in canonical registry order
     # regardless of where they appear in tool_sources. This protects
     # the dedup tie-break in _flatten_and_deduplicate_tools from
     # changing based on user-facing tool_sources ordering.
     for source in manifest.tool_sources:
-        adapter = REGISTRY.require(source.type)
+        adapter = registry.require(source.type)
         if adapter.scope != "per_source":
             continue
         result = _invoke_per_source_adapter(
             adapter, source, base_dir, manifest, verbose=verbose
         )
         _absorb(result, source.type, per_source_loaded, bag, adapter)
 
-    # Pass 2 — every per-scan adapter fires once, in REGISTRY order.
+    # Pass 2 — every per-scan adapter fires once, in registry order.
     # Covers framework adapters (always check their manifest section
     # internally and may emit zero LoadedToolSource entries when not
     # configured) and manifest-only adapters (openai_api,
     # anthropic_api, n8n).
-    for adapter in REGISTRY.per_scan_adapters():
+    for adapter in registry.per_scan_adapters():
         result = adapter.load(None, base_dir, manifest)
         _absorb(result, adapter.source_type, per_scan_loaded, bag, adapter)
 

src/agents_shipgate/inputs/adapter_validation.py

@@ -312,8 +312,22 @@ def _protocol_error(adapter: Any) -> str | None:
     except (TypeError, ValueError):
         # Builtins / C extensions; accept — runtime wrapper will catch.
         return None
-    # ToolSourceAdapter.load takes (source, base_dir, manifest). The
-    # bound method drops ``self``, leaving 3 positional parameters.
+    # ToolSourceAdapter.load is invoked as
+    # ``adapter.load(source, base_dir, manifest)`` — three positional
+    # arguments, never any keyword arguments. The validator must accept
+    # any signature compatible with that exact call shape and reject
+    # everything else. Three failure modes the original v0.20 (PR #111
+    # review fix P2 #4) implementation missed:
+    #
+    # - Too FEW positional slots, e.g. ``load(self, source)`` — the
+    #   bound method has only 1 positional slot; the dispatcher's
+    #   3-arg call would crash with ``TypeError: too many positional
+    #   arguments``.
+    # - Required keyword-only parameters, e.g.
+    #   ``load(self, source, base_dir, manifest, *, must_set)`` — the
+    #   dispatcher never passes kwargs; the call would crash with
+    #   ``TypeError: missing 1 required keyword-only argument``.
+    # - Too MANY required positional params (preserved from v0.20).
     positional = [
         p
         for p in sig.parameters.values()
@@ -324,24 +338,54 @@ def _protocol_error(adapter: Any) -> str | None:
             inspect.Parameter.VAR_POSITIONAL,
         )
     ]
+    if not positional:
+        return (
+            "adapter.load must accept positional arguments "
+            "(source, base_dir, manifest)"
+        )
     required = [
         p
         for p in positional
         if p.kind != inspect.Parameter.VAR_POSITIONAL
         and p.default is inspect.Parameter.empty
     ]
-    # The wrapper calls adapter.load(source, base_dir, manifest) — three
-    # positional arguments. Accept up to 3 required positional slots;
-    # extra optional slots are fine.
     if len(required) > 3:
         return (
             "adapter.load must accept at most 3 required positional "
             f"parameters (source, base_dir, manifest); got {len(required)}"
         )
-    if not positional:
+    # Count how many positional slots can absorb arguments. Required +
+    # optional contribute one each; ``*args`` absorbs any remaining.
+    accepting_slots = sum(
+        1
+        for p in positional
+        if p.kind != inspect.Parameter.VAR_POSITIONAL
+    )
+    has_var_positional = any(
+        p.kind == inspect.Parameter.VAR_POSITIONAL for p in positional
+    )
+    if accepting_slots < 3 and not has_var_positional:
         return (
-            "adapter.load must accept positional arguments "
-            "(source, base_dir, manifest)"
+            "adapter.load must accept at least 3 positional arguments "
+            f"(source, base_dir, manifest); got {accepting_slots} "
+            "positional slot(s) and no *args"
+        )
+    # Required keyword-only parameters are not satisfiable by the
+    # ``plugin(source, base_dir, manifest)`` call shape. Optional
+    # kw-only (with defaults) and ``**kwargs`` are fine. Mirrors the
+    # plugin-validation signature gate.
+    required_kw_only = [
+        p
+        for p in sig.parameters.values()
+        if p.kind == inspect.Parameter.KEYWORD_ONLY
+        and p.default is inspect.Parameter.empty
+    ]
+    if required_kw_only:
+        names = ", ".join(p.name for p in required_kw_only)
+        return (
+            "adapter.load has required keyword-only parameter(s) "
+            f"({names}); the dispatcher calls adapter.load(source, "
+            "base_dir, manifest) with no keyword arguments"
         )
     return None
 

src/agents_shipgate/inputs/protocol.py

@@ -152,6 +152,38 @@ def register(self, adapter: ToolSourceAdapter) -> None:
             )
         self._adapters[adapter.source_type] = adapter
 
+    def clone(self) -> AdapterRegistry:
+        """v0.20 (PR #111 review follow-up): return a shallow-copy
+        registry snapshot for use as a *per-scan* adapter registry.
+
+        The clone shares no mutable state with the original — its
+        ``_adapters`` dict is a fresh copy. Subsequent ``register()`` /
+        third-party additions on the clone do NOT propagate to the
+        global ``REGISTRY``. Closes two P1 review findings:
+
+        - ``--no-plugins`` on a later in-process scan now actually
+          disables third-party adapters discovered by an earlier scan
+          (which previously persisted in the global registry).
+        - Collision detection on the second scan sees only the
+          builtins (clone-time state), not adapters carried over from
+          scan one, so a stable third-party adapter no longer
+          mis-reports as ``source_type_collision``.
+
+        Tests that monkeypatch ``REGISTRY._adapters`` before
+        ``_load_inputs`` runs are preserved: the clone captures the
+        global's state at that moment, including monkeypatch
+        modifications.
+        """
+
+        clone = AdapterRegistry(autopopulate=False)
+        # Lazy-populate ourselves first so the clone inherits the
+        # canonical builtin set even when this is the global REGISTRY's
+        # first read.
+        self._ensure_populated()
+        clone._adapters = dict(self._adapters)
+        clone._populated = True
+        return clone
+
     def get(self, source_type: str) -> ToolSourceAdapter | None:
         self._ensure_populated()
         return self._adapters.get(source_type)
@@ -258,6 +290,21 @@ def discover_third_party_adapters(
     """Walk ``entry_points('agents_shipgate.adapters')``, validate each
     third-party adapter, and register the valid ones onto ``registry``.
 
+    **Per-scan registry contract (v0.20 review fix).** ``registry``
+    MUST be a per-scan instance (typically ``REGISTRY.clone()`` from
+    ``_load_inputs``), NOT the global ``REGISTRY`` itself. Mutating
+    the global broke two trust invariants:
+
+    1. ``--no-plugins`` on a later scan didn't disable adapters
+       registered by an earlier scan, because the dispatcher still
+       resolved them through the global registry.
+    2. Collision detection on the second scan misclassified
+       previously-registered third-party adapters as
+       ``source_type_collision`` against themselves.
+
+    Callers pass a clone so each scan starts from a known-good
+    builtins-only snapshot.
+
     Returns the list of ``LoadedAdapter`` records (both valid and
     invalid). When ``loaded_adapters`` is provided, the per-row
     ``info`` dicts are appended to it for downstream
@@ -268,14 +315,6 @@ def discover_third_party_adapters(
     ``plugins_enabled`` is not explicitly ``True``. ``--no-plugins``
     on the CLI translates to ``plugins_enabled=False`` and forces this
     off even when the env var is set.
-
-    Calling this twice with the same ``registry`` is safe: collision
-    detection runs against the registry's current state, and
-    already-registered third-party adapters are rejected with
-    ``source_type_collision``. A test that monkeypatches
-    ``entry_points`` should also reset the registry between runs (use
-    a fresh ``AdapterRegistry(autopopulate=False)`` or
-    ``monkeypatch.setattr`` for full isolation).
     """
 
     from agents_shipgate.inputs.adapter_validation import (

src/agents_shipgate/schemas/manifest.py

@@ -47,24 +47,80 @@ class EnvironmentConfig(BaseModel):
     promotion_to: str | None = None
 
 
+#: v0.20 (PR #111 review fix P1 #3): the curated set of built-in
+#: source types that may legitimately appear in ``tool_sources[].type``.
+#: Used for documentation only — ``ToolSourceConfig.type`` is open
+#: (``str``) so third-party adapters from the ``agents_shipgate.adapters``
+#: entry-point group can declare custom source types. Unknown source
+#: types fail at dispatcher time via ``AdapterRegistry.require``.
+BUILTIN_TOOL_SOURCE_TYPES: tuple[str, ...] = (
+    "mcp",
+    "openapi",
+    "openai_agents_sdk",
+    "google_adk",
+    "langchain",
+    "crewai",
+    "codex_plugin",
+)
+
+#: Built-in adapters that are intentionally NOT permitted in
+#: ``tool_sources[]`` because they are per_scan-only and ingest
+#: configuration from their dedicated top-level manifest section.
+#: Placing one of these in ``tool_sources`` is a user mistake; we
+#: reject it at manifest-load time with a clear error rather than
+#: silently no-op'ing it in pass 1 of the dispatcher.
+BUILTIN_PER_SCAN_ONLY_TOOL_SOURCE_TYPES: frozenset[str] = frozenset(
+    {"n8n", "openai_api", "anthropic_api", "validation"}
+)
+
+
 class ToolSourceConfig(BaseModel):
     model_config = STRICT_MODEL_CONFIG
 
     id: str
-    type: Literal[
-        "mcp",
-        "openapi",
-        "openai_agents_sdk",
-        "google_adk",
-        "langchain",
-        "crewai",
-        "codex_plugin",
-    ]
+    # v0.20 (PR #111 review fix P1 #3): opened from a closed Literal to
+    # ``str`` so manifests can reference third-party per_source adapters
+    # registered via the ``agents_shipgate.adapters`` entry-point group.
+    # Without this relaxation, ``ToolSourceConfig.model_validate``
+    # rejected ``type: my_custom_source`` at manifest-load time —
+    # before adapter discovery had a chance to register the loader —
+    # making the v0.20 third-party adapter surface unusable for its
+    # main advertised use case.
+    #
+    # Built-in source types are enumerated in
+    # ``BUILTIN_TOOL_SOURCE_TYPES`` above (documentation only). Unknown
+    # source types are still rejected with a routable ``ConfigError``
+    # (exit 2) at dispatch time via ``AdapterRegistry.require``. Typos
+    # in built-in names therefore fail loudly with the same exit code
+    # as before — just from a different code path.
+    #
+    # Per_scan-only built-ins (``n8n``, ``openai_api``, ``anthropic_api``,
+    # ``validation``) remain explicitly REJECTED here (model validator
+    # below) because they ingest config from their dedicated top-level
+    # manifest section, not from ``tool_sources[]``.
+    type: str
     path: str | None = None
     trust: str | None = None
     mode: str | None = None
     optional: bool = False
 
+    @model_validator(mode="after")
+    def reject_per_scan_only_builtins(self) -> ToolSourceConfig:
+        # Each per_scan-only built-in has its own dedicated manifest
+        # section (``n8n.workflows``, ``openai_api.tools``, …); putting
+        # it in ``tool_sources`` is always a user mistake. Third-party
+        # per_scan adapters (also discovered via entry points) do NOT
+        # require ``tool_sources`` entries either; the dispatcher
+        # iterates them in pass-2 regardless.
+        if self.type in BUILTIN_PER_SCAN_ONLY_TOOL_SOURCE_TYPES:
+            raise ValueError(
+                f"tool_sources entry {self.id!r} declares type "
+                f"{self.type!r}, which is a per_scan-only built-in. "
+                f"Move this configuration to the top-level "
+                f"``{self.type}:`` manifest section."
+            )
+        return self
+
     @model_validator(mode="after")
     def require_path_when_needed(self) -> ToolSourceConfig:
         if (

tests/harness/test_overlay_renderer.py

@@ -45,15 +45,21 @@ def test_40_shipgate_yaml_renders_clean_for_every_archetype(tmp_path: Path) -> N
 
 
 def test_every_archetype_uses_a_valid_tool_source_type() -> None:
-    """Each archetype's ``ToolSource.type`` must be in the v0.1 manifest enum.
-
-    Catches regressions like the original ``openai_api`` typo for
-    clean-read-only that would otherwise only surface when an operator runs
+    """Each archetype's ``ToolSource.type`` must be a known built-in
+    source type (third-party extensions are intentionally NOT used in
+    canonical archetypes — the harness ships a fixed set).
+
+    v0.20 PR #111 review fix: ``ToolSourceConfig.type`` relaxed from
+    Literal to ``str`` to support third-party adapters; this test now
+    pins the archetype set against the explicit
+    ``BUILTIN_TOOL_SOURCE_TYPES`` constant. Catches regressions like
+    the original ``openai_api`` typo for clean-read-only that would
+    otherwise only surface when an operator runs
     ``agents-shipgate doctor`` on a rendered 40-shipgate-yaml manifest.
     """
-    from agents_shipgate.schemas.manifest import ToolSourceConfig
+    from agents_shipgate.schemas.manifest import BUILTIN_TOOL_SOURCE_TYPES
 
-    allowed = set(ToolSourceConfig.model_fields["type"].annotation.__args__)
+    allowed = set(BUILTIN_TOOL_SOURCE_TYPES)
     for archetype, ctx in ctx_mod.ARCHETYPE_CONTEXTS.items():
         for ts in ctx.tool_sources:
             assert ts.type in allowed, (