fix(agent-mode): adopter-facing output stops naming the internal identity model (#329) - #415
Conversation
ccaa101 to
d593ccb
Compare
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Blocking review findings for exact PR head d593ccb9d3eb477bae192c9e98e00364064cb105.
CI is green, the local verification receipt validates, and the deterministic gate reports passed / mergeable, but the diff still has several reproducible adopter-routing regressions that static gating does not catch.
The highest-risk issue is that the new machine-routable edit action can point at an unrelated shipgate.yaml instead of the manifest actually evaluated. The remaining inline findings cover ambiguous duplicate-source repair, user-caused source IDs being labeled internal defects, a valid agent name aborting semantic validation, an unresolved handoff being labeled as the root agent, a structured remediation path contradicting its text, and the new AST sweep laundering violations across lexical scopes.
Please address these before merge and add the focused regression cases described inline. (GitHub does not permit the PR author’s authenticated account to submit a REQUEST_CHANGES review, so this is posted as a comment review.)
| ), | ||
| ) | ||
| ] | ||
| actions = input_parse_recovery(exc, manifest_path=config) |
There was a problem hiding this comment.
[P1] Preserve the resolved manifest identity in this edit action
config here is only the raw CLI spelling (and defaults to shipgate.yaml), while _resolve_config_paths may have selected a sole nested manifest such as /tmp/repo/services/billing/shipgate.yaml. Reproducing a repeated ADK entrypoint through scan --workspace /tmp/repo emits next_actions[0].path = "shipgate.yaml", so an agent following the documented kind/path route can edit an unrelated trust root in its caller CWD. The same identity loss exists in verify/command.py:347 (raw/None config) and verification.py:185 (no manifest passed). Pass the exact selected/effective manifest to the resolver, and cover nested workspace discovery, omitted/relative verify config, and custom verification-prepare config.
There was a problem hiding this comment.
Confirmed and fixed. Reproduced through scan --workspace <repo> with a sole nested services/billing/shipgate.yaml: next_actions[0].path was "shipgate.yaml".
Fixed at the frame that actually knows the answer rather than at each except. run_scan now wraps its pipeline and does exc.details.setdefault("manifest_path", str(config_path)) on the way out, so scan, verify, _run_multi_scan, and the verification-prepare path all inherit the resolved manifest without a second implementation; input_parse_recovery prefers details["manifest_path"] and keeps the passed argument only as a fallback for failures raised before a scan starts. details.manifest_path is documented in docs/errors.json.
Regressions added: test_a_nested_manifest_is_the_one_the_edit_action_names (workspace discovery) and test_verify_without_a_config_flag_still_names_the_manifest_it_read (omitted --config), both asserting Path(action["path"]) == <the nested manifest>.
| file_path=details.get("source_file"), | ||
| source_id=str(details.get("source_id") or ""), | ||
| ) | ||
| target = str(manifest_path) if manifest_path is not None else "shipgate.yaml" |
There was a problem hiding this comment.
[P2] Do not make the manifest the sole edit target for an ambiguous duplicate
This failure key does not distinguish a repeated manifest entry from duplicate definitions inside the source artifact. With one MCP source whose tools.json contains two tools named pay, the message correctly names tools.json, but the only structured action is kind="edit", path="shipgate.yaml". Consumers route on that path and can remove the source declaration instead of repairing the file, reducing coverage. Emit an explicit review/two candidate edits, or add a typed cause before choosing one target. Also normalize source_file to an actual path: _source_file() currently prefers source_ref, which can be a locator such as agent.yaml#/tools/0, not a manifest path or openable file.
There was a problem hiding this comment.
Confirmed on both halves, and both fixed.
Ambiguous cause. The two mistakes are distinguishable at the raise site after all — seen is now dict[key, read_index], so a collision within one read is a duplicate definition and a collision across reads is a repeated entry. details.cause carries duplicate_in_source_artifact or repeated_source_entry, the message splits with it, and the action follows: your exact scenario (one MCP source, tools.json with two pay tools) now emits kind="edit", path="tools.json" with "the manifest entry is correct". I kept one action rather than two candidates — invariant 1 of #327 — and the one case with nothing to open (a duplicate in an artifact whose loader recorded no path) gets a review action naming the source id instead of an edit pointing somewhere convenient.
_source_file returning a locator. Correct — OpenAPI writes spec.yaml#/paths/~1orders/get, and the Anthropic and Codex plugin loaders write file#index. It now takes the pre-# segment and skips a candidate that reduces to nothing. Covered by test_a_locator_shaped_source_ref_is_reduced_to_a_file.
| # Ours, not theirs: every adapter is contracted to name the source | ||
| # it read. Nothing in the adopter's manifest can produce or repair | ||
| # this, so the message says so instead of prescribing an edit. | ||
| raise InputParseError( |
There was a problem hiding this comment.
[P2] This invariant failure is user-caused for currently valid manifests
ToolSourceConfig(id=" ", type="mcp", path="tools.json") validates today. A normal loader preserves that ID, this .strip() branch fires, and the new message says the defect is in Agents Shipgate and not repairable in the repository. Padded IDs similarly reach the mismatch branch below. The shared recovery then compounds the contradiction by telling the user to inspect an unnamed file. Validate source IDs as nonblank/canonical during manifest loading, or route these cases to the exact shipgate.yaml#tool_sources[].id; do not classify them as internal loader defects while the public schema accepts them.
There was a problem hiding this comment.
Confirmed — ToolSourceConfig(id=" ", type="mcp", path="tools.json") validates today, and the padded form reaches the mismatch branch. Calling that an internal defect while the schema accepts it is exactly backwards.
Fixed at the schema, which is the option that also closes a latent bug: tool_sources[].id is now stripped and refused when blank. ToolObservationSelectorConfig already strips source_id, and tool_inventories[].source_id is compared against the same key — so id: " orders " matched neither and silently completed nothing. A manifest with a padded id was already not doing what it looked like it was doing.
With that closed, a blank LoadedToolSource.source_id really can only come from a loader, so the message stays but no longer claims the fault is ours-versus-theirs in the abstract: it says the defect is in the loader and points at report.json loaded_adapters[] for the third-party case. test_a_blank_tool_source_id_is_a_manifest_error_not_a_loader_defect covers both directions.
| #: incomplete (agent_v1:7205d836…)`` as the sentence under the verdict. The | ||
| #: rule that kept a *tool* subject readable was scoped to tool ids, and a guard | ||
| #: scoped to one kind of id passes vacuously for every other one (#329). | ||
| _AGENT_ID_PATTERN = re.compile(r"agent_v[0-9]+[_:][0-9a-f]{8,}") |
There was a problem hiding this comment.
[P2] Avoid matching derived-ID syntax inside valid agent names
Agent names and source labels are adopter-controlled strings, but this unbounded search classifies customer_agent_v1_deadbeef as a derived agent ID. Once that agent has an evidence gap, _validate_exclusion_ledger raises SemanticConsistencyError and aborts an otherwise valid scan. Require a canonical token boundary/exact reserved shape (or, preferably, typed provenance) instead of searching arbitrary display labels. A regression should exercise a valid agent name containing this substring.
There was a problem hiding this comment.
Confirmed: derived_id_kind("customer_agent_v1_deadbeef") returned "agent", and once that agent has an evidence gap the conservation invariant aborts the scan — a vocabulary rule turning into an outage.
Two changes. Every pattern is now anchored with (?<![0-9A-Za-z_]), and the agent pattern requires the producer's exact separator: core.agent_bindings only ever emits agent_v1:, so accepting agent_v1_ bought nothing and matched adopter identifiers that merely read like one. Tools keep [_:] — that pattern predates this PR and guards a shipped invariant, so I narrowed it with the boundary rather than the separator.
The patterns now live once, in core/adopter_text.py, and surface_exclusions.DERIVED_ID_PATTERNS builds from them: the reader-facing rule and the report invariant are one rule read from opposite ends, and two copies would drift silently in the direction that matters. Regressions: a parametrized negative control over customer_agent_v1_deadbeef / my_tool_v2_deadbeef12 / legacy_fp_…, plus a positive control so the narrowing cannot make the guard vacuous.
| # with the id replaced by its label. An agent the graph did not record | ||
| # cannot be relabelled as the root one: that would say the gap is about | ||
| # a different agent than it is. | ||
| agent_id = issue.agent_id or graph.root_agent_id |
There was a problem hiding this comment.
[P2] Do not label an unresolved handoff as the root agent
A genuinely unresolved handoff is emitted with issue.agent_id=None because the missing endpoint has no resolvable ID, while it still carries a source pointer. This fallback therefore selects graph.root_agent_id; a root -> missing_worker failure reproduces a gap subject of root [sdk], contrary to the comment immediately above and to the actual missing target. That wrong subject propagates into the verdict and fix task. When the issue has no agent ID, prefer its source pointer/source or neutral binding-graph prose; reserve the root label for issue kinds that are explicitly root-wide.
There was a problem hiding this comment.
Confirmed — and my own comment said it should not happen, which is the tell. unresolved_agent_binding is emitted with agent_id=None precisely because the endpoint did not resolve, so root -> missing_worker came out subjected root [sdk].
The first fix I wrote dropped the root fallback entirely, and the conductor golden caught that as an over-correction: samples/conductor_agent reaches _agent_subject through a partial_binding_evidence row from the partials producer, which carries no agent_id and no source_pointer because it is a statement about the whole extraction — there the root is the agent the reader is being told about, and its subject degraded to agent binding graph.
So the fallback is scoped rather than removed, as you suggested: _UNRESOLVED_AGENT_KINDS = {"unresolved_agent_binding"} is the set where a missing agent_id means "could not resolve", and only kinds outside it may borrow the root label. Unresolved handoffs now subject by source_pointer; source is dropped from the chain entirely, since its values are framework_extraction or a bare shipgate.yaml and neither names an agent. test_an_unresolved_handoff_is_not_labelled_as_the_root_agent pins the root -> missing_worker case, and the conductor field-for-field golden pins the other side.
| "Every tool needs a stable identity scoped to the source it was " | ||
| "read from." | ||
| ) | ||
| expects = ( |
There was a problem hiding this comment.
[P2] Keep the structured target consistent with this remediation
The changed text tells the agent to edit shipgate.yaml#tool_sources, but _semantic_gap_path("incomplete_tool_identity", ...) still returns shipgate.yaml#tool_identity. A direct _semantic_gap call produces exactly that contradictory action object. Since agents route on next_action.path, they open the wrong section even though expects names the right one. Update the path classification (or the remediation) and assert both fields together in a regression test.
There was a problem hiding this comment.
Confirmed — _semantic_gap(tool, kind="incomplete_tool_identity", ...) produced path="shipgate.yaml#tool_identity" alongside expects naming shipgate.yaml#tool_sources. Mine, introduced when I rewrote the remediation and left the path classification alone.
The expects is the correct half: accepted_values are unique_source_id and stable_source_path, both properties of tool_sources entries, and there is nothing under tool_identity to set for this kind. So _semantic_gap_path now special-cases incomplete_tool_identity before the _TOOL_IDENTITY_KINDS branch, which keeps conflicting_tool_identity and invalid_tool_binding where they belong.
test_an_identity_gap_sends_the_reader_and_the_agent_to_one_place asserts both fields together, per your note — either one alone passes.
| for value in node.values | ||
| if isinstance(value, ast.FormattedValue) and isinstance(value.value, ast.Name) | ||
| } | ||
| names: dict[str, list[str]] = {} |
There was a problem hiding this comment.
[P2] Keep f-string substitutions local to their lexical scope
This map is keyed only by identifier across the whole module. If two functions both assign guidance, one unanchored ("Inspect source_id before proceeding anywhere else.") and one containing shipgate.yaml, both f-strings are rendered as the concatenation of both assignments; the unrelated anchor launders the violation and internal_vocabulary() returns no offenders. The RHS literals are then marked substituted, so neither is checked alone. Resolve assignments per enclosing scope/reaching definition and add a two-function same-name negative control.
There was a problem hiding this comment.
Confirmed with your exact construction, and it is worse than laundering one string: both right-hand sides were then marked substituted, so neither was checked alone either. The probe printed the concatenation twice with no offenders.
_emitted_strings now builds a lexical scope chain (_scope_chains), keys assignments by (owning scope, name), and resolves an interpolation by walking outward from the JoinedStr's own chain — first scope that binds the name wins. Only the bindings actually resolved are marked substituted, which also preserves the fix from the previous round (a name no f-string interpolates is never consumed, so guidance = "…" handed to NextAction(why=guidance) is still swept).
test_the_extractor_resolves_a_name_in_its_own_scope is your two-function negative control; it asserts exactly one of the two renders as an offender and that the offender does not carry the other scope's anchor. It sits beside the two probes that pin the other directions — the extractor is the thing every other sweep in the file depends on, so it gets its own tests.
d593ccb to
c41527e
Compare
|
All seven findings addressed at head
Two notes worth surfacing rather than leaving in the inline threads: The unresolved-handoff fix needed scoping, not removal. Dropping the root fallback outright regressed Three published values move, plus one narrowed manifest value, all written up in the Sample goldens are unchanged from the previous head — the scoping restored the same values. |
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Second-pass review of c41527ed733734162878a068f743c25dde09a2d4, after checking the replies and fixes from the previous review.
I reran the changed-area tests, Ruff, schema-generation checks, diff checks, all current GitHub workflows, and the Shipgate verifier. Those are green. The full local xdist run completed all ordinary tests; eight packaging cases could not set up because the isolated builder could not download hatchling>=1.31.0 in this restricted environment, while the repository's GitHub packaging/CI checks are green.
The inline findings below are independently reproduced behavioral gaps: some recovery actions point to deleted or wrong files, aggregated adapters select the wrong duplicate repair, unresolved handoffs still name healthy agents, and the new vocabulary/schema guards have bypasses. I consider the P1/P2 findings blocking for this change.
GitHub does not allow the authenticated PR author to submit REQUEST_CHANGES, so this is posted as a blocking COMMENT review.
| # would otherwise name `shipgate.yaml` relative to the caller's CWD — | ||
| # an unrelated trust root (#329 review). Recorded once here rather | ||
| # than at each raise site, none of which knows the manifest either. | ||
| exc.details.setdefault("manifest_path", str(config_path)) |
There was a problem hiding this comment.
[P1] Preserve checkout paths across archived verification
verify --base HEAD --head HEAD scans the committed head in a temporary archive, so config_path here is an agents-shipgate-verify-head-*/head/shipgate.yaml path. That directory is removed before the CLI handles the InputParseError. I reproduced a repeated ADK entrypoint and, after the command returned, both details.manifest_path and next_actions[0].path named the deleted temporary file. This breaks the canonical verifier recovery route (and docs/errors.json tells consumers to prefer this detail). Translate archive paths back to the logical checkout manifest before the temporary tree is cleaned up.
There was a problem hiding this comment.
Confirmed exactly as described — I reproduced verify --base HEAD --head HEAD on a repeated ADK entrypoint and both details.manifest_path and next_actions[0].path named a file under a deleted agents-shipgate-verify-head-* tree.
Fixed where the archive mapping is known: the head run_scan call in verify/orchestrator.py now has an except AgentsShipgateError that sets details["manifest_path"] = str(git_root / config_relative) before re-raising, inside the try that owns the temporary directory. run_scan still records what it read via setdefault, so this is the one frame that knows the archive is not the logical thing and overrides it; nothing else has to know archives exist.
The base scan deliberately keeps run_scan's value — its failures are caught locally as a base-comparison-unavailable state and never reach the CLI error handler.
test_an_archived_head_names_the_checkout_manifest_not_the_temporary_one asserts both the emitted path and details.manifest_path resolve to the checkout manifest and that the file still exists after the command returns, which is the property that actually broke.
| # to open. The identity triple that detected the collision is | ||
| # kept in ``details`` for machine consumers and bug reports | ||
| # (#329); the sentence names the tool and the one file to edit. | ||
| cause = ( |
There was a problem hiding this comment.
[P2] Track artifact reads rather than adapter batches
The OpenAI and Anthropic loaders aggregate all configured tool artifacts into one LoadedToolSource, so read_index identifies the adapter batch rather than the artifact read. Repeating the same configured file therefore keeps first_read == read_index; my OpenAI repro reported duplicate_in_source_artifact and told me to edit a valid JSON artifact instead of removing the repeated manifest entry. Preserve per-artifact occurrence provenance (or deaggregate these loaders) and cover repeated OpenAI and Anthropic entries.
There was a problem hiding this comment.
Confirmed. My OpenAI repro (openai_api.tools: [tools.json, tools.json]) reported duplicate_in_source_artifact and pointed at a valid file.
I took your first option rather than deaggregating: the occurrence provenance the loaders drop is still present in the manifest, and reading it there needs no loader to change. schemas/manifest/_artifacts.repeated_declared_artifacts(manifest) returns the paths any single declared list names more than once, threaded through _build_canonical_tools into build_tool_identity_catalog. The cause is now repeated_source_entry when the reads differ or the manifest declares that artifact twice, and duplicate_in_source_artifact otherwise.
Scoped to one list on purpose: a spec referenced from two different blocks is two declarations of different things, not a repeat. tool_sources is skipped because each entry is already its own read, so the existing discriminator covers it.
test_an_aggregating_loader_still_reports_a_repeated_manifest_entry is the OpenAI case end to end; the MCP in-artifact case and the ADK repeated-entrypoint case are asserted beside it so all three stay distinguished.
| # `source_pointer` next, because it is a location — `source` is | ||
| # `framework_extraction` or a bare `shipgate.yaml`, which names no | ||
| # agent and reads as jargon in a subject line. | ||
| if issue.agent_id: |
There was a problem hiding this comment.
[P2] Do not label unresolved handoffs with the healthy source agent
The production resolver emits two shapes that bypass the intended fallback: an ambiguous declaration target emits unresolved_agent_binding with the healthy source agent's ID, so this branch wins before the unresolved-kind check; a raw missing endpoint emits incomplete_handoff_graph with no ID, but that kind is absent from _UNRESOLVED_AGENT_KINDS, so it falls back to the root. Reproducing both through resolve_agent_binding_graph yielded subjects root / root [provider] even though the issue pointers identified the unresolved handoff. Prefer the unresolved target/pointer for both shapes and test through the real graph builder.
There was a problem hiding this comment.
Confirmed, and my previous fix was written against a constructed issue rather than the resolver — which is exactly how it missed both shapes. Reproducing through resolve_agent_binding_graph:
- an ambiguous handoff target emits
unresolved_agent_bindingwithagent_idset to the declaring agent andsource_pointer=/agent_bindings/declarations/0/handoffs/0; - an incomplete handoff edge emits
incomplete_handoff_graphwithagent_id=edge.source_agent_id, also healthy, with the entrypoint pointer.
So a rule keyed on "has an agent_id" could not work. It is now keyed on the kind: _UNRESOLVED_TARGET_KINDS = {unresolved_agent_binding, unresolved_bound_tool, incomplete_handoff_graph} — the kinds where something referenced did not resolve — and for those the pointer wins outright over any agent id on the issue. Everything else keeps the agent, and a kind that names none keeps the root, which is what the whole-extraction row in samples/conductor_agent needs.
Both shapes are now tested through the real builder (test_an_incomplete_handoff_is_not_labelled_by_its_source_agent, test_an_ambiguous_handoff_target_is_not_labelled_by_its_referrer), each asserting the producer really does carry the healthy agent before asserting the subject does not.
| return [ | ||
| NextAction( | ||
| kind="edit", | ||
| path="shipgate.yaml", |
There was a problem hiding this comment.
[P2] Use the resolved manifest for CHANGE_ME recovery
manifest was resolved above specifically for nested/discovered configurations, but this branch discards it. With services/billing/shipgate.yaml selected by --workspace and a missing CHANGE_ME source, the action still points to caller-relative shipgate.yaml; the shared resolver now also propagates that wrong target into verification preparation. Set the action path (and ideally the prose) from manifest.
There was a problem hiding this comment.
Confirmed and fixed — the branch was written before manifest existed and I did not revisit it. It now uses path=manifest and names it in the prose.
The bare filename was load-bearing for one assertion (test_cli.py pinned next: Edit shipgate.yaml); that test now asserts the hint names the manifest actually passed, which is the stronger claim.
| ) | ||
| return NextAction( | ||
| kind="edit", | ||
| path=str(source_file), |
There was a problem hiding this comment.
[P1] Rebase artifact edits to the manifest directory
source_file is recorded relative to the manifest, but this publishes it as if it were relative to the caller's CWD. Scanning an absolute nested manifest from another checkout produced path: "tools.json" for services/billing/tools.json, which can send an agent to edit an unrelated file in the caller's repository. Resolve relative artifact paths against Path(manifest).parent before emitting this edit action.
There was a problem hiding this comment.
Confirmed. source_file comes off the tool as a manifest-relative path and the action publishes one string that a consumer opens verbatim, so a nested manifest declaring tools.json produced path: "tools.json" in the caller's repository.
_artifact_beside_manifest rebases a relative artifact path against Path(manifest).parent; absolute paths and a manifest that resolves to no directory are left alone. test_a_duplicate_inside_an_artifact_edits_the_artifact now asserts Path(action["path"]) == workspace / "tools.json" rather than the bare name.
| #: The patterns themselves live in :mod:`agents_shipgate.core.adopter_text`, | ||
| #: which owns the reader-facing half of the same rule. Two copies would drift, | ||
| #: and the drift would be silent in exactly the direction that matters. | ||
| DERIVED_ID_PATTERNS: tuple[tuple[str, re.Pattern[str]], ...] = ( |
There was a problem hiding this comment.
[P2] Enforce the invariant for every derived-ID shape
The shared vocabulary defines observation and fingerprint shapes too, but the report conservation guard imports only tool and agent IDs. Because a source_warning is copied directly into an EvidenceGap.subject, subjects containing obs_v1_... or fp_... currently pass semantic validation. That contradicts the new claim that any derived ID is refused in adopter-facing subjects. Include all applicable shared shapes, or apply the common vocabulary check to these subjects.
There was a problem hiding this comment.
Confirmed, and it is the same mistake this PR was written to fix, one level down: I widened the rule from tool ids to "any derived id" and then wired only the two shapes I had seen in a subject.
DERIVED_ID_PATTERNS now carries all four — tool, agent, observation, finding — so a source_warning copied verbatim into EvidenceGap.subject cannot smuggle an obs_v1_… or fp_… past validation. The article in the error message is derived from the noun rather than special-cased, since "an observation" was about to be wrong too.
| bindings = _lexical(chains[id(node)], value.value.id) | ||
| if not bindings: | ||
| continue | ||
| local[value.value.id] = [ |
There was a problem hiding this comment.
[P2] Validate reaching definitions and branches separately
_lexical returns every assignment in a scope, and _render_literal also concatenates the two mutually exclusive IfExp branches. An anchored definition/branch can therefore launder an unanchored one: assigning guidance = "Inspect source_id before proceeding", interpolating it, then assigning anchored text later produces one combined string and no offender. Track the definition that reaches each use and preserve branch alternatives as separate strings; add negative controls for both cases.
There was a problem hiding this comment.
Both confirmed with your constructions, and both fixed.
Reaching definitions. _reaching resolves lexically and then positionally: the innermost scope binding the name, and within it the last assignment at or above the use. A use above every definition in its scope is a forward reference (a closure called later), so there every definition stays reachable and each is kept as an alternative rather than merged.
Branch alternatives. _literal_alternatives returns one string per IfExp arm instead of concatenating them, and _cartesian expands a name bound to several alternatives into one rendering per choice. So a conditional with one anchored and one unanchored arm now yields two strings, and the unanchored one is checked on its own.
Negative controls for both are in test_the_extractor_resolves_a_name_in_its_own_scope and the two probes beside it; I verified them against your exact guidance examples before writing the tests.
| suffix = f" Did you mean {exc.suggestion}?" if exc.suggestion else "" | ||
| typer.echo( | ||
| f"Unknown fingerprint: {exc.fingerprint}.{suffix}", err=True | ||
| f"Unknown fingerprint: {exc.fingerprint}.{suffix} No entry in " |
There was a problem hiding this comment.
[P2] Keep fingerprint-shaped IDs out of the error prose
The close-match error still prints both the rejected and suggested fp_... values; internal_vocabulary() flags both. This module is omitted from the static sweep for the documented --help carve-out, so the actual error path is never checked, while the PR documents only help as exempt. Either add a narrowly scoped, explicitly tested exception for this error context or keep these values in structured error fields rather than adopter-facing prose.
There was a problem hiding this comment.
Confirmed, and the sharper half of the finding is that the module was outside the sweep, so the claim was untested either way.
cli/explain_finding.py is now in ADOPTER_FACING_MODULES, listed in MODULES_WITH_GIVEN_IDS — a named, documented set whose only effect is ids_are_given=True, which drops the id shapes and nothing else. The term rules still apply to that module, so the help and the error still have to name findings[].fingerprint in report.json, which is what makes the echoed value locatable.
The flag was renamed from ids_are_prefilled to ids_are_given because it now covers both cases where the reader is not asked to decode anything: the tool filled it in (the declaration scaffold), or the reader supplied it (this error). test_the_given_id_allowance_is_narrow_and_still_checks_the_terms pins that the carve-out exempts the shape and refuses a native_locator sentence in the same module — otherwise listing a module here would be a way to smuggle a whole file past the sweep.
| mode: str | None = None | ||
| optional: bool = False | ||
|
|
||
| @field_validator("id", mode="before") |
There was a problem hiding this comment.
[P2] Publish the nonblank constraint in JSON Schema
This field_validator is invisible to generated JSON Schema. The runtime model rejects tool_sources[].id: " ", while docs/manifest-v0.1.json accepts it and generate_schemas.py --check remains green. Since the published schema is part of the manifest contract, encode a JSON-Schema-visible non-whitespace constraint and add runtime/schema parity tests for empty and whitespace-only IDs.
There was a problem hiding this comment.
Confirmed — docs/manifest-v0.1.json accepted " " while the runtime refused it, and generate_schemas.py --check stayed green because the validator is invisible to it.
ToolSourceConfig.id now carries Field(json_schema_extra={"pattern": r"\S"}), which reaches the published schema as "pattern": "\\S". It is a search rather than an anchor, so " orders " passes and is then stripped, while "" and " " fail in both places. The field_validator stays because it owns the message — it names the keys the id is joined on, which a generic constraint error cannot.
Two statements of one rule need a guard, so test_the_published_schema_and_the_runtime_agree_about_source_ids validates the same document against docs/manifest-v0.1.json with jsonschema and against the model, and asserts the two verdicts are equal across plain, padded, empty, whitespace-only, and tab/newline ids.
| f"member source_id={source_id!r}, tool={tool!r} " | ||
| f"matched {match_count} observations" | ||
| f"member source_id={source_id!r}, tool={tool!r} matched " | ||
| f"{match_count} {noun} in that source, not one — narrow it at " |
There was a problem hiding this comment.
[P3] Give zero matches a possible repair
A configured nonempty source can still have match_count == 0 when the exact named tool is absent. Narrowing that exact source_id/tool selector cannot create a match, so this advice is impossible to follow. Ask the adopter to correct the selector so it names exactly one tool (while retaining “narrow” for multiple matches), and test both zero and many.
There was a problem hiding this comment.
Confirmed — I had folded both directions into one sentence and only the "many" direction was possible to follow.
Split by count: zero matches reads matched no tool in that source — correct the member at shipgate.yaml#tool_identity.bindings[].members to name a tool that source exposes, and above one keeps narrow the member … so it names one. Both are asserted in tests/test_evidence_gap_ranking.py, alongside the two zero-cases that have their own repair (produced no tool observations, no tool source with id) so the four stay distinct.
c41527e to
070e905
Compare
|
All twelve second-pass findings addressed at head
Three of these are worth calling out beyond the table. Two of my earlier fixes were right in principle and wrong in reach. The handoff-subject rule was written against a hand-constructed issue, so it missed both shapes the real resolver emits — an ambiguous declaration target carries the declaring agent's id outright, and an incomplete edge carries the source agent's. Testing through The aggregated-loader finding had a fix that needed no loader to change. The provenance those loaders drop is still present in the manifest — a path declared twice in one list is a repeated entry whatever the adapter did with it — so the answer is read from the config rather than inferred from observations that genuinely cannot tell the two apart. All three duplicate scenarios (ADK repeated entrypoint, MCP in-artifact, OpenAI aggregated) are now asserted side by side. The
One thing I have not changed and would flag rather than decide alone: |
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Third-pass review of 070e905f, after re-reading the prior threads and testing the follow-up fixes. I reproduced 12 remaining issues (5 P1, 7 P2); each inline comment includes the concrete failure shape.
The highest-risk cluster is recovery provenance: exact machine actions can still target a different checked-out ref/trust root, a nonexistent nested path, or a JSON pointer posing as a file. Separately, raw artifact spellings can let one physical tool file enter the catalog twice, and the new adopter-vocabulary guard has several executable bypasses.
Validation on the exact head:
- focused CLI/config/identity/vocabulary/evidence tests passed;
ruff check .,compileall, schema generation check, andgit diff --checkpassed;- the repository's source-entrypoint Shipgate verifier returned
control_state=complete/decision=passed; - all three GitHub workflow runs are green;
- the full local suite completed with only the eight packaging tests erroring at their shared isolated-build fixture because this restricted environment could not download
hatchling>=1.31.0; the hosted packaging CI is green.
GitHub currently reports this PR as non-mergeable because main advanced (the local merge preview confirms conflicts), so the final resolution needs another CI/review pass after rebasing. Since the authenticated account owns this PR, GitHub only permits a COMMENT review here; these findings still block my sign-off.
| "Referenced file is present, parseable, and inside the manifest directory." | ||
| ), | ||
| ), | ||
| action=input_parse_recovery(exc)[0], |
There was a problem hiding this comment.
[P1] Pass the resolved manifest into this recovery call. verification prepare calls the input loader directly, so run_scan never injects details.manifest_path; the resolver therefore falls back to the literal shipgate.yaml. I reproduced this with a repo whose only manifest is services/billing/shipgate.yaml and whose source is CHANGE_ME.yaml: verification prepare --config services/billing/shipgate.yaml emits kind: edit, path: shipgate.yaml, targeting a different trust root. Thread the exact resolved manifest (and cover the prepare path), rather than relying on details that this caller cannot supply.
There was a problem hiding this comment.
Confirmed — prepare builds its plan through the input loader, so nothing injects manifest_path and the resolver fell back to the literal shipgate.yaml.
Fixed at the call site, which is the frame that knows: input_parse_recovery(exc, manifest_path=root / config_relative). That is the same value prepare already computed for the plan, so there is no second resolution to drift.
Also worth noting for the general shape of this: run_scan's hook uses setdefault, so a caller that knows better can always supply the answer, and a caller that does not still gets one.
| # that no longer exists by the time anyone read it (#329 | ||
| # review). The logical manifest is the same relative path in | ||
| # the checkout, which is what the reader opens. | ||
| exc.details["manifest_path"] = str(git_root / config_relative) |
There was a problem hiding this comment.
[P1] Do not project every archived head onto the live checkout. With commit A containing the duplicate and checkout B containing the fix, verify --base A --head A emits an edit action for B's existing shipgate.yaml, even though B no longer contains the repeated entry. Also, this rewrites only details.manifest_path; a generic missing/malformed input remains named in message by its temporary archive path, and the generic recovery tells the caller to inspect that path after it has been deleted. Bind recovery to the evaluated ref/blob, or map to the checkout only when the archived head is demonstrably the live tree and translate every actionable artifact path.
There was a problem hiding this comment.
Both halves confirmed, and the projection was the wrong idea rather than a wrong detail.
Ref binding. The checkout is only a valid substitute when the archived head is the checkout, so that is now the condition: head_tree == tree_sha(git_root, "HEAD"). When it is not, no path is published at all — details.evaluated_ref and details.manifest_in_ref are set instead, and the action is a review saying "this failure is in <path> as of <ref>, which is not the tree you have checked out". I verified your exact A/B setup: commit A with the duplicate, checkout B with the fix, verify --base A --head A now names A and publishes no path.
The prose. You are right that rewriting only details left the message naming the archive. The archive root is a prefix this function constructed, so stripping it from exc.args is a substitution rather than a guess: Input file not found: /tmp/…/head/tools.json becomes Input file not found: tools.json, which is true of whichever tree was evaluated, and evaluated_ref says which. Verified on a missing-input repro that no agents-shipgate-verify-head- string survives anywhere in the envelope.
I did not translate artifact paths, because they are no longer published as actionable — see the reply on diagnostics.py:315.
| candidate = Path(source_file) | ||
| if candidate.is_absolute(): | ||
| return source_file | ||
| parent = Path(manifest).parent |
There was a problem hiding this comment.
[P1] source_file is not universally relative to the manifest. I reproduced a root manifest declaring agents/agent.py, where that ADK entrypoint mounts McpToolset(inventory_path="inventory.json"); a duplicate in agents/inventory.json produces an edit action for nonexistent <repo>/inventory.json. The loader resolved the inventory relative to the entrypoint, but this join rebases it a second time against the manifest. Carry a resolved manifest-relative/absolute origin (or its actual base) in the typed provenance instead of assuming one base here.
There was a problem hiding this comment.
Confirmed, and this one changed my mind about the approach rather than the arithmetic.
There is no base to rebase against. source_file is manifest-relative for most sources and entrypoint-relative for an inventory a framework file mounts, and the loader does not record which — so any join here is a guess, and the previous round's rebase was simply a different wrong answer than the bare path had been.
So no declared artifact is published as a routable path any more. The duplicate-inside-an-artifact case is a review action naming the artifact and the tool source in its sentence, where the filename is something to grep for rather than something to open; only the manifest, which the run demonstrably read, is ever an edit target. That also removes _artifact_beside_manifest entirely.
This is the option you offered on the round-2 thread ("emit an explicit review"), which I should have taken then instead of computing a path I could not justify.
|
|
||
| if tool.source_path: | ||
| return tool.source_path | ||
| for candidate in (tool.source_ref, tool.source_location, tool.source_pointer): |
There was a problem hiding this comment.
[P1] Do not treat source_location and source_pointer as file paths by default. They are distinct optional provenance fields that a third-party adapter may legally populate without source_path/source_ref: duplicate tools with source_location="agent.py:12" emit kind: edit, path: agent.py:12, while source_pointer="/tools/0" emits an absolute edit target /tools/0. Only a structured path—or a positively recognized file-backed locator—should authorize an edit; otherwise use the existing review fallback.
There was a problem hiding this comment.
Confirmed for both fields.
_source_file is now: source_path if set — the structured field, which the adapters that set it set to a path and nothing else — else source_ref only when it contains no #, which is the shape the plain-path producers write (Google ADK, MCP). source_location and source_pointer are gone from the chain: they are separate optional provenance fields, a third-party adapter may legally populate either alone, and agent.py:12 and /tools/0 are not files.
The fragment-parsing heuristic went with them. A value that names no file is worth less than no value, because the caller already has a review route for exactly that case.
test_only_a_field_that_names_a_file_can_name_a_file covers all six shapes including your two.
| seen: set[str] = set() | ||
| for item in value: | ||
| if isinstance(item, ArtifactPathConfig) and item.path: | ||
| if item.path in seen: |
There was a problem hiding this comment.
[P1] Normalize the physical artifact identity and preserve which tool-producing list it came from. With openai_api.tools: [tools.json, ./tools.json], the same file is read twice but this returns no repeat and the scan succeeds with two canonical pay tools; [./tools.json, ./tools.json] instead misclassifies the valid file as internally duplicated. In the other direction, repeating tools.json only under unrelated openai_api.test_cases poisons this global set and makes a real MCP in-file duplicate look like a repeated tool source. A global set of raw spellings cannot select the repair; retain normalized (section/list, resolved path) occurrence provenance.
There was a problem hiding this comment.
All three sub-cases confirmed, and the third — the test_cases poisoning — is the one that showed the set was keyed on the wrong thing.
Three changes:
- Only tool-producing lists count.
TOOL_PRODUCING_ARTIFACT_LISTSnames them, andtests/test_config.py::test_every_declared_artifact_list_is_classifiedwalks the manifest model and fails if any artifact list is neither in that set nor explicitly excluded — so a new list cannot arrive unclassified and inherit an answer. - Repeats are counted within one list, not globally, so a path under
openai_api.test_casescannot say anything aboutopenai_api.tools. - Paths are normalized (
posixpath.normpath) on both sides of the join, through one function, sotools.jsonand./tools.jsonare one spelling.
That leaves the first sub-case, which is the substantive one: two spellings of one file were two declarations, so the identities never collided and the catalog gained two pay tools with no error at all. Normalizing only the repeat set would not have fixed that. ArtifactPathConfig.path is now canonicalized at the boundary, so the two spellings are one declaration and the existing duplicate check sees it — verified: [tools.json, ./tools.json] now reports repeated_source_entry instead of succeeding. Backslash-bearing paths are left alone (not this function's to reinterpret) and ../outside.yaml is preserved exactly, so the containment checks still see what the author wrote.
I did not go as far as (section, resolved path) provenance carried on the tool: the tool does not know which list it came from, and mapping source_type back to a list would be another table to drift. Restricting to tool-producing lists closes the reported failure without one. The residual is a cross-framework coincidence — the same path in two different tool-producing lists, one with a genuine in-file duplicate — which I am happy to take if you would rather have the exact provenance.
| if not keys: | ||
| return _literal_alternatives(node, names) | ||
| rendered: list[str] = [] | ||
| for key in keys: |
There was a problem hiding this comment.
[P2] This is not a Cartesian product when two or more names have alternatives. With left = unanchored source_id | anchored shipgate.yaml and right = unanchored source_type | anchored report.json, this emits four synthetic strings that each retain an anchor from the other unspecialized name, while never emitting the real unanchored/unanchored branch. I reproduced every returned string passing internal_vocabulary(). Use itertools.product across all value lists and add a two-variable regression.
There was a problem hiding this comment.
Confirmed with your exact construction — I verified every returned string passed before fixing it, including the missing unanchored/unanchored branch.
It is itertools.product across all value lists now, so each rendering specializes every multi-valued name at once. Your case produces four strings, one of which is Inspect source_id first. Then check source_type. and is reported with both offenders. The two-variable probe is in the scratch harness I used; the committed regression is the two-arm and two-definition cases, and I have added the two-variable one alongside them.
| # before a report exists. `cli/scan/declarations.py` is deliberately absent — | ||
| # it emits one assembled file, swept whole above, and its per-line fragments | ||
| # would read as violations of a rule the file satisfies. | ||
| ADOPTER_FACING_MODULES = ( |
There was a problem hiding this comment.
[P2] The sweep still omits adopter-facing third-party adapter failures. inputs/adapter_validation.py emits messages such as source_type 'mcp' is reserved by a built-in adapter, and report/markdown.py prints every validation_errors/runtime_errors entry, but that producer is absent here and those two JSON keys are absent from ADOPTER_FACING_KEYS. Thus a current rendered error still leaks the bare internal term while all guards pass. Include this producer/output surface and pin a real adapter-validation message.
There was a problem hiding this comment.
Confirmed, and the leak was live: report/markdown.py prints every validation_errors / runtime_errors entry under Loaded Adapters, so source_type 'mcp' is reserved by a built-in adapter was reaching adopters while every guard passed.
Three changes: inputs/adapter_validation.py is in ADOPTER_FACING_MODULES; validation_errors and runtime_errors are in ADOPTER_FACING_KEYS; and the three messages now name shipgate.yaml#tool_sources[].type rather than the class attribute. That spelling serves both readers — it is the manifest key the adopter edits, and it is unambiguous to the adapter author about which value collided.
test_a_real_adapter_validation_message_is_swept drives validate_adapter_entry_point with a colliding adapter and pins the real emitted message, rather than asserting against a string I wrote in the test.
| swept = 0 | ||
| for lineno, text in _emitted_strings(REPO_ROOT / "src/agents_shipgate" / module): | ||
| stripped = text.strip() | ||
| if not stripped or _KEY_LIKE.match(stripped): |
There was a problem hiding this comment.
[P2] Do not apply key/fragment heuristics to strings already known to be emitted. Probes containing typer.echo("native_locator") and typer.echo("Inspect source_id before proceeding.") are both returned by _emitted_strings, yet this loop records no offender: the first looks key-like and the second has fewer than five words. These are complete console messages, not structured keys or fragments. Preserve emit-site AST context and limit the exemptions to values actually consumed into a separately checked expression.
There was a problem hiding this comment.
Confirmed with both probes — neither was reported.
The two exemptions exist for strings whose emit site is unknown: a dict key that reads like prose, a fragment assembled elsewhere. Neither can be true of a string handed straight to typer.echo or to a NextAction field, so the extractor now records that context (_definitely_emitted) and a known emit site suspends both. _emitted_strings returns (line, text, definite), and the sweep applies the full rule whenever definite is set.
The emit sites are the first argument of echo/secho and the values of the message-bearing keywords (why, expects, title, message, reason, recommendation, remediation), including both arms when one of those is a conditional. Verified against your two probes: typer.echo("native_locator") and the four-word source_id sentence are both reported now, while {"native_locator": 1} as a dict key is still exempt.
| """ | ||
|
|
||
| offenders = {term for term in INTERNAL_ONLY_TERMS if term in text} | ||
| if not ids_are_given: |
There was a problem hiding this comment.
[P2] Scope the allowance to the ID kind/value that was actually supplied. explain-finding is given a fingerprint only, but ids_are_given=True suppresses tool, agent, and observation ID shapes throughout the whole module; the declaration scaffold similarly pre-fills a tool ID but would forgive every other shape. For example, internal_vocabulary("Agent agent_v1:7205d836e4b3fee2 could not be resolved", ids_are_given=True) is empty. Pass allowed patterns or concrete values instead of a module/surface-wide boolean.
There was a problem hiding this comment.
Confirmed — your example returned empty, and a module-wide boolean was the wrong shape for a per-surface fact.
internal_vocabulary now takes given_id_kinds: Iterable[str] and drops only those kinds; an unknown kind raises rather than silently forgiving nothing. MODULES_WITH_GIVEN_IDS became a mapping, so explain-finding gets {"finding"} and the declaration scaffold is checked with {"tool"}. Your exact string — an agent id in explain-finding prose — is an offender again.
test_the_given_id_allowance_is_narrow_and_still_checks_the_terms now pins three things together: the echoed fingerprint passes, an agent id under the same allowance does not, and a native_locator sentence is refused either way. Otherwise listing a module there would still be a way to smuggle a file past the sweep.
| "next_actions" | ||
| ], | ||
| "recovery_hint": "Follow `next_actions[]`. For scan/doctor tool-source failures, run `agents-shipgate doctor -c shipgate.yaml --json` to inspect `unresolved_sources[]`. For report-consuming commands, inspect `source_report` when present (or the `--from` path in the message) and regenerate a current `report.json` if needed." | ||
| "recovery_hint": "Follow `next_actions[]`. For scan/doctor tool-source failures, run `agents-shipgate doctor -c shipgate.yaml --json` to inspect `unresolved_sources[]`. For report-consuming commands, inspect `source_report` when present (or the `--from` path in the message) and regenerate a current `report.json` if needed. `message` and `next_actions[]` name files, symbols, and manifest keys only; when a failure has internal identifiers behind it they are in `details`, which is for routing and bug reports and is never required to act on the error. Route on `details.failure` when it is present rather than pattern-matching the prose. `duplicate_tool_in_source` means one tool source produced the same tool twice; `details.cause` says which repair applies — `repeated_source_entry` (the manifest names one artifact twice; edit `details.manifest_path`) or `duplicate_in_source_artifact` (one artifact defines the tool twice; edit `details.source_file`). `details.manifest_path` is the manifest the run actually read, which is not always the path the command was spelled with, and for an archived `verify --base/--head` is translated back to the checkout — prefer it over reconstructing one from the invocation. Every `path` in `next_actions[]` is resolved the same way and is safe to open verbatim." |
There was a problem hiding this comment.
[P2] This tells consumers to edit details.source_file, although the same paragraph says details are never required to act and that field is not verbatim-openable. For a nested manifest it can remain tools.json while next_actions[].path is services/billing/tools.json; nested entrypoint-relative artifacts diverge further. Direct consumers exclusively to the resolved action path, or make the details field carry the same resolved identity before documenting it as an edit target.
There was a problem hiding this comment.
Confirmed — the paragraph said both things and only one of them was true.
Rewritten to lead with the rule rather than the fields: open only next_actions[].path, which is the one field resolved for the caller and omitted entirely when the failure belongs to a ref that is not checked out. details is documented as routing and bug-report material whose paths are recorded as the loader saw them and are not verbatim-openable, and it no longer names any of them as an edit target. The routing keys it does document are the ones that are safe to switch on: failure, cause, manifest_placeholders, evaluated_ref.
That is now true rather than aspirational, since no declared artifact is published as an actionable path at all — see the diagnostics.py:315 thread.
070e905 to
9db29bc
Compare
…tity model (#329) Invariant 5 of #327. Running the tool on your own repository for the first time could produce: Duplicate tool observation identity: source_type='google_adk_function', source_id='google_adk:agent.py', native_locator='agent.py#map_salesforce_account_to_sap_bp' Three internal concepts, none of them in the manifest that person wrote, two of them derived — and the one recoverable fact, that a file was listed twice, unstated. #321 fixed the collision behind it; the message shape was a separate and more general problem, and nothing tracked it. Every string whose purpose is to tell a person what to do next now names a file, a symbol, an agent, or a manifest key: console output, the agent-mode `message` / `next_action` / `next_actions[]`, `agent-handoff.json` prose, `fix_task.instructions[]`, and PR comment text. Internal identifiers move to an additive `details` object on the error envelope, where a machine consumer or a bug report can still read them. `report.json` evidence blocks and the tool catalog are untouched — they are the identity model, and they are supposed to be precise. Two defects the audit found beyond the reported one: - A binding gap whose issue named no tool fell back to the derived agent id, so `samples/conductor_agent` shipped "the agent's tool binding graph is incomplete (agent_v1:7205d836…)" as the sentence under its verdict. The report's conservation invariant, which already refused a raw *tool* id in a gap subject, now refuses any derived id, matched by shape — a guard scoped to one kind of identifier passes vacuously for every other one. - `scan` and `verify` each caught `InputParseError` and wrote their own recovery, so a failure with a precise route on one command got generic advice on the other. One resolver now serves both and the assembly path. Review round (seven findings, all reproduced before fixing): - The emitted `edit` action named the CLI's raw `-c` spelling, so a `scan --workspace <repo>` that discovers a sole nested `services/billing/shipgate.yaml` published `path: "shipgate.yaml"` — an unrelated trust root in the caller's cwd. `run_scan` now records the manifest it read on the way out, once, for every caller. - A tool read twice is either a repeated manifest entry or a duplicate definition inside the artifact, and the action carries one path. The check reports which cause it saw and the action follows it, instead of naming both. - `_source_file` returned `source_ref` verbatim, which several adapters write as a locator (`spec.yaml#/paths/…`); an edit action must name a file. - `tool_sources[].id` accepted blank and padded values, so a manifest error was reported as a defect in Agents Shipgate. It is stripped and refused at load — it is the key `tool_inventories[]` and `tool_identity.bindings[]` join on, and both were already stripped where they are declared. - The derived-id patterns matched inside adopter-controlled names, so an agent named `customer_agent_v1_deadbeef` would abort an otherwise valid scan through the conservation invariant. They now require a token boundary and the producer's exact separator, and live in one module. - `unresolved_agent_binding` carries no `agent_id` *because the agent did not resolve*, so falling through to the root labelled `root -> missing_worker` as `root [sdk]`. The root fallback is scoped to whole-graph kinds. - `incomplete_tool_identity` published `path: shipgate.yaml#tool_identity` while its `expects` named `tool_sources`, sending the agent and the human to different sections. - The guard's own AST sweep resolved names module-wide, so two functions both assigning `guidance` were rendered as the concatenation of both and an unrelated `shipgate.yaml` laundered the offender. Resolution is lexical. `tests/test_adopter_vocabulary.py` is the guard: it enumerates the adopter-facing strings four ways — every evidence-gap kind through the real renderers, every published message builder, every hand-written string at an emit site in the modules that produce this output, and the shipped sample artifacts — plus three end-to-end runs, and it pins its own extractor. Closes #329 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9db29bc to
51ef95f
Compare
|
Rebased onto Three of these changed the approach rather than the arithmetic, and they are the ones worth reading: Recovery paths: the rule replaced the fixes. Round 2 had me computing an artifact path and rebasing it against the manifest. You showed that base is wrong for an inventory a framework file mounts — and the honest conclusion is that a declared artifact has no single base, so any join here is a guess and my rebase was just a different wrong answer. No declared artifact is published as a routable The derived-ID guard needed position and provenance, not a better boundary. You are right that The The rest, briefly: Verification on this head: One note on my own process, since it cost you a round: Still open for you rather than me: the |
1 participant
Closes #329. Invariant 5 of #327.
The reported defect
Running the tool on your own repository for the first time could produce:
Three internal concepts, none of them in the manifest that person wrote, two of them derived — and the one recoverable fact, that a file was listed twice under
google_adk.python_entrypoints, is the one thing the message does not say. It now reads:…and the next action is an
editon the manifest rather than "inspect the file referenced in the error". The identity triple moves to a newdetailsobject on the agent-mode envelope, where a machine consumer or a bug report can still read it. #321 fixed the collision behind this failure; the message shape was a separate problem and nothing tracked it.The rule
Internal identity vocabulary may appear as evidence in machine-read artifacts. It may not appear in anything whose purpose is to tell a person what to do next.
report.jsonevidence blocks,tool_catalog,identity_assessment, and the verification artifacts are untouched — they are the identity model and are supposed to be precise.Two categories, because the terms are not equally unlocatable (
core/adopter_text.py):native_locator, observation ids, and derivedobs_v…/tool_v…/agent_v…/fp_…shapes, matched by shape rather than by the field name that carries them. There is no manifest key, no field, no file to send the reader to.source_id,source_type,fingerprint. These really are keys the adopter owns (tool_identity.bindings[].members[]takes the first two,tool_inventories[].source_idandagent_bindings.root.source_idthe first,findings[].fingerprintthe third). Spelled with the surface they belong to they are locatable; spelled bare they are the model leaking.Checked on rendered messages, not fragments: a list of selector keys is fine next to
at shipgate.yaml#tool_identity.bindingsand meaningless on its own.Two defects the audit found beyond the reported one
A digest was the subject of a shipped verdict. A binding gap whose issue names no tool falls back to the agent, and the fallback was the derived agent id — so
samples/conductor_agentshippedInsufficient evidence: the agent's tool binding graph is incomplete (agent_v1:7205d836…)as the sentence under its verdict, printed again inagent_summary.first_recommended_action.why, the CLIImprove evidence:line, and the GitHub step summary. It now reads(durable_order_agent [conductor_workflows]). The report's conservation invariant — which already refused a raw tool id in a gap subject — now refuses any derived id: a guard scoped to one kind of identifier passes vacuously for every other one, which is exactly how this survived #404 and #408.scanandverifydisagreed about the same failure. Each caughtInputParseErrorand wrote its own recovery, so the precise route existed on one command and not the other — the second-implementation bug class from #322. One resolver (cli/diagnostics.input_parse_recovery) now servesscan,verify, and the verifier assembly path, routing on the typeddetails.failurekey rather than on message text, so the sentence a human reads stays free to change without breaking the machine route.The guard test
tests/test_adopter_vocabulary.pyenumerates the adopter-facing strings four ways, because no single enumeration reaches all of them:EvidenceGapkind the report schema declares (30 of them), pushed through the real renderers —evidence_gap_headline,evidence_gap_action_text, andfix_task.instructions[]. A new gap kind without adopter-facing wording fails here.core.source_warnings, plus the grouped display projection and the declaration scaffold, called and checked as the reader sees them. The sweep table is checked against the module's own__all__, so a new builder cannot arrive unswept.report.jsonandreport.md, with a non-empty assertion so an unmatched glob cannot make the sweep vacuous.Plus three end-to-end runs: #321's failure walked through
scanand again throughverify, and one realinsufficient_evidenceverdict whoseagent-handoff.json,verifier.json, and PR comment are swept as written. And negative controls over the exact strings that shipped, so the rule cannot pass by doing nothing.Other messages changed
Tool observation obs_v1_3f9a1c…→ names the tool and the filematched 0 observations→matched 0 tools in that source, not one — narrow it at shipgate.yaml#tool_identity.bindings[].membersSHIP-DIAG-UNKNOWN-ADAPTER-SOURCE-TYPEUnknown adapter source_type 'acme'→No adapter handles tool_sources[].type 'acme' in shipgate.yamlincomplete_tool_identityaccepted_valuesstable_native_locator→stable_source_pathInputParseErrorsexplain-findingunknown fingerprintfindings[].fingerprintin the report it readCompatibility
No version moves —
contract_version,report_schema_version,minimum_control_contract_version, and every published schema document are unchanged. No field removed or retyped, no error kind added, no exit code moved. One additive envelope field (details, documented indocs/errors.json), one gapsubjectvalue change in two samples, and prose. Migration note inSTABILITY.md; the rule is written up for contributors inCONTRIBUTING.md.Verification
ruff check .clean;compileallclean;generate_schemas.py --checkclean.pytest -n auto), includingtest_adapter_static_only.py.conductor_agent(report.json + report.md) andsupport_refund_agent(report.json + packet.json); the diff is the subject value and nothing else.Deliberately out of scope
Report internals stay precise, per the issue's non-goals. Two adopter-visible spots are documented carve-outs rather than fixes:
explain-finding --help, whose argument is a fingerprint and which says which field to copy it from, and thetool_ida declaration template pre-fills for the reader to paste — removing that would put back the same-name ambiguity #388 closed. Both are recorded incore/adopter_text.py.🤖 Generated with Claude Code