fix(verify): distinguish an unreadable diff from trigger no_match

[pengfei-threemoonslab]

Summary

• An unreadable PR diff is no longer reported as "nothing here is agent-related." verify --preview collapsed every diff-acquisition failure into one message, then evaluated the trigger catalog against the empty inputs that failure left behind — publishing skip_reason: "no_match" with the rationale "nothing in this PR signals a tool-surface change" about a PR it had never read. Fixes bug(verify-preview): distinguish unreadable diff from trigger no_match #308.
• Diff acquisition is classified rather than flattened. not_attempted, refs_missing, merge_base_missing, unrelated_histories, objects_missing, metadata_limit_exceeded, body_limit_exceeded, git_timeout, git_failed — read off Git's own diagnostic (stderr was DEVNULL, which is why every failure looked identical; it is now piped and drained on its own thread under an 8 KiB cap). They travel on a new required verifier.json diff_status block with a bounded, path-redacted excerpt, the precise repair, and fetch_repairable.
• Git reports two opposite failures identically as "no merge base", and they are separated: merge_base_missing is a shallow checkout that truncated a merge base which does exist (deepen — fetch_repairable: true), while unrelated_histories is two roots with no common ancestor, which no fetch can ever create (route to a human). Sending the second to fetch_base looped an agent forever.
• Metadata and body are collected separately, so a body that cannot be read no longer discards the changed paths that were read. A blobless clone answers --name-status in full, and those paths are exactly what says a PR touches an agent surface. The untracked-path inventory moves ahead of the body read for the same reason — a brand-new capability file appears in no git diff at all.
• The trigger evaluator gained the state it was missing (issue comment 3: fixing this at the call sites would not hold). input_status and evaluation_status, with should_run / run_shipgate / skip / skip_reason null when the withheld verdict would have been a skip — see the asymmetry below.
• An unreadable diff outranks every adoption route in preview, manifest or not (issue comment 1).

The rule behind the asymmetry

Rule matching is monotone in path and diff evidence: adding evidence can only add matches. So a skip verdict reached from incomplete input is unsound — the missing bytes are exactly what would have flipped it — and is withheld (evaluation_status: not_evaluated, verdict fields null). A run verdict is sound and is still published (evaluation_status: evaluated, should_run: true). Consumers must branch on evaluation_status, not on should_run alone, and read matched_rules before attributing a run to anything the diff showed: in an adopted repository TRIGGER-EXISTING-MANIFEST-PRESENT force-runs on the manifest alone, with changed_files: [].

Before / after, reproduced on real repositories

input shape	before	after
shallow clone, no reachable merge base	skip_reason: "no_match"; note said "exceeded static output bounds"	merge_base_missing, fetch_repairable: true, next_action: fetch_base, remediation deepens — and a test proves git fetch --deepen then reads the diff clean
unrelated roots, non-shallow	same, and would have routed to an endless fetch_base	unrelated_histories, fetch_repairable: false, routed to a human to correct the base
blobless partial clone, GIT_NO_LAZY_FETCH=1	same message, and src/agent.py discarded	partial / objects_missing, changed_files keeps src/agent.py, remediation names git fetch --refetch
body over _DIFF_BODY_LIMIT	same message again, paths thrown away	partial / body_limit_exceeded, paths preserved
no shipgate.yaml (cold start)	"Shipgate is not configured in this workspace" → initialize	reports the diff failure and routes to the repair

Google ADK agent.py is matched by the diff_contains: "FunctionTool(" token, not by any path glob — losing the diff body genuinely loses the match, which is why a body-only failure reports not_evaluated rather than no_match.

Type

• CLI or GitHub Action behavior
• Report, schema, or SARIF output

Verification

CI is authoritative for python -m ruff check ., python -m compileall -q src tests, and python -m pytest.

Additional local checks run:

• Full pytest -q suite green (0 failures) and ruff check . clean.
• Every issue repro shape and every review finding built as a real Git repository and driven end-to-end through the CLI, before and after — shallow clone, unrelated orphan roots, blobless partial clone, over-limit body, adopted-repo force-run, and the mixed committed-fetchable / worktree-deterministic failure.
• Committed-tree self-dogfood: verify --config shipgate-self.yaml --base main --head HEAD --ci-mode advisory → merge_verdict: human_review_required (this PR edits trust roots, so a human reviews it; the self workflow fails only on blocked,unknown).
• Schemas regenerated with scripts/generate_schemas.py; llms-full.txt regenerated with scripts/build-llms-full.py.

Release-readiness notes

• No user-code import added to default scan paths
• No network access added to default scan paths
• New or changed check IDs are documented in docs/checks.md — n/a, no check IDs added or changed
• Report/schema changes are additive or documented in STABILITY.md

Schema versions

Trigger catalog 0.2 → 0.3 and verifier 0.6 → 0.7 (v0.6 stays a frozen, readable reference; pre-v0.7 artifacts normalize to diff_status.completeness: "unknown"). contract_version deliberately stays at 19, and report_schema_version and every other counter are unchanged: STABILITY.md decouples schema counters from the contract counter, and contract bumps are tied to CLI-surface breaks. Both bumps have a migration note in STABILITY.md.

Reviewer notes

• diff_status is top-level, not inside trigger. Burying it in the trigger block would repeat the original mistake — making a diff-acquisition fact look like a trigger opinion. That is what forced the verifier bump. It is required on current artifacts, so a payload that omits it cannot pass as one that read its diff cleanly.
• The status, the repair, and the headline are all derived from one selected failure. When two collectors fail, _worst_diff_failure breaks ties toward the failure a fetch cannot repair, so fetch_repairable: false can never appear beside an authorized fetch_base, and the headline can never say "human review required" over an agent_action_required route.
• Existing tests that asserted the old behaviour were changed, because the behaviour was the bug: test_verify_preview_missing_base_without_manifest_recommends_init (now expects fetch_base, renamed) and test_scenario_docs_only_no_shipgate_fails_closed (should_run was False, now None).
• Nullable verdict fields are the one thing a consumer must adapt to. Treating null as falsy stays safe — it routes to "do not claim this PR is irrelevant" — but reporting it as "skip" is not. The only public schema that already types should_run (docs/feedback-schema.v0.1.json) declares it nullable, so nothing contradicts.
• shipgate check is untouched. It errors loudly on an unreadable diff rather than misleading, so it is outside this issue — but now that the paths survive it could return a partial BoundaryChangeSet instead. Worth a follow-up.
• Overlap with Fail closed when verify cannot resolve a comparison base #305. That PR adds fetch_required / selection_required to run_verify using the same evaluate(paths=[], diff_text="", …) pattern. On this base that call needs input_status=INPUT_UNAVAILABLE, or its new fail-closed states ship with a contradictory trigger block — which is the sequencing the issue asked for.
• tests/golden/codex_boundary_result/*.json were regenerated with CLAUDECODE unset (the root conftest scrubs it, so regenerating from an agent shell would write agent: "claude-code" into the goldens).

🤖 Generated with Claude Code

[pengfei-threemoonslab]

Engineering review outcome: changes requested before merge.

The fail-closed design is strong, but four contract inconsistencies remain: unrelated histories are routed to an unrecoverable fetch loop; worktree partial evidence is still discarded; preview can contradict its own published trigger verdict; and the agent-facing contract omits the intentional partial-run exception. See the inline findings for concrete reproductions and fixes.

Verification performed: targeted verifier/trigger/schema tests and Ruff passed; GitHub CI, Agents Shipgate, and self-dogfood are green. The full local suite had no assertion failures; eight wheel-packaging setup cases could not install hatchling in the network-restricted sandbox. The exact-head Shipgate receipt validated successfully and reported human_review_required, release decision review_required, 0 blockers, and 18 trust-root review items. Static analysis only.

[pengfei-threemoonslab]

[P2] Distinguish shallow history from genuinely unrelated histories

merge_base_missing is not always fetch-repairable. The new _unrelated_histories fixture creates two orphan roots in a non-shallow repository; no git fetch --deepen or git fetch --unshallow can create a common ancestor, so this routes the agent to fetch_base forever. Please make repairability conditional on the repository's shallow/object state (or split the reason) and route a non-shallow missing merge base to correcting the base or human review.

[pengfei-threemoonslab]

Fixed in 09c31d1 — split the reason rather than making one reason conditionally repairable.

You are right that the fixture proves the bug: two orphan roots in a non-shallow repo can never gain a common ancestor, so fetch_base was an infinite loop. git rev-parse --is-shallow-repository discriminates the two causes exactly, and I verified both directions on real repositories before changing anything — a --depth 1 clone with a --depth 1 base fetch reports no merge base and is-shallow-repository: true, and git fetch --deepen genuinely repairs it; the orphan-root repo reports the identical Git error with is-shallow-repository: false.

• merge_base_missing now means shallow checkout truncated a merge base that does exist → fetch_repairable: true, remediation deepens.
• New unrelated_histories means no common ancestor exists → fetch_repairable: false, routed to review with "confirm the base names the right comparison point — a force-push or a rewritten branch produces this".
• A failed shallow probe returns git_failed, not either of the above: it is the one case with no basis to claim the histories are unrelated, so it synthesizes neither repair.

Chose splitting over a conditional flag so the token itself is the machine-readable answer. Regression tests: test_shallow_history_reports_a_repairable_missing_merge_base, test_unrelated_histories_are_never_routed_to_another_fetch, test_unrelated_histories_route_a_verify_run_to_a_human, and test_deepening_a_shallow_clone_actually_repairs_the_diff — the last one actually runs git fetch --deepen and asserts the diff then reads complete, so the remediation is pinned as the one that works rather than the one that reads well.

[pengfei-threemoonslab]

[P2] Merge typed worktree evidence before failing

When working_tree_context() raises DiffInputError, worktree_failure already contains the authoritative changed_files, but this block only updates diff_input; it never merges those paths (or any available text) into the verifier accumulators. I reproduced diff_status: partial with changed_files: [] while base_notes said the paths were collected, and a changed tools/new_mcp.json therefore lost its path-rule match. Please merge the partial context before building the artifact.

[pengfei-threemoonslab]

Fixed in 09c31d1 — and the reproduction found a second hole next to it.

The handler now merges worktree_failure.changed_files and .diff_text into the accumulators before evaluate() and _build_verifier(), so base_notes and changed_files can no longer disagree.

While writing the regression test I hit the related gap: an untracked tools/new_mcp.json was still missing after the merge, because working_tree_context collected the untracked inventory after the body read. A brand-new file appears in no git diff at all, so a body failure dropped it entirely. ls-files --others is cheap path metadata and independent of the body, so it now runs immediately after --name-status. Same reasoning as splitting metadata from body in the committed-ref collector — every cheap, independent read happens before the expensive one that can fail.

I also attached paths and text to the DiffInputError raised when the binary-hiding guard cannot run inside working_tree_context, which previously propagated bare.

Regression test test_verify_merges_partial_worktree_paths_into_the_change_set drives the real CLI with a shrunk body bound and asserts both the note and changed_files name the path, and that TRIGGER-MCP-EXPORT-CHANGED fires. It deliberately omits --head, since archive_head=head is not None is what selects the worktree collector — my first draft passed --head and silently exercised nothing.

[pengfei-threemoonslab]

[P2] Keep the headline consistent with the published trigger verdict

An incomplete diff can still yield a sound run verdict: with partial evidence containing tools/new_mcp.json, the evaluator intentionally returns evaluation_status: "evaluated" and should_run: true. This branch nevertheless publishes “no relevance verdict was reached” as both headline and control.reason. Please branch on trigger.evaluation_status; when a run match exists, say relevance was established but the complete diff must still be recovered.

[pengfei-threemoonslab]

Fixed in 09c31d1. The branch now reads the published trigger result instead of assuming the diff failure implied no verdict:

• run_shipgate true → "…the paths it did read already show an agent-capability surface, so relevance is established; recover the full diff before trusting any merge verdict."
• otherwise → the previous "no relevance verdict was reached".

next_action stays fetch_base/review: relevance being established does not make the merge verdict knowable, so recovering the diff is still the right first step. merge_verdict stays unknown and can_merge_without_human stays false.

Verified on your exact scenario — a blobless clone whose partial paths include tools/new_mcp.json — where evaluation_status: evaluated, should_run: true, TRIGGER-MCP-EXPORT-CHANGED matched. Covered by test_partial_evidence_that_proves_relevance_keeps_its_run_verdict, which asserts on both headline and control.reason, since they were both wrong and are derived separately.

[pengfei-threemoonslab]

[P2] Document the intentional partial-run exception

This says every incomplete diff implies not_evaluated/null, but the evaluator deliberately preserves evaluation_status: "evaluated" and should_run: true when partial path or manifest evidence already proves Shipgate should run. STABILITY.md later documents that exception. Please align this canonical skill, AGENTS.md, the generated skill mirrors, and llms-full.txt so consumers do not override or misreport a valid run verdict.

[pengfei-threemoonslab]

Fixed in 09c31d1 across every teaching surface, not just this one — the same overreaching sentence was in AGENTS.md, all four decide-shipgate-relevance.md copies, all three SKILL.md copies, and llms-full.txt.

They now state the monotonicity rule that STABILITY.md already carried: an input_status other than complete alongside evaluation_status: evaluated is not a contradiction — the paths that were read already proved Shipgate should run — so honor should_run: true rather than overriding it, and still say the diff needs recovering. AGENTS.md adds the explicit instruction to branch on evaluation_status, not on should_run alone. docs/agent-contract-current.md gains the same note, and the new unrelated_histories token is in every reason list.

Byte-identity across the copies is preserved; EXPECTED_CLAUDE_CODE_SKILL_RENDER_SHA256 is updated for both SKILL.md and the prompt. Neither outgoing hash is appended to prior_render_sha256, since both were introduced earlier on this same unmerged branch and never shipped.

[pengfei-threemoonslab]

Engineering re-review of current head 09c31d15. The earlier unrelated-history routing and lost worktree-path findings are fixed, but four P2 contract inconsistencies remain, detailed inline below.

The PR description also needs refreshing: its reason list still omits unrelated_histories; one summary bullet still says every incomplete input nulls all verdict fields; and the before/after table still combines shallow and unrelated histories even though the implementation now deliberately separates them.

Verification performed: all three current GitHub workflows are green; targeted verifier/trigger/contract tests and Ruff passed; the exact-head verification receipt reproduced as valid. Self-dogfood remains correctly human-routed (release_decision: review_required, 0 blockers, 18 review items) because the PR edits trust roots.

[pengfei-threemoonslab]

[P2] Enforce diff_status on current v0.7 artifacts

The comment says this field is always emitted by v0.7, but both this model and _build_verifier default it to None. Removing diff_status from a current verifier_schema_version: "0.7" payload still passes VerifierArtifact.model_validate, and the generated v0.7 schema does not require it. That leaves the core input-health contract unenforced and makes a malformed current artifact indistinguishable from the legacy-unknown case. Please require a non-null status for current v0.7 artifacts and confine the nullable legacy representation to the compatibility-normalization path.

[pengfei-threemoonslab]

Fixed in bfe7c3e — you are right that a documented invariant enforced nowhere is not an invariant.

diff_status is now a required, non-null field on VerifierArtifact, so it is in the generated v0.7 schema's required set and model_validate rejects a current payload without it. _build_verifier also drops its default, so a new call site has to decide rather than inherit complete.

The nullable representation is confined to the compatibility path exactly as you asked, but it needed somewhere honest to land: a pre-v0.7 artifact recorded nothing, and normalizing that to complete would manufacture the one claim the field exists to stop. So DiffCompleteness gains a fourth value, unknown, produced only by legacy normalization (VerifierDiffStatus.unknown()). It is not complete, so it still withholds permission to read a negative trigger verdict — the fail-closed property is preserved, and it matches the BoundaryChangeSet.completeness vocabulary check already uses.

While making it required I also closed a smaller hole beside it: fetch_repairable was free to disagree with reason. A model validator now rejects fetch_repairable: true for any reason outside {refs_missing, merge_base_missing, objects_missing}, so an artifact cannot advertise a fetch as the repair for a failure a fetch cannot touch — the structural half of finding 3738940153.

Cost: 66 tests constructed VerifierArtifact without the field. All updated rather than papered over with a default. Covered by test_a_current_artifact_cannot_omit_its_input_health (asserts the current payload is rejected and that the same payload at 0.6 normalizes to unknown) and test_fetch_repairable_cannot_be_claimed_for_a_deterministic_failure.

[pengfei-threemoonslab]

[P2] Derive the repair action from the selected failure

When the committed diff first fails with a fetch-repairable partial result (for example objects_missing) and the worktree then fails with a stronger non-fetchable result (unavailable / git_failed), line 515 selects the worktree failure for diff_status, but this or preserves the earlier fetch_base action. I reproduced an emitted artifact with diff_status.reason: git_failed, fetch_repairable: false, and control.next_action.kind: fetch_base. Please consolidate the failures before deriving the action, or give the non-fetchable failure precedence, so the published status and authorized repair cannot disagree or loop.

[pengfei-threemoonslab]

Fixed in bfe7c3e. You identified the real defect: the action was derived incrementally while the status was derived by consolidation, so the two could not stay in agreement.

Both now come from one selection. Failures accumulate as (context, expects) pairs, and _worst_diff_failure picks one — ordered first by completeness, then breaking ties toward the failure a fetch cannot repair. Authorizing another fetch against a deterministic failure is precisely the loop the ordering exists to prevent, so when the two halves are equally incomplete the non-fetchable one wins. _diff_failure_action and the headline are then derived from that single selected failure, so diff_status and the authorized repair are structurally incapable of disagreeing.

This also removed the last incremental branch: the ref_missing case previously left the action to _derive_verifier_control, so routing is now uniform across all three collectors.

Belt and braces on top, since this class of contradiction had already occurred twice: VerifierDiffStatus now rejects fetch_repairable: true for a non-fetchable reason at construction, so the disagreement cannot be expressed even by a future call site that bypasses this consolidation.

Reproduced your exact scenario as test_the_worst_failure_decides_both_status_and_repair — committed diff fails refs_missing (fetchable), worktree then fails deterministically — and asserted reason: git_failed, fetch_repairable: false, next_action.kind != "fetch_base".

[pengfei-threemoonslab]

[P2] Do not attribute every run verdict to diff paths

run_shipgate can be true without any path evidence: in an adopted repository, TRIGGER-EXISTING-MANIFEST-PRESENT force-runs independently of the unreadable diff. With a missing base I reproduced changed_files: [], force_run: true, followed by this claim that the paths already show a capability surface. The skill mirrors repeat the same explanation. Please distinguish the force-run rule (or say only that available independent evidence established Shipgate must run) instead of asserting paths were read.

[pengfei-threemoonslab]

Fixed in bfe7c3e. Correct, and it is a sharper point than it first looks: the previous round's fix made the headline agree with should_run but then over-explained why, attributing a manifest-driven verdict to paths that were never read.

The branch now separates the two kinds of evidence via _matched_diff_evidence(), which asks whether any matched rule has an action other than force_run:

• a rule matched on the change set → "the change it did read already shows an agent-capability surface"
• force_run only → "this workspace is already configured for Shipgate, so verification must run regardless"

Verified on an adopted repo with a missing base: changed_files: [], force_run: true, and the headline now says "already configured for Shipgate" rather than claiming a surface was seen.

The skill mirrors repeated the same explanation, so all of them changed too — the three SKILL.md copies, all four decide-shipgate-relevance.md copies, AGENTS.md, STABILITY.md, and docs/agent-contract-current.md now tell consumers to read matched_rules before attributing a run verdict, and say plainly that a force_run match rests on the manifest rather than on anything the diff showed. Covered by test_a_force_run_verdict_is_not_attributed_to_unread_paths.

[pengfei-threemoonslab]

[P2] Keep the failed-verification headline aligned with the control route

This branch now carries a typed diff failure and repair action, but it does not supply a failure-specific headline, so _verifier_headline() maps every failed/unknown scan to "human review required". For a missing base ref the same artifact has control.state: agent_action_required, human_review.required: false, and next_action.kind: fetch_base, while both headline and control.reason say human review is required. Please derive the failure headline/reason from diff_status and the selected control action so the operational contract does not contradict itself.

[pengfei-threemoonslab]

Fixed in bfe7c3e. _verifier_headline() collapses every failed/unknown scan to "human review required", which is right for a failed scan and wrong for a failed input that an agent can repair itself — and control.reason is derived from the same string, so both surfaces contradicted the route.

The diff_unavailable branch now passes a headline_override from the new _diff_failure_headline(), derived from the same selected failure that produced the control action:

• fetch_repairable → "…the history it needs is not available locally yet, so no verdict was reached. Make it available, then rerun verify." (pairs with agent_action_required / fetch_base)
• otherwise → "…fetching cannot repair this, so no verdict was reached and a human must resolve the input." (pairs with human_review_required)

Verified on your missing-base-ref case: control.state: agent_action_required, next_action.kind: fetch_base, human_review.required: false, and neither headline nor control.reason now contains "human review required". test_the_failure_headline_matches_the_control_route asserts exactly that pairing on both surfaces.

The PR description is also refreshed as you asked: unrelated_histories is in the reason list, the summary bullet now states the run/skip asymmetry instead of claiming every incomplete input nulls all verdict fields, and the before/after table has separate rows for shallow versus unrelated histories.

[pengfei-threemoonslab]

Post-merge engineering re-review of the merged head bfe7c3e3. The four findings from the previous pass are fixed and the PR description is now accurate, but two P2 verifier-contract validation gaps remain, detailed inline below. They are not produced by the normal emitter path, but both the current Python reader and the published v0.7 wire schema still accept malformed or contradictory diff_status states, so follow-up fixes are recommended.

Verification performed: GitHub CI, Agents Shipgate, and self-dogfood are green; targeted verifier/trigger/contract tests and Ruff passed; the full suite reached 100% with no assertion failures (eight packaging setup cases could not download hatchling>=1.31.0 in the network-restricted sandbox). The exact-head verification receipt reproduced as valid. Self-dogfood correctly reported human_review_required, release decision review_required, 0 blockers, and 18 trust-root review items. Static analysis only.

[pengfei-threemoonslab]

[P2] Require an explicit current-v0.7 completeness value

Requiring the top-level diff_status block does not yet require any input-health claim inside it: completeness still defaults to complete, and the public literal includes the compatibility-only unknown value. Starting from an emitted current-v0.7 payload, I replaced the block with {} and both VerifierArtifact.model_validate and docs/verifier-schema.v0.7.json accepted it; Pydantic silently materialized completeness: complete. Replacing it with {"completeness":"unknown"} also passed both validators even though the comment above says unknown is reachable only through legacy normalization. Please require completeness on the current wire model/schema and reject unknown when the original artifact version is v0.7, keeping that state confined to the pre-v0.7 compatibility reader.

[pengfei-threemoonslab]

[P2] Enforce the diff-status relationships in the public contract

This condition skips repairability validation when completeness == "complete", so VerifierDiffStatus(completeness="complete", fetch_repairable=True) currently passes Pydantic. The generated v0.7 JSON Schema is looser still because model_validator relationships are not emitted: it accepts partial without a reason, complete with reason: refs_missing, and unavailable + unrelated_histories + fetch_repairable: true. Downstream consumers that validate against the published schema can therefore accept states the Python model rejects, including a repair flag on a diff that claims to be complete. Please make any true repairability flag require a fetch-repairable reason and encode the completeness/reason/repair relationships in the generated schema, for example with a tagged union or allOf conditions.