Fix the two real-history failure modes: action_id crash degrade + config-bound capability detection#256
Merged
Merged
Conversation
…hing the scan A --diff-from report or baseline serialized by a pre-#226 engine can carry duplicate action_id values in its round-tripped action_surface_facts. compute_action_surface_diff indexed both sides through the hard duplicate-id guard, so the scan died with exit 2 and a Config error pointing at a manifest that is fine — the block/goose miner shape (4 of 120 mined PRs). A gate must fail-safe on inputs it inferred itself, not fail-closed with a config error. - compute_action_surface_diff takes an optional warnings sink (same pattern as build_action_surface_facts since a968a6e). With a sink, duplicate ids on either side are disambiguated via the existing tool-name-suffix strategy — matching what the head side's build-time disambiguator produces, so a degraded base lines up with a fresh head instead of flagging the whole surface as churned — and one warning is recorded per collision. No explicit-id carve-out at diff time: both snapshots are engine output whose manifest-authored identities were already validated at build. - The scan decision phase threads its existing action_surface_warnings sink through, so the degrade lands in report.source_warnings and routes the release to review_required rather than crashing. - Sinkless callers (the redaction path keeps its own public-only ordinal disambiguator) preserve the legacy hard ConfigError. - Regression tests mirror #226: a unit test on the diff path plus a full run_scan integration test that round-trips a goose-shaped report with collapsed ids through --diff-from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…IG-BINDING-*)
Implements the detection half of
docs/engineering/config-bound-capability-detection.md — the 2026-06
design-partner pilot blind spot: a toolkit factory whose authority-
bearing constructor argument is bound from configuration hides its
effective tool surface from static enumeration on BOTH sides of a
verify diff, so removing the binding (an authority expansion dressed
as cleanup) or editing the bound config produced no signal at all.
Fact layer:
- inputs/config_trace.py: conservative same-file tracer classifying a
factory's authority-bearing argument as bound from a config read
(json/yaml/toml/tomllib load, os.environ / os.getenv, an in-file
pydantic BaseSettings subclass) or unknown. Same-scope assignment
tracing only (enclosing function, then module body); reassigned or
unresolvable names are ambiguous -> unknown. Literal config-file
paths are recovered from load("p") / load(open("p")) /
loads(Path("p").read_text()).
- ToolkitScopeBound gains config_binding
(literal|config|absent|unknown) + config_path. The OpenAI-SDK
toolkit extractor labels every bound; the previously-silent
non-literal-configuration case (a fail-open: the factory vanished
from every surface) now emits a config marker or an unknown marker
plus a source warning, mirroring the ADK dynamic-toolset fix.
- core/toolkit_scope.py carries the new states through the existing
toolkit_scope_bound policy-fact channel via new summary sentinels;
literal/absent facts keep a byte-identical summary and value_hash so
existing base reports do not churn.
- LangChain/CrewAI dynamic-tool-surface facts gain config_binding /
config_path keys when (and only when) the dynamic tools expression
traces to a config read — purely additive, legacy shape untouched.
Checks (category verify: suppression-immune, floor-protected, one
decision engine, nothing fires without a VerificationContext):
- SHIP-CAP-CONFIG-BINDING-REMOVED (high): a factory site present on
both sides moved config-bound -> absent; the toolkit may fall back
to everything-enabled defaults. Routes to review_required/blocked
instead of silent insufficient_evidence parity. An unreadable
(unknown) head binding NEVER fires it — the design doc's
false-positive guard — and a head moving to a literal allowlist is
the safe direction and emits nothing.
- SHIP-CAP-CONFIG-BINDING-CHANGED (medium review item): both sides
config-bound and the bound config file is in changed_files; static
analysis cannot diff the effective tool list, so the config delta is
routed to review. Pathless (env/settings) bindings never guess a
file match.
- Both recommendations carry the explicit-inventory remedy with the
factory's source path and line, which build_fix_task projects into
instructions/allowed_repairs (acceptance #3).
- SHIP-SCOPE-TOOLKIT-UNBOUNDED now skips config/unknown bindings
("mounted without a scope bound" would be a false claim) and its
remedy names the inventory alternative; the capability-scope
broadening check likewise fails safe on config/unknown heads. The
base/head bound collectors + instance pairing move to _verify_common
so the two diff checks share one identity model
(provider:binding key, line-movement tolerant).
Registration: registry BUILTIN_CHECKS, docs/checks/verify.yaml (+
scope.yaml remedy), docs/checks.md table + detail sections,
regenerated docs/checks.json and llms-full.txt.
Tests: tests/fixtures/config_bound_factory (pilot-shaped repo where
the head deletes the config binding) with end-to-end assertions —
base scans to insufficient_evidence, the verify diff yields
SHIP-CAP-CONFIG-BINDING-REMOVED and review routing, and fix_task
carries the inventory remedy with the factory site. Plus tracer,
extractor, codec (incl. legacy value_hash stability), and
classification unit tests. Full suite green: zero new findings on all
existing golden samples (acceptance #2).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review P1: SHIP-CAP-CONFIG-BINDING-CHANGED only fired when the bound config file itself was in changed_files, so repointing a factory's binding from config/safe.json to a pre-existing config/broad.json — only the .py file changes — produced no finding and release_decision could stay passed. The retarget is now its own trigger: when both sides carry a literal config_path and they differ, emit the review item with previous_config_path -> config_path evidence, independent of changed_files. Conservative guards keep: pathless (env/settings) bindings never compare; config->unknown still never fires. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
P1 fixed — good catch, and thank you for reproducing it. The retarget ( 🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Roadmap item 2 (the W26 corpus's active fix), implemented per
docs/engineering/config-bound-capability-detection.mdand calibrated against the real repos as the design doc demands.Fix 1 — duplicate
action_iddegrades instead of crashing (0e5e282)The goose crash (
Duplicate action_surface action_id, 4/120 W26 rows dead at exit 2) survived #226 via a second path:compute_action_surface_diffon a base reference serialized by a pre-#226 engine. Reproduced first, then fixed with #226's own pattern — an optional warnings sink, degrade via the existing tool-name-suffix disambiguator (reconstructs the same ids head-side dedup produces, so base pairs cleanly), warning lands insource_warnings→ review_required. Sinkless callers keep the hard error.Fix 2 — config-bound capability detection (
1e1e488)All three detections from the design doc, hung on the existing
toolkit_scope_boundfact channel (the one that already round-trips base↔head):SHIP-CAP-CONFIG-BINDING-REMOVED(high, suppression-immune): a factory's config binding deleted between base and head — the dangerous "cleanup that expands authority" case.SHIP-CAP-CONFIG-BINDING-CHANGED(medium review item): both sides bound + the bound config file inchanged_files.SHIP-SCOPE-TOOLKIT-UNBOUNDEDremedy upgraded with the explicit-inventory alternative + factory site; broadening checks now skipconfig/unknownstates so they can't claim "full surface mounted" on surfaces they can't see.inputs/config_trace.py): json/yaml/toml loads,os.environ, in-file pydantic settings; same-file, enclosing-scope only; unresolvable →unknown, andconfig → unknownnever fires — the design doc's false-positive guard.Real-repo calibration (fresh clones, all 10 W26 engaged rows re-evaluated on this branch)
insufficient_evidenceinsufficient_evidence— correct: these diffs don't touch the binding; the blind spot this PR closes (silent binding removal) is proven by the fixture e2e (IE parity → SHIP-CAP-CONFIG-BINDING-REMOVED + review_required)scan_failed(crash)insufficient_evidencescan_failed(crash)insufficient_evidencescan_failed(crash)human_review_requiredPost-fix score on the W26 labels (#255):
unscored4→0; the crash class is eliminated from real history. One safe PR (goose#9717, an endpoint-deletion capability reduction) now review-routes — conservative, surfaced, and honest, vs silently dead before.Acceptance (design doc, all three)
SHIP-CAP-CONFIG-BINDING-REMOVED,review_required, not IE parity. ✅samples/untouched (no false positives on static repos). ✅ (independently re-run: exit 0)fix_taskcarries the inventory remedy withsource path:line. ✅ (asserted in-test)Scope cuts documented in-code: LangChain/CrewAI surfaces get the
config_bindingannotation but no base↔head carriage channel yet; severity calibration removal=high / changed=medium per spec.Merge-order note: touches generated
llms-full.txt+docs/checks.json, as does #254 — whichever merges second re-runs the generators.🤖 Generated with Claude Code