Skip to content

feat(evidence): ask only what the scanner cannot prove (#410 increment 2) - #416

Merged
pengfei-threemoonslab merged 10 commits into
mainfrom
claude/increment-2-prefilled-effects-7d668a
Aug 25, 2026
Merged

feat(evidence): ask only what the scanner cannot prove (#410 increment 2)#416
pengfei-threemoonslab merged 10 commits into
mainfrom
claude/increment-2-prefilled-effects-7d668a

Conversation

@pengfei-threemoonslab

@pengfei-threemoonslab pengfei-threemoonslab commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Increment 2 of the evidence-first declaration RFC (#410): pre-filled effects, the questionnaire, and the progress counter.

Why

Adoption stalls at a wall of blanks. The fourth adk-samples#1745 walk (recorded on #410) faced 0/12 pass-eligible actions and a report that described the work as "24 semantic evidence gaps" — a symptom count with no order and no finish line — while the same report already held a derived financial_write reading for the one tool that moved money. Declaring 2 of 12 tools was enough to reach a verdict; nothing in the product said which 2.

What changed

Effects the scan observed are pre-filled. suggested-declarations.yaml now prints the readings behind each effect question and, where they support one conservative answer, offers that answer in the effect: line instead of <REVIEW_REQUIRED>:

# ── Question 1 of 2 · effect · z_refund_payment [src] ───────────────────────
# What this scan read this action's effect as:
#   financial_write — risk_hint:auth_scope
# Assumed in the absence of evidence, and never proposed from:
#   write — mcp_protocol_default
# Proposed below: financial_write — at or above every reading here, so
# confirming it can only over-declare. Replace it if you can defend a
# different reading.
---
# closes: missing_effect_evidence
# merge into: shipgate.yaml#action_surface.actions[tool='z_refund_payment']
tool: z_refund_payment
tool_id: tool_v2_240558ab…
source_id: src
# proposed from the evidence above — keep it to confirm, or replace it.
# accepted: read | write | destructive | external_communication | …
effect: financial_write

Why pre-filling is safe, mechanically rather than editorially:

The file is a numbered questionnaire. Blocks carry Question 3 of 5 banners ordered by how much answering them can move the verdict — money, outward communication, destruction first, which is what reached a verdict in two answers on the walk. One block answers both of an action's questions where it has two (Questions 1–2 of 2), and a counted question with no blank to fill — a source conflict — is still numbered and shown, so the numbering never skips.

One order, everywhere. evidence_gaps now places declaration-question rows in the same order the questionnaire asks them, so the CLI's Improve evidence: / Next action: row and "Question 1" name the same action. The permutation is restricted to question rows trading places with each other; binding rows, unenumerated surfaces, and identity conflicts keep their positions exactly.

Progress, from one rendering. The CLI, the questionnaire header, and the PR comment all print the same sentence:

Declaration questions: 1 of 2 answered; 1 open (1 authority).

A question is not the same thing as a declaration. The denominator counts only what both halves can be measured on — the effect and authority of one action_surface.actions row — and answered is the exact counterfactual: dimensions that gap when the same action is re-resolved without its declaration. An action whose effect an OpenAPI method or an MCP annotation established was never asked and never enters total, so a repository cannot improve its progress bar by restating what the scan already knew. (Verified: samples/clean_read_only_agent declares both dimensions and reports 0 of 0.)

Bug found and fixed on the way

The exhaustive proposal sweep crosses heuristic readings with policy-eligible structural evidence, and that turned up a live defect in the resolver: the read/side-effect conflict branch read the manifest's own risk_tags as source evidence. Declaring risk_tags: [code_execution] on a tool whose server published readOnlyHint: true was reported as "high-confidence read and side-effect evidence conflict" attributed to tool_source — but the side-effect half was the reviewer's own line. This also broke the risk_tags repair declaration_below_inferred_evidence publishes: it could not close the row it was printed on whenever the action carried a read-only annotation. The conflict now excludes DECLARATION_CLAIM_SOURCES, the set that already exists to name exactly those claims. Two sources disagreeing is still a conflict.

Measured

samples/google_adk_agent with its action_surface removed — the same shape as the walk, in-tree and reproducible:

before after confirming the one proposed value
verdict review_required blocked
counts critical=0, high=1 critical=2, high=3
blockers 0 SHIP-POLICY-APPROVAL-MISSING and SHIP-ACTION-FINANCIAL-WRITE-CONTROL-MISSING, both naming request_refund_approval
declaration questions 0 of 2 answered; 2 open (1 effect, 1 authority) 1 of 2 answered; 1 open (1 authority)
human values typed 1 — effect: financial_write, which the questionnaire had already filled in

Same two critical checks the fourth adk-samples#1745 walk reached after hand-writing six lines of YAML for two tools. Before this change the file offered effect: <REVIEW_REQUIRED> and the report said "2 semantic evidence gaps".

Surface discipline

Headline metric: time-to-first-gateable-verdict (and its companion from the walk, questions-to-first-blocker). No new CLI command or sub-app; everything extends scan, verify, and the existing advisory artifact. Two additive report fields — semantic_coverage.declaration_questions and evidence_gaps[].next_action.observed_readings[] — neither of which gates.

Compatibility

Report 0.36 → 0.37; packet 0.13 → 0.14 and verifier 0.10 → 0.11 follow because they embed the same block. All prior versions keep their published bytes (pinned by sha256 in test_effect_coverage.py) and are read forward, with an absent counter reported as 0 of 0 rather than as a claim that nothing was owed. Full bump checklist applied: schemas regenerated, sample and packet goldens updated (run_id unchanged), doc stamps, adoption-kit renderer source plus prior_render_sha256, and llms-full.txt.

Tests

New tests/test_declaration_questionnaire.py (17 tests), including two exhaustive sweeps: a proposal accounts for every reading it is printed under (all 1–3 reading combinations) and the proposal closes the row it is printed on (those combinations × six structural-evidence variants, ~460 applications, apply-and-re-resolve). Plus: nothing is proposed from a protocol default or from a heuristic read; the numbering never skips or repeats; a repository-controlled claim source cannot forge a line above a value a human pastes; the PR comment reports progress and omits the line when nothing was asked; and an end-to-end walk asserting that confirming exactly what the row published reaches blocked with the risk named.

Full suite green; ruff clean.

Refs #410. Increments 3 (per-source authority + policy packs) and 4 (drift pinning, adoption rungs, environment.target: template, CODEOWNERS check) remain.

🤖 Generated with Claude Code


Review loop

Four passes over the branch after the first commit. Every finding is fixed on this branch; the commits are separate so each is readable on its own.

Pass 1 — one derivation, one permutation (0d63263)

  • The kind -> dimension routing was inverted independently in declarations.py and release_decision.py, and the effect gap-kind set was spelled a third time as _EFFECT_GAP_KINDS. All three now read the one table, pinned by a test that its entries are real gap kinds and that no kind answers two dimensions.
  • _in_question_order tie-broke on gaps.index(gap) — value equality on a pydantic model, so two rows that render identically both map to the first index and the permutation stops being a bijection.
  • _drop_duplicate_blocks folded byte-identical blocks together while keeping only the first one's question keys, so a question could be numbered by the counter and answered by no block.
  • DeclarationQuestion carried readings/proposal that nothing read — a second derivation of what the gap row already publishes.

Pass 2 — the gate that moves with the schema (6c73239)

  • production_safety_requirements() pinned required_report_schema_version="0.36", and _evaluate_receipt compares it for exact equality. On a 0.37 engine every qualification receipt fails with "qualification report schema mismatch" — on every case, for a reason that has nothing to do with safety. No test caught it. Now pinned equal to ReadinessReport's own default by a test, so the next bump cannot leave the gate behind. Swept the rest of src/ and scripts/ for the class; the only other version literals are the explain-finding and scenario minimum supported versions, which are floors and correctly unchanged.

Pass 3 — rendering, found by rendering (d4654fe)

Built a workspace exercising every shape at once (a merged effect+authority block, a source conflict with no blank to fill, a 60-character tool name) and read the output:

  • Blocks were emitted first and comment-only entries after them, so a file came out numbered 2, 3–4, 5–6, 1. Numbering that does not run in order is worse than no numbering; the two renderings are one queue and are now interleaved.
  • A long tool name ran the banner off the line, leaving a ragged heading with trailing whitespace. The banner now elides — safe precisely there, since the block beneath carries the exact tool and tool_id.

Pass 4 — the guard that stopped guarding (c5abf0e)

  • test_no_shipped_template_asserts_on_a_humans_behalf still passed after this branch introduced pre-filled effects — because _shipped_templates built its tools without a semantic assessment, so the proposal path was never reached. The invariant has genuinely changed, so the test now states the new truth (effect/risk_tags may carry a proposal; it must be a closed-vocabulary value and must arrive beside the readings that justify it) and fails if no template exercises that path. Verified by perturbing what it guards, in both directions.
  • Confirmed by inspection that nothing applies a declaration_template: its only consumers are the advisory scaffold writer and a count in the verify prose.

Pass 5 — the prose this branch made false (b04462f)

  • docs/engineering/insufficient-evidence-cold-start.md stated, in the present tense, that "every human-owned value stays <REVIEW_REQUIRED>". It is an engineering-history doc with rounds, so the 2026-07 paragraph keeps its account and gains a pointer, and a Third round section records what changed, why a proposal is safe mechanically rather than editorially, and the two defects that surfacing the proposals exposed.
  • docs/mental-model.md's artifact table promised the file asserts nothing. It now says what a pre-filled effect: is and what still makes it inert.
  • STABILITY.md gained the contract for both new fields (e98cfac).

Pass 6 — a final read of the assembled file (3b38853)

  • A block answering two questions of one dimension rendered Questions 2–3 · effect, effect, which reads as a rendering fault rather than as two questions.

Adversarial pass (the method from #409, applied to the new surface). Tried to cheat the counter: declaring effect: read on tools the scanner reads as external_communication and financial_write leaves the counter at 0 of 2 answered, 2 open and the verdict blocked — a downgrade closes nothing. An acknowledged override does count as answered, which is accurate (a human answered it, on the record) and still cannot reach passed, because it is carried as a review concern.

pengfei-threemoonslab and others added 8 commits August 24, 2026 15:41
…t 2)

Adoption stalls at a wall of blanks. The fourth `adk-samples#1745` walk faced
`0/12` pass-eligible actions and a report that described the work as "24
semantic evidence gaps" — a symptom count with no order and no finish line,
while the same report already held a derived `financial_write` reading for the
one tool that moved money. This turns that surface into a questionnaire.

* **Effects the scan observed are pre-filled.** `suggested-declarations.yaml`
  prints the readings behind each effect question and, where they support one
  conservative answer, offers it in the `effect:` line instead of a blank. A
  proposal is never weaker than any reading, comes from the closed
  `ActionEffect` vocabulary rather than from source content, and is offered
  only where something was observed — a protocol default and a heuristic
  reading of `read` both keep the blank.
* **The file is numbered and counted.** Blocks carry `Question 3 of 5` banners
  ordered by how much answering them can move the verdict, and the header, the
  CLI, and the PR comment all render one shared progress sentence.
* **A question is not a declaration.** The denominator counts only the
  `effect`/`authority` of an `action_surface.actions` row, and `answered` is
  the exact counterfactual: dimensions that gap when the action is re-resolved
  without its declaration.

Also fixes a resolver defect the exhaustive proposal sweep uncovered: the
read/side-effect conflict read the manifest's own `risk_tags` as source
evidence, so the `risk_tags` repair could not close the row it was printed on
whenever the action carried a `readOnlyHint`.

Report schema 0.36 → 0.37, packet 0.13 → 0.14, verifier 0.10 → 0.11; all
additive, all prior versions frozen and read forward.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… review)

Six findings from the review pass on this branch, all of the same two classes
this codebase keeps hitting.

*Second implementations.* The `kind -> dimension` routing was inverted
independently in `declarations.py` and `release_decision.py`, and the effect
gap-kind set was spelled a third time as `_EFFECT_GAP_KINDS`. All three now
read `ANSWERABLE_ISSUE_KINDS` / `DIMENSION_BY_GAP_KIND` from the one module
that defines them, pinned by a test that the table's entries are real gap kinds
and that no kind answers two dimensions. `DeclarationQuestion` also carried
`readings` and `proposal` that nothing read — a second derivation of what the
gap row already publishes — so they are gone.

*Permutations that are not bijections.* `_in_question_order` tie-broke on
`gaps.index(gap)`, which resolves by value equality on a pydantic model: two
rows that render identically both mapped to the first index, duplicating one
and dropping the other. It now sorts `(question, original position)` pairs.
`_drop_duplicate_blocks` folded byte-identical blocks together while keeping
only the first one's question keys, so a question could be numbered by the
counter and answered by no block — it now merges the keys and the readings.

Also: a conflict row now prints the readings behind it. It is the row with no
blank to fill *because* its sources disagree, so what each one says is exactly
what the reviewer has to go and reconcile.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second review pass. `production_safety_requirements()` pinned
`required_report_schema_version="0.36"`, and `_evaluate_receipt` compares it
for **exact equality** — so on a 0.37 engine every qualification receipt fails
with "qualification report schema mismatch: expected 0.36, got '0.37'", on
every case, for a reason that has nothing to do with safety. No test caught it:
`test_production_defaults_pin_the_exact_beta_contract` asserts the strata and
the thresholds but never the schema.

Pinned now by `test_the_qualification_gate_demands_the_schema_the_engine_emits`,
which compares the requirement against `ReadinessReport`'s own default, so the
next bump cannot leave the gate behind. `benchmark/safety-qualification/README.md`
carried the same stale number.

Swept the rest of the tree for the class: the only other version literals in
`src/` and `scripts/` are the `explain-finding` and `scenario` *minimum*
supported versions, which are floors and correctly unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third review pass, both found by rendering a workspace that exercises every
shape at once rather than by reading the code.

*The numbers did not run in order.* Blocks were emitted first and comment-only
entries after them, so an action whose effect question has no blank to fill —
a source conflict — printed a file numbered 2, 3–4, 5–6, 1. Numbering that
does not run in order is worse than no numbering. The two renderings are one
queue and are now interleaved by question number.

*A long tool name ran the banner off the line.* A subject is
repository-controlled and unbounded; one 60-character name left a ragged
heading with a trailing space and no rule. The banner now elides to fit, which
is safe precisely here: it is a heading, and the block directly beneath it
carries the exact `tool` and `tool_id` a reader acts on. Pinned by a test that
also asserts no generated line ends in whitespace.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fourth review pass. `test_no_shipped_template_asserts_on_a_humans_behalf` still
passed after this branch introduced pre-filled effects — for the wrong reason.
`_shipped_templates` built its tools without a semantic assessment, so
`propose_effect_declaration` was never reached and every effect template kept
its sentinel. A guard that stops covering the thing it guards is the "a test
that can skip in CI is not a guard" class again.

The invariant has genuinely changed, so the test now says the new truth and
guards it: every scalar is still a sentinel or a selector, except `effect` and
`risk_tags`, which may carry a proposal — and a proposal must be a value from
the closed `ActionEffect` vocabulary (never source content) and must arrive
beside the `observed_readings` that justify it. A resolved tool is in the
fixture set, and the test fails if no template exercises the proposal path at
all.

Verified by perturbing what it guards: writing an off-vocabulary value into the
template fails on the vocabulary clause, and suppressing the readings fails on
"no shipped template exercised the proposal path" — no readings, no proposal.

Also confirmed by inspection that nothing applies a `declaration_template`: its
only consumers are the advisory scaffold writer and a count in the verify
prose, and `auto_apply=false` / `requires_human_review=true` are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
STABILITY.md is the external contract for what each published field means and
what may change under it. The two additive fields were documented in the agent
contract but not there: `declaration_questions` (what counts as a question,
why `answered` is a counterfactual rather than a declaration count, and that
nothing gates on it) and `observed_readings` with the pre-filled template (that
a proposal comes from the closed vocabulary, is never weaker than a reading, is
offered only where something was observed, and is still applied by nobody).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fifth review pass, over the prose rather than the code.

`docs/engineering/insufficient-evidence-cold-start.md` stated, in the present
tense, that "every human-owned value stays `<REVIEW_REQUIRED>`". That is now
false for exactly one field. It is an engineering-history doc with rounds, so
the 2026-07 paragraph keeps its account and gains a pointer, and a *Third
round* section records what changed, why a proposal is safe mechanically rather
than editorially, and the two defects that surfacing the proposals exposed —
the manifest allowed to contradict itself, and the guard that stopped guarding.

`docs/mental-model.md`'s artifact table promised the file asserts nothing; it
now says what a pre-filled `effect:` is and what still makes it inert.

The rule the third round adds: a progress counter must be measurable on both
halves. Inventory and `agent_bindings` declarations are human answers too and
are deliberately outside the denominator — there is no counterfactual for them,
and counting what can only be measured on one side is how a progress bar starts
lying.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Final read of the assembled file. A block that answers two questions of one
dimension rendered "Questions 2-3 · effect, effect", which reads as a
rendering fault rather than as two questions. Reachable only through the
defensive duplicate-block merge, but the banner is the line a reader trusts to
tell them what a block is for.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@pengfei-threemoonslab pengfei-threemoonslab left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result: changes requested

I reviewed exact head 3b38853fe2705f9d33c6f815ff12f47f0f56f04f against base 0a7443644c14f98c03692d8936b0bdd8bc848ac5. This is posted as a COMMENT review because the authenticated GitHub account is also the PR author, so GitHub does not permit a formal self-request-changes review.

1. [P1] Do not ask a declaration question that a declaration cannot close

src/agents_shipgate/core/declaration_questions.py:80-85 classifies partial_authority_evidence as declaration-answerable, but src/agents_shipgate/core/semantic_assessment.py:1314-1327 unconditionally preserves that issue whenever source_status == "partial", even after a reviewed authority declaration is present.

I reproduced this with a complete MCP tool carrying readOnlyHint: true and auth.scopes: ["docs:read"]. The scan asks one authority question. Applying the exact scoped declaration the scaffold requests (mode: scoped, auth_type: oauth2, the same scope) and rescanning still leaves partial_authority_evidence and the counter at 0 of 1 answered. The advertised finish line is therefore unreachable for this supported input shape.

Either route this issue to source correction instead of the questionnaire, or change the resolver so an appropriate reviewed declaration can actually close it. A round-trip invariant test would help: every kind in ANSWERABLE_ISSUE_KINDS should have at least one generated answer that makes its question answered on rescan.

2. [P2] Preserve a passed v0.13 packet during the v0.14 upgrade

src/agents_shipgate/packet/json_packet.py:88-102 now records v0.13 in legacy_version, and :174-175 consequently calls _upgrade_semantic_coverage_v08 for it. That helper explicitly applies only to v0.7 and older (:261-266). As a result, loading an immediately previous v0.13 packet whose release decision is passed silently rewrites it to insufficient_evidence with a claim that v0.13 predates evidence-backed semantic coverage.

Please separate recognized prior versions from pre-v0.8 versions, or gate this downgrade to v0.1-v0.7. An immediate-prior-version test with a passed decision would catch this compatibility regression.

3. [P2] Keep protocol defaults separate from observed effect evidence

src/agents_shipgate/core/semantic_assessment.py:1039-1055 groups readings only by effect and ORs the observed bit. When the same effect has both a protocol-default claim and an observed or inferred claim, the result is one observed=True row containing both sources.

For example, an unannotated MCP tool plus a writes_data risk hint produces write, sources ("mcp_protocol_default", "risk_hint:keyword"), observed=True. The scaffold then presents the protocol default under the observed-evidence section, although this PR correctly defines protocol defaults as absence of evidence. Group by (effect, observed) or otherwise retain provenance-class separation through rendering.

4. [P2] Do not render non-contiguous question numbers as a range

src/agents_shipgate/core/declaration_questions.py:284-290 sorts equal-display-subject questions by dimension before tool ID. Two canonical tools sharing the same displayed name/provider are therefore numbered tool A effect = 1, tool B effect = 2, tool A authority = 3, tool B authority = 4. The scaffold correctly merges rows per tool, but src/agents_shipgate/cli/scan/declarations.py:453-456 renders those blocks as Questions 1–3 and Questions 2–4, falsely claiming the intervening question owned by the other tool.

Either make each tool questions contiguous or render exact non-contiguous lists such as Questions 1 and 3.

5. [P2] Regenerate the sample JSON goldens instead of only relabeling them

The expected reports for simple_openai_api_agent, simple_langchain_agent, and simple_crewai_agent change only report_schema_version from 0.36 to 0.37. Fresh scans at this PR head emit the new v0.37 members, while those checked-in files contain no declaration_questions or observed_readings members at all. For example, the fresh OpenAI sample reports declaration-question coverage {total: 4, answered: 4, open: 0}; its expected JSON omits the field.

tests/test_reports.py:151-174 only asserts the version string and decision, so it blesses a stale body despite its own message saying to regenerate the golden. Please regenerate these artifacts and add a comparison that detects structural drift, as the Conductor expected report already demonstrates.

Small documentation cleanup: AGENTS.md:749 and the mirrored llms-full.txt:774 still label the Release Evidence Packet as v0.13 after this PR makes v0.14 current.

Validation performed

  • All hosted PR checks are green at the reviewed head.
  • 238 focused questionnaire/scaffold/effect/release/safety tests passed.
  • The full suite had no executed-test failures; 8 packaging tests initially errored together because their isolated wheel fixture could not download hatchling in the restricted environment, then all 8 passed when rerun with dependency access.
  • Focused Ruff, generated-schema check, and git diff --check passed.
  • I ran deterministic repros for findings 1-4 and regenerated the three sample reports in temporary output directories for finding 5.
  • Agents Shipgate reports review_publishable / review_required: PR updates are allowed, while merge remains human-gated because protected agent-instruction surfaces changed.

Addresses all five review findings on #416.

**1 [P1] `partial_authority_evidence` advertised an unreachable finish line.**
The resolver preserves that issue whenever the *source's* authority evidence is
ambiguous or incomplete, whatever the manifest declares — "reviewed authority
cannot replace ambiguous or incomplete source authority alternatives" is a
deliberate safety property, so the fix is not to weaken it. Reproduced exactly
as reported: an MCP tool published with scopes and no auth type asks one
authority question, and writing the exact scoped block the scaffold requests
leaves the counter at `0 of 1 answered`. It is now excluded from
`ANSWERABLE_ISSUE_KINDS` and routed to `provide_source` with no declaration
template and an instruction naming the source shapes that close it.

The same defect one branch deeper, found while writing the round-trip test:
`conflicting_effect_evidence` is raised about either surface, and only the
branch the resolver attributes to `action_surface_declaration` is answerable —
a server publishing both `readOnlyHint: true` and `destructiveHint: true`
contradicts itself and no declaration touches that. Added the invariant test
the review asked for, over every kind and both branches, and verified it by
perturbation: re-adding either kind fails it.

**2 [P2] A recognised prior packet version had its verdict rewritten.** The
downgrade was gated on `legacy_version` — "is this a version I recognise" —
rather than on "is this before v0.8", the set `_upgrade_semantic_coverage_v08`
documents itself as being for. This predates the branch: on `main`, a stored
v0.8-v0.12 `passed` packet already loaded as `insufficient_evidence`, explained
by a claim about history false of it; adding v0.13 to the ladder made it reach
the immediately previous version. Now scoped to v0.1-v0.7, with a test over the
post-v0.8 versions; the existing v0.1-v0.7 downgrade test is untouched.

**3 [P2] Protocol defaults were folded into observed evidence.** `effect_readings`
grouped by effect and OR-ed the `observed` bit, so an unannotated MCP tool with
a `writes_data` hint produced one `observed=True` row carrying
`mcp_protocol_default` among its sources — printed under "what this scan read
this action's effect as", which is exactly what a default is not. Grouped by
`(effect, observed)` now; the proposal reasons over values and is unchanged.

**4 [P2] Non-contiguous numbers rendered as a range.** Ordering put dimension
before tool id, so two canonical tools sharing a display subject interleaved
and the block merged for one owned questions 1 and 3 — announced as
"Questions 1-3", claiming the other tool's. Ordering is tool-major now (the
dimension order still holds within an action), and the banner renders exact
lists (`Questions 1 and 3`) so the renderer cannot lie if ordering ever changes.

**5 [P2] Sample goldens were relabelled, not regenerated.** All four are
regenerated with real scans; `report.md` is byte-identical, so this is purely
the missing v0.37 members. Added a structural-drift guard comparing the set of
field paths in each golden against a fresh scan — values legitimately differ
between runs, a missing member never does. Verified it catches exactly the
reported case.

Plus the documentation cleanup: the Release Evidence Packet heading in
`AGENTS.md` (mirrored into `llms-full.txt`), and a stale mixed version list in
`docs/agent-contract-current.md` — two of its five numbers were already wrong —
replaced by a pointer to the one table that carries them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

All five findings reproduced and fixed at 1107e670. Two of them were bigger than reported, and one predates this branch.

1 [P1] partial_authority_evidence — fixed, and one branch deeper

Reproduced exactly: an MCP tool with readOnlyHint: true and auth.scopes: ["docs:read"] asks one authority question, and applying the exact scoped block the scaffold requests leaves 0 of 1 answered.

I took your first route rather than the second. _source_authority returns partial from the source's own shape, and the resolver's preservation of it — "reviewed authority cannot replace ambiguous or incomplete source authority alternatives" — is a deliberate safety property, so relaxing the resolver would be the wrong trade. It is now out of ANSWERABLE_ISSUE_KINDS and routed to provide_source with no declaration template, since publishing one was the deeper version of the same fault: a next step that cannot change the answer.

Writing your suggested round-trip invariant found the same defect one branch down. conflicting_effect_evidence is raised about either surface — semantic_assessment.py:586 blames tool_source when the source asserts read-only and a side effect at once, and :601 blames action_surface_declaration when the declaration is weaker than policy-eligible evidence. Only the second is answerable; a server publishing both readOnlyHint: true and destructiveHint: true contradicts itself and no declaration touches it. DECLARATION_ATTRIBUTED_KINDS now defers to the resolver's own attribution.

test_every_answerable_kind_has_an_answer_that_closes_it walks every kind with a configuration that raises it and a declaration that should close it. Verified by perturbation, in both directions: re-adding partial_authority_evidence with an honest case fails with "applying {…} left the authority question open — the counter advertises a finish line this answer cannot reach", and emptying DECLARATION_ATTRIBUTED_KINDS fails the self-contradicting-source test.

2 [P2] Packet upgrade — correct, and it predates this branch

Confirmed against 0a744364: v0.7 → insufficient_evidence, v0.11 → insufficient_evidence, v0.12 → insufficient_evidence. The gate was on legacy_version ("is this a version I recognise") rather than on "is this before v0.8", so every packet-schema bump has been quietly adding the immediately previous version to the rewritten set. Adding 0.13 made it reach the version people are most likely to have stored, which is how you caught it.

Now split into _PRE_SEMANTIC_COVERAGE_VERSIONS (0.10.7, frozen — it can only ever be a claim about history) and the ladder, which stays the sole authority on what is readable. v0.7 still downgrades; 0.80.14 keep their verdict. The existing 0.10.7 parametrisation was already encoding the right intent and is untouched; the new test covers 0.8, 0.11, 0.12, 0.13.

3 [P2] Readings grouped by (effect, observed)

Reproduced: EffectReading(effect='write', sources=('mcp_protocol_default', 'risk_hint:keyword'), observed=True). Now two rows, so the default renders under "Assumed in the absence of evidence, and never proposed from" where it belongs. The proposal is unaffected — it reasons over values, and the observation is still what unlocks it.

4 [P2] Both halves

Ordering is tool-major now ((-rank, subject, tool_id, dimension)), which makes each action's questions contiguous by construction; the dimension order still holds within an action, which is all it was ever there for. And the banner renders exact lists — Question 3, Questions 1–2, Questions 1 and 3 — so a range is only written when the numbers really are contiguous. A renderer that spans any set is one ordering change away from claiming another action's question.

5 [P2] Goldens regenerated, with a guard

All four regenerated with real scans. report.md came out byte-identical, so the diff is purely the missing v0.37 members.

For the guard I compared structure rather than values: the set of field paths a fresh scan emits must equal the golden's, with list indices collapsed. Values legitimately differ between runs (paths, hashes) and are already covered by the markdown/packet byte comparisons and the Conductor field-for-field check; a missing member never legitimately differs. Verified by deleting declaration_questions from the crewai golden — it names the six absent paths and says to regenerate rather than relabel.

Documentation

AGENTS.md heading fixed (and regenerated into llms-full.txt). While there: docs/agent-contract-current.md:174 carried a mixed version list of which two of five numbers were already wrong (verifier v0.6, handoff v6). Half-updating it would have left it just as misleading, so the snapshot is replaced by a pointer to the single table that carries current versions.


Full suite green, ruff clean, generate_schemas.py --check clean. No schema version moves for any of this — all five are behaviour or content fixes within v0.37.

@pengfei-threemoonslab pengfei-threemoonslab left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review result: changes still requested

I reviewed exact head 1107e670e523aa52474b2ec2dc29e2a100f2d8b1 against the previously reviewed head 3b38853fe2705f9d33c6f815ff12f47f0f56f04f and base 0a7443644c14f98c03692d8936b0bdd8bc848ac5. This is posted as a COMMENT review because the authenticated GitHub account is also the PR author, so GitHub does not permit a formal self-request-changes review.

The v0.13 packet upgrade, observed/default reading split, tool-major and exact-number rendering, regenerated report goldens, and primary v0.14 documentation are now fixed. The partial-authority row is also correctly removed from the questionnaire counter. Three executable-remediation defects remain, plus one documentation stamp.

1. [P1] Point provide_source at the source, not at an action declaration

src/agents_shipgate/ci/release_decision.py:1614-1617,1663-1677

partial_authority_evidence now correctly emits kind: provide_source, but _semantic_gap_path() has no source-owned case and falls through to shipgate.yaml#action_surface.actions[tool="lookup"].

I reproduced this with a complete MCP tool that publishes auth.scopes: ["docs:read"] but no auth type. The resulting action has source_ref: agent.py, says “A reviewed action declaration cannot close this row,” and simultaneously sets its machine-readable path to the manifest action row. The release-decision reason also renders “Fix at shipgate.yaml…”.

This path is the addressable target consumed by short forms and coding agents, so the contradiction is operational rather than cosmetic. For this kind, return the actual source artifact/reference, or leave path unset when no exact source path exists. Please add an end-to-end assertion for the fully rendered action, not only its kind and lack of template.

2. [P1] Preserve issue provenance when building conflict repair actions

src/agents_shipgate/ci/release_decision.py:1068-1073,1582-1599

The questionnaire filter correctly excludes source-owned conflicting_effect_evidence, but _semantic_coverage() drops issue.source when it calls _semantic_gap(). The generic conflict branch therefore still publishes all effect values, targets the manifest action row, and says that a conservative reviewed action declaration may resolve the issue.

For an MCP tool with both readOnlyHint: true and destructiveHint: true, the issue is explicitly attributed to tool_source. Adding the exact conservative effect: destructive declaration leaves the identical conflict on rescan. Pass provenance into the action builder and route a source-owned conflict exclusively to correcting the source annotations. The declaration-owned branch can retain the manifest route. The added test only checks that the conflict is not counted as a question, so it misses this non-closing published action.

3. [P2] Treat reviewed risk_overrides as declaration evidence

src/agents_shipgate/core/semantic_assessment.py:736-768 and src/agents_shipgate/core/risk_hints.py:457-465

The new source-conflict exclusion misses the other manifest-owned positive-risk surface. risk_overrides.tags becomes a high-confidence hint with source: manual and basis: reviewed_declaration, then appears as the claim source risk_hint:manual, which is not in DECLARATION_CLAIM_SOURCES.

I reproduced readOnlyHint: true plus a reviewed risk_overrides tag of code_execution. It is mislabeled as conflicting_effect_evidence(source=tool_source); adding matching effect: code_execution and action risk_tags: [code_execution] still leaves the same conflict. This contradicts the helper’s documented invariant that source annotations cannot block a reviewed over-declaration. Exclude claims whose basis is reviewed_declaration, or explicitly include the manual override claim source. The new test covers only action_surface.actions[].risk_tags, not this sibling manifest path.

4. [P3] Update the report-schema version in the RC note

docs/report-v1-consolidation-rc.md:5

This PR makes report_schema_version: "0.37" current, but the RC note still says the current runtime is 0.36. Update the stamp to 0.37, or make the sentence explicitly historical.

Validation performed

  • All hosted PR checks are green at the reviewed head.
  • 225 focused questionnaire, semantic, scaffold, and effect tests passed; all 57 release-decision tests passed.
  • Genuine historical v0.8-v0.13 packet probes validate and preserve their original decisions; all four regenerated report goldens match fresh normalized scans.
  • The full suite had no executed-test failures. Eight packaging tests initially errored together because the isolated wheel fixture could not download hatchling in the restricted environment; all eight passed when rerun with dependency access.
  • Focused Ruff, generated-schema validation, and git diff --check passed.
  • Agents Shipgate reports review_publishable / review_required: updating the PR is allowed, while merge remains human-gated because protected agent-instruction and release-trust surfaces changed.

Three executable-remediation defects and a stamp, all the same shape as the
first round: a published repair that cannot close the row it is printed on.

**1 [P1] The path contradicted the sentence above it.** `partial_authority_evidence`
emitted `provide_source` and said "a reviewed action declaration cannot close
this row" while `_semantic_gap_path` fell through to
`shipgate.yaml#action_surface.actions[...]` — the target coding agents and the
`Fix at …` line actually consume. It now resolves the tool's own published
evidence (`tools.json#/tools/0`), or `None` when nothing openable is known; the
row stays addressable through its rerun command either way.

**2 [P1] Provenance was dropped before the action was built.** `_semantic_coverage`
passed `issue.source` as a display `source_ref` but not to the action builder,
so a `conflicting_effect_evidence` the resolver blames on `tool_source` still
published every effect value, the manifest route, and "add a conservative
reviewed action declaration" — and adding `effect: destructive` leaves the
identical row. `issue_source` is threaded through, and the source-owned branch
routes to the source with no template and no effect vocabulary. The
declaration-owned branch is unchanged.

One predicate now decides both: `is_declaration_answerable(kind, source)`.
Counting a row the published repair cannot close, and publishing a declaration
for a row the counter knows is unanswerable, are the same defect from two ends,
so they may not be judged by two spellings.

**3 [P2] The other manifest-owned positive-risk surface.** The source-conflict
exclusion covered `action_surface.actions` but not `risk_overrides.tags`, which
reaches the effect dimension as `risk_hint:manual` with basis
`reviewed_declaration`. A reviewed `code_execution` tag on a tool published
with `readOnlyHint: true` was reported as the source contradicting itself, and
declaring the matching effect and risk tag could not clear it. Manifest
ownership is decided by both routes now — and `_validated_hint_basis` grants
`reviewed_declaration` to no source other than `manual`, so tool-published
content cannot reach it.

**4 [P3]** `docs/report-v1-consolidation-rc.md` runtime stamp.

Tests assert the fully rendered action — kind, `path`, `accepted_values`,
template, and the projected reason — not just the kind, plus a round trip
showing the conservative declaration really does leave the conflict standing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

All four addressed at 63c7593e. The two P1s share one root cause, so they are fixed with one predicate rather than two branches.

1 + 2 [P1] One predicate for "can a declaration close this?"

Both reproduced. partial_authority_evidence said "a reviewed action declaration cannot close this row" while _semantic_gap_path fell through to shipgate.yaml#action_surface.actions[...]; and _semantic_coverage passed issue.source only as a display source_ref, so the source-owned conflicting_effect_evidence still published every effect value and the manifest route.

These are the same defect seen from two ends — counting a row the published repair cannot close, and publishing a declaration for a row the counter already knows is unanswerable. Judging them by two spellings is how they drift apart again, so is_declaration_answerable(kind, source) is now the single predicate, read by the questionnaire filter and by the action builder. issue_source is threaded into _semantic_gap, and None is treated as source-owned — the conservative default, which never promises a declaration that may not work.

What the rows publish now:

partial_authority_evidence    provide_source  path: tools.json#/tools/0  template: None
conflicting_effect_evidence   provide_source  path: tools.json#/tools/0  template: None
  accepted_values: ['single_effect_annotation', 'consistent_permission_class']
  expects: "Correct this tool's published annotations at the source so they agree —
            a tool is read-only or it has a side effect, not both — ... A reviewed
            action declaration cannot close this row, whatever effect it names."

path resolves the tool's own published evidence, or is left unset when nothing openable is known — verified that the row stays addressable through its rerun command in that case, so nothing loses its affordance.

As asked, the tests assert the fully rendered action — kind, path, accepted_values, declaration_template, the expects text, and that the projected release-decision reason no longer says Fix at shipgate.yaml#action_surface. test_a_self_contradicting_source_is_not_sent_to_the_manifest also applies the conservative effect: destructive and asserts the identical conflict is still there, so the reason the row must not suggest it is pinned rather than asserted in prose.

3 [P2] risk_overrides is the manifest speaking

Reproduced: a reviewed code_execution tag on a tool published with readOnlyHint: true came back as conflicting_effect_evidence(source=tool_source), and declaring the matching effect and risk_tags left it standing.

I took the basis route rather than adding the source name, because it is the semantically load-bearing one: _validated_hint_basis grants reviewed_declaration only when hint.source == "manual", and risk_overrides.tags is the only producer of source="manual" — so in this dimension that basis means "a human wrote this into the manifest" and tool-published content cannot claim it. Manifest ownership is now source in DECLARATION_CLAIM_SOURCES or basis == "reviewed_declaration"; both are needed, since action_scope claims carry a structural_scope basis and so are invisible to the second test.

The tool now resolves cleanly: no conflict, and 2 of 2 answered.

4 [P3] Stamp updated

docs/report-v1-consolidation-rc.md:50.37. Swept the rest of the tree for stale current-schema prose; nothing else outside the deliberately historical mentions.


Full suite green, ruff clean, generate_schemas.py --check clean. No sample golden moved — the structural-drift guard and the Conductor field-for-field check both pass unchanged, which is the evidence that this routing change did not touch any shipped sample's rows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant