From a781b3d6232faa5e15a11d55b8f5d5cf83072d4d Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 18 Oct 2026 23:11:10 +0800 Subject: [PATCH 1/2] fix(decision): derive the exit code and the merge verdict from the one decision engine The exit code dropped every suppressed finding before matching fail_on, while build_release_decision keeps a suppressed mandatory current-surface control as a non-waivable blocker. Advisory mode with fail_on therefore exited 0 on a report whose decision was blocked by a blocker in that very fail_on (support_refund_agent with three checks.ignore entries). exit_code_for_report now takes the blockers the engine computed and exits 20 when one has a severity in fail_on, in either mode. Strict exit codes cannot move: a non-empty blockers list is a blocked decision, which strict mode already fails. validate_semantic_consistency refuses a report whose blocker in fail_on would not fail CI. The verify-run outcome, the verification receipt and the safety qualification script read the decision-to-merge-verdict projection from schemas/verifier.py instead of holding copies of its table; a test refuses a new dict-literal copy under src/ or scripts/. Closes #1029 --- CHANGELOG.md | 1 + STABILITY.md | 9 +- scripts/run_safety_qualification.py | 11 +- src/agents_shipgate/ci/exit_policy.py | 30 ++- src/agents_shipgate/ci/release_decision.py | 8 +- .../core/semantic_consistency.py | 10 + .../schemas/verification_identity.py | 17 +- src/agents_shipgate/schemas/verify_run.py | 25 +- tests/test_release_decision.py | 195 +++++++++++++++- tests/test_scan.py | 94 ++++++++ tests/test_verdict_contract.py | 220 ++++++++++++++++++ 11 files changed, 574 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac9ee4d07..5ed3249a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ ### Changes +- **`checks.ignore` no longer turns a configured advisory gate green while the decision says `blocked`.** A suppression cannot waive a mandatory current-surface control (the four dedicated control families, such as `SHIP-ACTION-FINANCIAL-WRITE-CONTROL-MISSING`, and engine-raised control-pack obligations), so the release decision kept such a finding as a blocker; the exit code dropped it with every other suppressed finding before matching `fail_on`. `samples/support_refund_agent` in advisory mode with `fail_on: [critical]` and three `checks.ignore` entries printed `Decision: blocked`, `would_fail_ci=false` and exited `0`; without the entries it exited `20`. The exit code now also reads the decision's `blockers[]`: a blocker whose severity is in `fail_on` (including `verify`'s base `ci.fail_on` floor) exits `20` in either mode, for `scan`, `verify` and the Action, and `fail_policy.exit_code` / `would_fail_ci` in `report.json` and `verifier.json` say the same. Strict-mode exit codes are unchanged, because a blocker already meant a `blocked` decision, which strict mode fails; advisory mode without `fail_on` still exits `0`; and a suppression still waives every other finding. A report whose blocker in `fail_on` would not fail CI is refused when it is written. Separately, `verify-run.json`'s outcome, the verification receipt and the safety qualification script now read the decision-to-merge-verdict projection from `schemas/verifier.py` instead of carrying their own copies of its table, which would have refused every verify run the day that table changed; a test refuses a new copy. Migration: [STABILITY.md](STABILITY.md#release-decision-truth-table). (#1029) - **A Claude Code setting that runs a command, or an `env` entry that redirects requests or loads code, is a host-grant row, and a settings change no row shows is no longer "no change".** Adding `"statusLine": {"command": "curl … | sh"}`, `"apiKeyHelper": "curl … | sh"`, `env.ANTHROPIC_BASE_URL` or `env.NODE_OPTIONS` to `.claude/settings.json` gave no row: `diff` (text and `--json`), `verify`'s host comparison and the PR comment headlined `No static host-grant changes detected. No verdict is implied.`, and `audit --host` reported Claude Code coverage `complete`. Host-grants `0.9` (extended in place) now publishes `command_setting` grants for `statusLine`, `subagentStatusLine`, `fileSuggestion`, `apiKeyHelper`, `otelHeadersHelper`, `awsAuthRefresh` and `awsCredentialExport` (`high`, the command shown as a hook's is: name, digest and shape, never text) and `env` grants for endpoint, proxy, certificate, search-path and loader names (`*_BASE_URL`, OpenTelemetry exporter endpoints, `HTTPS_PROXY` and the other proxies, `NODE_EXTRA_CA_CERTS`, `NODE_TLS_REJECT_UNAUTHORIZED`, `SSL_CERT_FILE`, `NODE_OPTIONS`, `BASH_ENV`, `PATH`, `PYTHONPATH`, `LD_PRELOAD`, `GIT_SSH_COMMAND` and the other names `docs/host-boundary-support.md` lists, read without regard to case; `high`, the value digested and never published), so adding one is a `⚠ high added` row and editing one a `changed` row naming the two digests. A settings file's other keys are published as display-only `unmodelled_keys` with digests; when one changes with no row, the coverage item names it in `detail` and the headline reads `Changed, not compared: .claude/settings.json (keys this entry does not compare: …)`, as it does, without keys, for any settings, MCP or hook file that changed with no row. A key no reader models that is shaped as a command (an object with a `command`, or a `…Helper` string) is a blocking limit, so `audit --host` reports coverage `partial`, as for `policyHelper`. The cold-start benchmark's oracle moves those seven settings and those `env` names from unsupported to supported. Migration: [STABILITY.md](STABILITY.md#host-commands-env-1027). (#1027) - **An n8n MCP Client Tool that exposes every tool of its server is read as doing so.** n8n's node selects its tools with `include` (`all`, `selected` or `except`) and leaves it out of an exported workflow at its default, `all`; the reader recognised only `toolSelection`, `toolsToInclude` and `toolSelectionMode`, which n8n does not write. So the default export, `include: all` and `include: except` each entered the catalog as one concretely named tool, with no wildcard, no dynamic surface and no `SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED`. Each is now one `.*` action, which raises `SHIP-INVENTORY-WILDCARD-TOOLS` and, with no local inventory declared, `SHIP-N8N-DYNAMIC-TOOL-SURFACE-NOT-ENUMERABLE` and the unfiltered finding (`high` in `production_like` and `production`, `medium` otherwise); an `except` node's `excludeTools` are listed in the finding's `excluded_tools`; `selected` names its `includeTools`, and a `selected` node naming none is read as `all`, as n8n runs it. The earlier spellings remain aliases, and `include` decides when both are present. A workflow whose MCP Client Tool keeps n8n's default now reports these findings where it passed; select explicit tools in the node, or declare a local MCP inventory. Migration: [STABILITY.md](STABILITY.md#mcp-reader-conformance-1025). (#1025) - **`SHIP-MCP-ENV-SECRET-PASSTHROUGH` counts a secret an MCP server forwards by name, not only an `env` entry.** A secret-like variable in Codex's `env_vars` list or `env_http_headers`, any `bearer_token_env_var`, and a `${NAME}` reference in a `headers` value (`.mcp.json`) now fire the finding (an editor's `${env:NAME}` is read as `NAME` there and in `env`, and a prompted `${input:…}` names no variable) and add to the server's risk score as an `env` entry always did. A credential-named header (`Authorization`) counts the variable it carries whatever that variable is called, and a value written where a variable name belongs is redacted in the evidence, not echoed. (#1025) diff --git a/STABILITY.md b/STABILITY.md index d9446871c..1aad92dfe 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -5166,7 +5166,7 @@ These are **intentionally different signals**, kept apart for backwards compatib #### Release decision truth table -The classification below is the contract for how every active finding lands in `release_decision.{blockers, review_items}[]` and which `contribution_rules[].rule` (v0.17+) fires for it. Starting in v0.33, a finding with typed `support` is considered active for release contribution only when `support.policy_eligible=true`; otherwise it is excluded with `rule="unsupported_evidence"` before severity or `blocks_release` is consulted. Suppressed findings are excluded with `rule="suppressed"` after that evidence-eligibility check. Legacy findings without typed support retain the table's established behavior. +The classification below is the contract for how every active finding lands in `release_decision.{blockers, review_items}[]` and which `contribution_rules[].rule` (v0.17+) fires for it. Starting in v0.33, a finding with typed `support` is considered active for release contribution only when `support.policy_eligible=true`; otherwise it is excluded with `rule="unsupported_evidence"` before severity or `blocks_release` is consulted. Suppressed findings are excluded with `rule="suppressed"` after that evidence-eligibility check, except a mandatory current-surface control (the four dedicated control families and engine-raised control-pack obligations): `checks.ignore` records it but cannot waive it, so it stays a blocker with `rule="policy_block_new"` whatever its baseline status (the last row). Legacy findings without typed support retain the table's established behavior. Notation: `fail_on` is `release_decision.fail_policy.fail_on` after `ci_mode` resolution (advisory → empty, strict → `["critical"]`, plus any explicit `--fail-on` override). `blocker_severities` = `{critical} ∪ fail_on`. `review_tier` = `{critical, high, medium}` (or any severity when `requires_human_review=true`). @@ -5182,14 +5182,17 @@ Notation: `fail_on` is `release_decision.fail_policy.fail_on` after `ci_mode` re | false | any | new / null | no | yes | review_item | `review_required` | 0 | | false | any | matched | no | yes | review_item | `review_required` | 0 | | false | any | new / null / matched | no | no | excluded | `sub_threshold` | 0 | +| suppressed mandatory current-surface control | any | new / null / matched | n/a | n/a | **blocker** | `policy_block_new` | 20 | **Why baseline-matched policy findings drop to `review_items`, not `blockers`.** `blocks_release=true` represents an explicit *policy* decision (Action Surface Diff rule, `action_surface:` manifest entry, or policy-pack rule with `block: true`) that the finding must block release **on first appearance**. A baseline accepts technical debt that already passed prior review — the project agreed to ship with that finding present. Treating baselined policy debt as a hard blocker would defeat the purpose of `baseline save`. The baseline-aware drop is symmetric for severity-driven blockers and policy blockers: both land in `review_items` once accepted into the baseline, both become hard blockers if newly introduced. **Why `severity ∈ blocker_severities + matched + below review_tier` lands in `excluded`, not `review_items`.** A finding whose severity isn't in `{critical, high, medium}` (and which doesn't carry `requires_human_review=true`) has nothing for a human reviewer to act on per the v0.8 contract — it's been baselined and isn't severe enough to warrant attention. v0.17 records this in the audit so the (rare) edge case isn't silently invisible, but the `blockers[]`/`review_items[]` lists themselves are unchanged. -**Why exit code 20 depends on `--baseline-mode`.** `release_decision.{blockers, review_items}[]` always include the full set computed against `report.findings` (with suppressed excluded). The strict-mode exit code, however, is computed from `baseline_filtered_active(report, new_findings_only=...)` — when `--baseline-mode new-findings` is set (the default for the GitHub Action when `baseline:` is provided), baseline-matched policy and severity blockers are filtered out before the exit check, so exit is `0`. With `new_findings_only=False`, a matched policy blocker still triggers exit 20. The `release_decision` block remains baseline-aware in all cases; only the exit-code path changes mode. +**Why exit code 20 depends on `--baseline-mode`.** `release_decision.{blockers, review_items}[]` always include the full set computed against `report.findings` (with suppressed findings excluded, except a mandatory current-surface control, which stays a blocker). The exit code's finding check, however, is computed from `baseline_filtered_active(report, new_findings_only=...)` — when `--baseline-mode new-findings` is set (the default for the GitHub Action when `baseline:` is provided), baseline-matched policy and severity blockers are filtered out before the exit check, so exit is `0`. With `new_findings_only=False`, a matched policy blocker still triggers exit 20. The `release_decision` block remains baseline-aware in all cases; only the exit-code path changes mode. -**`verify`'s base `fail_on` floor (#1024).** With a base and without `--fail-on`, `verify` passes the base manifest's declared `ci.fail_on` to the exit computation only. The exit column then also reads `20` for any row whose finding is active after the same baseline filtering and whose severity is in that list, in either CI mode. The category and `rule` columns do not move, because `blocker_severities` stays `{critical} ∪ fail_policy.fail_on`, the change's own list. +**The exit code reads the decision's blockers (#1029).** Besides that finding check, the exit code is `20` whenever a `release_decision.blockers[]` entry has a severity in `fail_on`, in either CI mode. Suppression removes a finding from the finding check, so this is what keeps a suppressed mandatory current-surface control failing a configured `fail_on`: advisory mode with `fail_on: [critical]` exits `20` on the blocked decision such a control produces, as strict mode already did through its `blocked` rule. In strict mode the rule adds nothing: a non-empty `blockers[]` is a `blocked` decision, which already exits `20`. A suppression still waives every other finding, in both modes. + +**`verify`'s base `fail_on` floor (#1024).** With a base and without `--fail-on`, `verify` passes the base manifest's declared `ci.fail_on` to the exit computation only. The exit column then also reads `20` for any row whose finding is active after the same baseline filtering, or is a blocker, and whose severity is in that list, in either CI mode. The category and `rule` columns do not move, because `blocker_severities` stays `{critical} ∪ fail_policy.fail_on`, the change's own list. Concretely: a scan with one baseline-matched critical and zero new findings produces `summary.status = "release_blockers_detected"` AND `release_decision.decision = "review_required"`. Both are correct under their respective contracts. New consumers should read `release_decision.decision`. diff --git a/scripts/run_safety_qualification.py b/scripts/run_safety_qualification.py index b2eb8dae0..7b5fecee6 100644 --- a/scripts/run_safety_qualification.py +++ b/scripts/run_safety_qualification.py @@ -62,7 +62,7 @@ tier_for_requirements, ) from agents_shipgate.schemas.verification_identity import VerificationReceipt -from agents_shipgate.schemas.verifier import VerifierArtifact +from agents_shipgate.schemas.verifier import VerifierArtifact, map_merge_verdict from agents_shipgate.schemas.verify_run import VerifyRunArtifact if __package__: @@ -83,12 +83,9 @@ "blocked", ) QUALIFYING_ORIGINS = {"real_history", "rejected_or_reverted", "design_partner"} -EXPECTED_MERGE_VERDICT = { - "passed": "mergeable", - "review_required": "human_review_required", - "insufficient_evidence": "insufficient_evidence", - "blocked": "blocked", -} +# The verifier's projection, read from its one authority rather than copied: +# a copy would reject every receipt the day that authority changes (#1029). +EXPECTED_MERGE_VERDICT = {decision: map_merge_verdict(decision) for decision in DECISIONS} # Match the receipt reader's per-artifact bound; wheels have their own bound # because they contain the whole distribution. These are input limits, never diff --git a/src/agents_shipgate/ci/exit_policy.py b/src/agents_shipgate/ci/exit_policy.py index 013afd274..695467f0b 100644 --- a/src/agents_shipgate/ci/exit_policy.py +++ b/src/agents_shipgate/ci/exit_policy.py @@ -1,9 +1,12 @@ from __future__ import annotations +from collections.abc import Sequence + from agents_shipgate.schemas.common import ReleaseDecisionStatus, Severity from agents_shipgate.schemas.report import ( Finding, ReadinessReport, + ReleaseDecisionItem, ) GATE_FAILURE_EXIT_CODE = 20 @@ -25,6 +28,10 @@ def baseline_filtered_active(report: ReadinessReport, *, new_findings_only: bool Findings with typed support therefore contribute to process policy only when that support is policy-eligible. Legacy findings without support keep their established behavior for frozen-report compatibility. + + Suppression drops a finding here, but it does not drop a blocker the + decision engine keeps non-waivable: those reach the exit code through the + ``blockers`` argument of :func:`exit_code_for_report` (#1029). """ active = [ finding @@ -41,13 +48,22 @@ def exit_code_for_report( report: ReadinessReport, ci_mode: str, *, + blockers: Sequence[ReleaseDecisionItem], fail_on: list[Severity] | None = None, new_findings_only: bool = False, release_decision: ReleaseDecisionStatus | None = None, has_semantic_gaps: bool = False, ) -> int: - # Summary counts exclude suppressed findings, so strict mode intentionally - # passes when every critical finding has an explicit suppression reason. + """The process exit code for a scan, read from the one decision engine. + + ``blockers`` and ``release_decision`` are what ``build_release_decision`` + computed for this report; they are required so that no caller can derive + an exit code beside the decision instead of from it. The exit code is not + a projection of the decision alone (see STABILITY.md "Exit codes"): in + advisory mode it is 20 only when the configured ``fail_on`` matches. But + when it does match, it matches what the decision blocks on, including a + blocker no suppression can waive. + """ fail_on_resolved = effective_fail_on(ci_mode, fail_on) active = baseline_filtered_active(report, new_findings_only=new_findings_only) # v0.29: the semantic gate is not represented as a Finding on purpose — @@ -63,6 +79,16 @@ def exit_code_for_report( return GATE_FAILURE_EXIT_CODE if not fail_on_resolved: return 0 + # A suppression explains a waivable finding, so a suppressed finding + # leaves `active` and no longer matches `fail_on`. It cannot waive a + # mandatory current-surface control: the decision engine keeps that one + # as a blocker, and the exit code reads the engine's blockers rather than + # restating which suppressions count (#1029). Without this, advisory mode + # with `fail_on` exited 0 on a report whose decision was `blocked` by a + # blocker in that very `fail_on`. Strict mode already exits above on any + # `blocked` decision, so this changes no strict exit code. + if any(item.severity in fail_on_resolved for item in blockers): + return GATE_FAILURE_EXIT_CODE if any(finding.severity in fail_on_resolved for finding in active): return GATE_FAILURE_EXIT_CODE return 0 diff --git a/src/agents_shipgate/ci/release_decision.py b/src/agents_shipgate/ci/release_decision.py index 32c970037..0c1124aca 100644 --- a/src/agents_shipgate/ci/release_decision.py +++ b/src/agents_shipgate/ci/release_decision.py @@ -206,10 +206,13 @@ def build_release_decision( # blockers/review_items consider the full findings set, NOT # new_findings_only: baseline-matched criticals must remain visible # as accepted debt in review_items. The new_findings_only filter - # only affects fail_policy.exit_code (via exit_code_for_report). + # only affects fail_policy.exit_code (via exit_code_for_report, which + # also reads these blockers, so a blocker no suppression can waive + # still fails a matching fail_on — #1029). # v0.17: iterate report.findings directly so the contribution_rules # audit row set is exhaustive (suppressed findings get an audit row - # too, classified as excluded/suppressed). + # too, classified as excluded/suppressed, or as a blocker when the + # suppression cannot waive a mandatory current-surface control). blockers: list[ReleaseDecisionItem] = [] review_items: list[ReleaseDecisionItem] = [] contribution_rules: list[ContributionRule] = [] @@ -453,6 +456,7 @@ def build_release_decision( exit_code = exit_code_for_report( report, ci_mode, + blockers=blockers, fail_on=exit_fail_on, new_findings_only=new_findings_only, release_decision=decision, diff --git a/src/agents_shipgate/core/semantic_consistency.py b/src/agents_shipgate/core/semantic_consistency.py index 50fe13bd2..906ca9238 100644 --- a/src/agents_shipgate/core/semantic_consistency.py +++ b/src/agents_shipgate/core/semantic_consistency.py @@ -164,6 +164,16 @@ def validate_semantic_consistency( raise SemanticConsistencyError( "strict insufficient_evidence must produce would_fail_ci=true, exit_code=20" ) + # The exit code reads the decision's blockers (#1029): a blocker whose + # severity is in fail_on fails CI in either mode, whatever suppressed the + # finding behind it, and strict mode fails on any blocked decision. + if ( + any(item.severity in decision.fail_policy.fail_on for item in decision.blockers) + or (decision.fail_policy.ci_mode == "strict" and decision.decision == "blocked") + ) and (decision.fail_policy.would_fail_ci is not True or decision.fail_policy.exit_code != 20): + raise SemanticConsistencyError( + "a release blocker in fail_on must produce would_fail_ci=true, exit_code=20" + ) def _validate_exclusion_ledger(report: ReadinessReport) -> None: diff --git a/src/agents_shipgate/schemas/verification_identity.py b/src/agents_shipgate/schemas/verification_identity.py index 7830ae86a..a87b923a3 100644 --- a/src/agents_shipgate/schemas/verification_identity.py +++ b/src/agents_shipgate/schemas/verification_identity.py @@ -360,17 +360,14 @@ def _receipt_closes_one_identity_graph(self) -> VerificationReceipt: ) if self.decision_id != expected_decision_id: raise ValueError("receipt decision_id must hash its request, units, and outcome") - decision_projection = { - "passed": ("mergeable", True), - "review_required": ("human_review_required", False), - "insufficient_evidence": ("insufficient_evidence", False), - "blocked": ("blocked", False), - } - if self.decision in decision_projection: - expected_merge, expected_can_merge = decision_projection[self.decision] + if self.decision is not None: + # The verifier's projection, not a copy of it (#1029). Imported + # here because ``schemas.verifier`` imports this module. + from agents_shipgate.schemas.verifier import map_merge_verdict + if (self.merge_verdict, self.can_merge_without_human) != ( - expected_merge, - expected_can_merge, + map_merge_verdict(self.decision), + self.decision == "passed", ): raise ValueError("receipt outcome contradicts its release decision") elif (self.merge_verdict, self.can_merge_without_human) not in { diff --git a/src/agents_shipgate/schemas/verify_run.py b/src/agents_shipgate/schemas/verify_run.py index 136a07d12..d9c00ab73 100644 --- a/src/agents_shipgate/schemas/verify_run.py +++ b/src/agents_shipgate/schemas/verify_run.py @@ -15,6 +15,7 @@ VerificationPlan, content_id, ) +from agents_shipgate.schemas.verifier import applicability_for, merge_verdict_for VERIFY_RUN_SCHEMA_VERSION = "shipgate.verify_run/v5" @@ -204,29 +205,17 @@ class VerifyRunOutcome(BaseModel): @model_validator(mode="after") def _control_projects_outcome(self) -> VerifyRunOutcome: - expected_applicability = ( - "verified" - if self.decision is not None - else { - "not_run": "not_evaluated", - "skipped": "not_applicable", - "failed": "failed", - }.get(self.execution, "not_evaluated") + # Both projections come from the verifier's single authority, never a + # copy of its table: a copy keeps validating the old mapping after the + # authority changes, and every verify run then fails here (#1029). + expected_applicability = applicability_for( + decision=self.decision, execution=self.execution ) if self.applicability != expected_applicability: raise ValueError("verify-run applicability contradicts execution") if self.decision is not None and self.execution != "succeeded": raise ValueError("verify-run decisions require succeeded execution") - expected_verdict = ( - { - "passed": "mergeable", - "review_required": "human_review_required", - "insufficient_evidence": "insufficient_evidence", - "blocked": "blocked", - }.get(self.decision, "human_review_required") - if self.decision is not None - else ("mergeable" if self.execution == "skipped" else "unknown") - ) + expected_verdict = merge_verdict_for(decision=self.decision, execution=self.execution) if self.merge_verdict != expected_verdict: raise ValueError("verify-run merge verdict contradicts its decision") expected = bool( diff --git a/tests/test_release_decision.py b/tests/test_release_decision.py index 06914def9..7ee0a82b4 100644 --- a/tests/test_release_decision.py +++ b/tests/test_release_decision.py @@ -138,6 +138,27 @@ def _build( ) +#: A mandatory current-surface control: ``checks.ignore`` records the +#: exception, but the engine keeps the finding a blocker (#1029). +_MANDATORY_CONTROL = "SHIP-ACTION-FINANCIAL-WRITE-CONTROL-MISSING" + + +def _mandatory_control( + *, + severity: str = "critical", + baseline_status: str | None = None, + suppressed: bool = True, +) -> Finding: + finding = _finding( + check_id=_MANDATORY_CONTROL, + severity=severity, + baseline_status=baseline_status, + suppressed=suppressed, + ) + finding.blocks_release = True + return finding + + def test_advisory_with_new_critical_blocks_but_does_not_fail_ci(): report = _report(findings=[_finding(severity="critical", baseline_status="new")]) decision = _build(report, ci_mode="advisory") @@ -620,19 +641,28 @@ def test_insufficient_evidence_reason_lists_both_counts(): def test_fail_policy_exit_code_matches_exit_code_for_report(): """The shared-helper refactor must keep release_decision.fail_policy.exit_code - in lockstep with the standalone exit_code_for_report() across the matrix.""" + in lockstep with the standalone exit_code_for_report() across the matrix, + given the blockers and decision the engine computed.""" matrix = [ ("advisory", None, False, [_finding(severity="critical")]), ("strict", None, False, [_finding(severity="critical")]), ("strict", ["high"], False, [_finding(severity="high")]), ("strict", ["critical"], True, [_finding(severity="critical", baseline_status="matched")]), ("advisory", None, False, []), + ("advisory", ["critical"], False, [_mandatory_control()]), + ("advisory", ["critical"], False, [_finding(severity="critical", suppressed=True)]), + ("strict", None, False, [_mandatory_control()]), ] for ci_mode, fail_on, new_only, findings in matrix: report = _report(findings=findings) decision = _build(report, ci_mode=ci_mode, fail_on=fail_on, new_findings_only=new_only) expected = exit_code_for_report( - report, ci_mode, fail_on=fail_on, new_findings_only=new_only + report, + ci_mode, + blockers=decision.blockers, + fail_on=fail_on, + new_findings_only=new_only, + release_decision=decision.decision, ) assert decision.fail_policy.exit_code == expected, ( f"mismatch for ci_mode={ci_mode}, fail_on={fail_on}, new_findings_only={new_only}" @@ -666,8 +696,156 @@ def test_exit_code_is_the_ci_gate_signal_not_the_release_decision( report = _report() - assert exit_code_for_report(report, "advisory", release_decision=decision) == advisory_exit - assert exit_code_for_report(report, "strict", release_decision=decision) == strict_exit + assert ( + exit_code_for_report(report, "advisory", blockers=(), release_decision=decision) + == advisory_exit + ) + assert ( + exit_code_for_report(report, "strict", blockers=(), release_decision=decision) + == strict_exit + ) + + +_MATCHED_BASELINE = BaselineSummary(path=".agents-shipgate/baseline.json", matched_count=1) + + +@pytest.mark.parametrize( + ("fail_on", "new_findings_only", "baseline_status", "expected_exit"), + [ + # #1029: the suppression dropped the finding from the exit code's + # active set but not from the decision's blockers, so advisory + + # fail_on exited 0 on a `blocked` decision. + (["critical"], False, None, GATE_FAILURE_EXIT_CODE), + (["critical"], True, "new", GATE_FAILURE_EXIT_CODE), + (["critical", "high"], False, None, GATE_FAILURE_EXIT_CODE), + # Advisory semantics are otherwise unchanged: no fail_on, no failure; + # a fail_on that does not name the blocker's severity, no failure. + (None, False, None, 0), + (["high"], False, None, 0), + # The engine keeps a suppressed mandatory control a blocker whatever + # its baseline status, so the exit follows it — as strict mode's + # `blocked` rule already did. + (["critical"], True, "matched", GATE_FAILURE_EXIT_CODE), + ], +) +def test_advisory_fail_on_fails_on_a_blocker_no_suppression_can_waive( + fail_on, new_findings_only, baseline_status, expected_exit +): + report = _report( + findings=[_mandatory_control(baseline_status=baseline_status)], + baseline=_MATCHED_BASELINE if baseline_status == "matched" else None, + ) + + decision = _build( + report, ci_mode="advisory", fail_on=fail_on, new_findings_only=new_findings_only + ) + + assert decision.decision == "blocked" + assert [item.check_id for item in decision.blockers] == [_MANDATORY_CONTROL] + assert decision.fail_policy.exit_code == expected_exit + assert decision.fail_policy.would_fail_ci is (expected_exit != 0) + + +@pytest.mark.parametrize("ci_mode", ["advisory", "strict"]) +def test_a_suppression_still_waives_a_finding_that_is_not_a_mandatory_control(ci_mode): + """`checks.ignore` keeps clearing `fail_on` for every waivable finding.""" + + tools = [_tool()] + report = _report(findings=[_finding(severity="critical", suppressed=True)], tools=tools) + + decision = _build(report, ci_mode=ci_mode, tools=tools, fail_on=["critical"]) + + assert decision.blockers == [] + assert [rule.rule for rule in decision.contribution_rules] == ["suppressed"] + assert decision.fail_policy.exit_code == 0 + assert decision.fail_policy.would_fail_ci is False + + +def test_the_base_fail_on_floor_reads_the_same_blockers(): + """`verify`'s base `ci.fail_on` floor (#1024) reaches the same blocker.""" + + report = _report(findings=[_mandatory_control()]) + + decision = build_release_decision( + report=report, + tools=[], + ci_mode="advisory", + fail_on=None, + new_findings_only=False, + exit_fail_on_floor=["critical"], + ) + + assert decision.decision == "blocked" + # The floor reaches the exit code only; the run's own list is reported. + assert decision.fail_policy.fail_on == [] + assert decision.fail_policy.exit_code == GATE_FAILURE_EXIT_CODE + + +@pytest.mark.parametrize( + ("finding_factory", "fail_on", "new_findings_only", "expected_exit"), + [ + (lambda: _mandatory_control(), None, False, GATE_FAILURE_EXIT_CODE), + (lambda: _mandatory_control(severity="high"), None, False, GATE_FAILURE_EXIT_CODE), + ( + lambda: _mandatory_control(baseline_status="matched"), + None, + True, + GATE_FAILURE_EXIT_CODE, + ), + (lambda: _finding(severity="critical", suppressed=True), None, False, 0), + (lambda: _finding(severity="critical", suppressed=True), ["critical"], False, 0), + (lambda: _finding(severity="critical"), None, False, GATE_FAILURE_EXIT_CODE), + (lambda: _finding(severity="critical", baseline_status="matched"), None, True, 0), + ( + lambda: _finding(severity="critical", baseline_status="matched"), + None, + False, + GATE_FAILURE_EXIT_CODE, + ), + (lambda: _finding(severity="high"), None, False, 0), + (lambda: _finding(severity="high"), ["high"], False, GATE_FAILURE_EXIT_CODE), + (lambda: _policy_finding(severity="medium"), None, False, GATE_FAILURE_EXIT_CODE), + ( + lambda: _policy_finding(severity="medium", baseline_status="matched"), + None, + True, + 0, + ), + ( + lambda: _policy_finding(severity="medium", baseline_status="matched"), + None, + False, + GATE_FAILURE_EXIT_CODE, + ), + ], +) +def test_strict_exit_codes_are_unchanged( + finding_factory, fail_on, new_findings_only, expected_exit +): + """Strict mode already exits 20 on any `blocked` decision (#1029). + + Reading the engine's blockers therefore moves no strict exit code. These + rows pin the strict codes this change could have touched — suppressed, + baseline-matched, mandatory or not — and pass unchanged before it. + """ + + finding = finding_factory() + tools = [_tool()] + report = _report( + findings=[finding], + tools=tools, + baseline=_MATCHED_BASELINE if finding.baseline_status == "matched" else None, + ) + + decision = _build( + report, + ci_mode="strict", + tools=tools, + fail_on=fail_on, + new_findings_only=new_findings_only, + ) + + assert decision.fail_policy.exit_code == expected_exit def test_summary_status_remains_baseline_blind(): @@ -898,6 +1076,15 @@ def _policy_finding( False, False, ), + ( + "suppressed_mandatory_control_stays_a_blocker", + lambda: _mandatory_control(baseline_status="new"), + {"ci_mode": "advisory"}, + "blocker", + "policy_block_new", + True, + False, + ), ], ) def test_contribution_rules_truth_table( diff --git a/tests/test_scan.py b/tests/test_scan.py index 2c798b372..fc8445d9a 100644 --- a/tests/test_scan.py +++ b/tests/test_scan.py @@ -348,6 +348,100 @@ def test_fail_on_high_can_fail_ci_without_critical(tmp_path): assert exit_code == 20 +#: The three critical controls the #1029 repro ignores. Only the last is a +#: mandatory current-surface control, which a suppression cannot waive. +_IGNORED_CRITICAL_CONTROLS = ( + "SHIP-POLICY-APPROVAL-MISSING", + "SHIP-SIDEFX-IDEMPOTENCY-MISSING", + "SHIP-ACTION-FINANCIAL-WRITE-CONTROL-MISSING", +) + + +def _advisory_support_refund_sample( + tmp_path: Path, *, ignored: tuple[str, ...], fail_on: list[str] | None +) -> Path: + root = tmp_path / "support_refund_agent" + shutil.copytree(SAMPLE.parent, root, ignore=shutil.ignore_patterns("expected")) + manifest_path = root / "shipgate.yaml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["ci"]["mode"] = "advisory" + if fail_on is not None: + manifest["ci"]["fail_on"] = fail_on + manifest["checks"]["ignore"].extend( + {"check_id": check_id, "reason": "accepted for #1029"} for check_id in ignored + ) + manifest_path.write_text(yaml.safe_dump(manifest, sort_keys=False), encoding="utf-8") + return manifest_path + + +@pytest.mark.parametrize( + ("ignored", "fail_on", "expected_exit"), + [ + # The repro: the ignores must not turn the configured gate green. + (_IGNORED_CRITICAL_CONTROLS, ["critical"], 20), + # Control: the same gate without the ignores. + ((), ["critical"], 20), + # Advisory without fail_on still never fails CI, blocked or not. + (_IGNORED_CRITICAL_CONTROLS, None, 0), + ], +) +def test_checks_ignore_cannot_turn_an_advisory_fail_on_gate_green( + tmp_path, ignored, fail_on, expected_exit +): + """#1029: the exit code reads the blockers the decision engine keeps.""" + + manifest_path = _advisory_support_refund_sample(tmp_path, ignored=ignored, fail_on=fail_on) + out = tmp_path / "reports" + + result = CliRunner().invoke( + app, + ["scan", "--config", str(manifest_path), "--out", str(out), "--no-plugins"], + ) + + assert result.exit_code == expected_exit, result.output + report = json.loads((out / "report.json").read_text(encoding="utf-8")) + decision = report["release_decision"] + assert decision["decision"] == "blocked" + assert decision["fail_policy"]["exit_code"] == expected_exit + assert decision["fail_policy"]["would_fail_ci"] is (expected_exit != 0) + assert { + finding["check_id"] for finding in report["findings"] if finding["suppressed"] + } == set(ignored) + # The waivable controls stay waived; the mandatory one stays a blocker. + blocker_ids = {item["check_id"] for item in decision["blockers"]} + assert "SHIP-ACTION-FINANCIAL-WRITE-CONTROL-MISSING" in blocker_ids + assert ("SHIP-POLICY-APPROVAL-MISSING" in blocker_ids) is not bool(ignored) + assert ("SHIP-SIDEFX-IDEMPOTENCY-MISSING" in blocker_ids) is not bool(ignored) + + +def test_a_blocker_in_fail_on_that_would_not_fail_ci_is_refused_at_emission( + tmp_path, monkeypatch +): + """Negative control for #1029: an exit rule that stops reading the + decision's blockers fails the scan instead of shipping a green gate.""" + + from agents_shipgate.ci import release_decision + from agents_shipgate.core.semantic_consistency import SemanticConsistencyError + + reads_blockers = release_decision.exit_code_for_report + + def ignores_blockers(*args, blockers, **kwargs): + return reads_blockers(*args, blockers=(), **kwargs) + + monkeypatch.setattr(release_decision, "exit_code_for_report", ignores_blockers) + manifest_path = _advisory_support_refund_sample( + tmp_path, ignored=_IGNORED_CRITICAL_CONTROLS, fail_on=["critical"] + ) + + with pytest.raises(SemanticConsistencyError, match="release blocker in fail_on"): + run_scan( + config_path=manifest_path, + output_dir=tmp_path / "reports", + formats=["json"], + plugins_enabled=False, + ) + + def test_agent_finding_does_not_emit_duplicate_policy_evidence_source(tmp_path): """v0.19 reviewer-grade provenance: agent-level findings have a single citation site (the manifest pointer IS the primary diff --git a/tests/test_verdict_contract.py b/tests/test_verdict_contract.py index 4f1023c44..1837f1d4c 100644 --- a/tests/test_verdict_contract.py +++ b/tests/test_verdict_contract.py @@ -12,6 +12,8 @@ from __future__ import annotations +import ast +from pathlib import Path from typing import get_args import pytest @@ -27,16 +29,26 @@ ReleaseDecision, ReviewerSummary, ) +from agents_shipgate.schemas.verification_identity import ( + VerificationArtifactManifest, + VerificationArtifactRef, + VerificationReceipt, + content_id, +) from agents_shipgate.schemas.verifier import ( _DECISION_TO_VERDICT, + MergeVerdict, VerifierArtifact, VerifierDiffStatus, applicability_for, map_merge_verdict, merge_verdict_for, ) +from agents_shipgate.schemas.verify_run import VerifyRunOutcome CANONICAL = set(get_args(ReleaseDecisionStatus)) +MERGE_VERDICTS = get_args(MergeVerdict) +REPO_ROOT = Path(__file__).resolve().parents[1] # --- One vocabulary: every verdict surface shares the canonical enum -------- @@ -298,3 +310,211 @@ def test_artifact_model_validate_backfills_applicability_for_old_payloads() -> N } ) assert skipped.applicability == "not_applicable" + + +# --- Every validator reads the one projection (#1029) ----------------------- + + +def test_receipt_vocabulary_is_the_canonical_one() -> None: + """The receipt spells its enums inline; they must stay the canonical ones. + + ``schemas.verifier`` imports ``verification_identity``, so the receipt + cannot import ``MergeVerdict`` for its annotation. The published v1 + receipt schema is generated from these literals, so the order is pinned. + """ + + fields = VerificationReceipt.model_fields + assert get_args(fields["merge_verdict"].annotation) == MERGE_VERDICTS + (decision,) = ( + arg for arg in get_args(fields["decision"].annotation) if arg is not type(None) + ) + assert set(get_args(decision)) == CANONICAL + + +def _control_for(status: str): + if status == "passed": + return derive_agent_control(reason="Static verification passed.", subject_evaluated=True) + return derive_agent_control( + reason=f"Release decision is {status}.", human_review_required=True + ) + + +def _verifier_artifact(status: str, merge_verdict: str) -> VerifierArtifact: + return _artifact( + execution="succeeded", + release_decision=_release_decision(status), + decision=status, + merge_verdict=merge_verdict, + applicability="verified", + can_merge_without_human=status == "passed", + control=_control_for(status), + ) + + +def _verify_run_outcome(status: str, merge_verdict: str) -> VerifyRunOutcome: + return VerifyRunOutcome( + exit_code=0, + base_status="succeeded", + execution="succeeded", + applicability="verified", + decision=status, + merge_verdict=merge_verdict, + can_merge_without_human=status == "passed", + control=_control_for(status), + ) + + +def _receipt(status: str, merge_verdict: str) -> VerificationReceipt: + """A receipt whose content ids all hold, so only the projection decides.""" + + request_id = content_id("request") + unit_result_ids = [content_id("unit")] + can_merge = status == "passed" + decision_id = content_id( + { + "request_id": request_id, + "unit_result_ids": unit_result_ids, + "decision": status, + "merge_verdict": merge_verdict, + "can_merge_without_human": can_merge, + } + ) + manifest_fields = { + "request_id": request_id, + "decision_id": decision_id, + "artifacts": { + "report": VerificationArtifactRef( + path="report.json", + sha256=content_id("report"), + size_bytes=2, + media_type="application/json", + ) + }, + } + manifest = VerificationArtifactManifest( + artifact_set_id=content_id( + VerificationArtifactManifest.model_construct( + artifact_set_id="", **manifest_fields + ).model_dump(mode="json", exclude={"artifact_set_id", "schema_version"}) + ), + **manifest_fields, + ) + fields = { + "request_id": request_id, + "subject_id": content_id("subject"), + "input_set_id": content_id("inputs"), + "engine_requirement_id": content_id("engine"), + "executor_id": content_id("executor"), + "decision_id": decision_id, + "artifact_set_id": manifest.artifact_set_id, + "unit_result_ids": unit_result_ids, + "decision": status, + "merge_verdict": merge_verdict, + "can_merge_without_human": can_merge, + "artifact_manifest": manifest, + } + receipt_id = content_id( + VerificationReceipt.model_construct(receipt_id="", **fields).model_dump( + mode="json", + exclude={"receipt_id", "schema_version", "attempt_id"}, + exclude_none=False, + ) + ) + return VerificationReceipt(receipt_id=receipt_id, **fields) + + +#: Every validator that checks a decision against its merge verdict, with the +#: error it raises when the two disagree. +VALIDATORS = { + "verifier.json": (_verifier_artifact, "must be the projection"), + "verify-run.json": (_verify_run_outcome, "merge verdict contradicts its decision"), + "verification-receipt.json": (_receipt, "contradicts its release decision"), +} + + +@pytest.mark.parametrize("validator", sorted(VALIDATORS)) +@pytest.mark.parametrize("status", sorted(CANONICAL)) +def test_every_decision_status_runs_through_every_validator(validator, status) -> None: + build, contradiction = VALIDATORS[validator] + expected = map_merge_verdict(status) + + assert build(status, expected).merge_verdict == expected + for other in MERGE_VERDICTS: + if other == expected: + continue + with pytest.raises(ValidationError, match=contradiction): + build(status, other) + + +@pytest.mark.parametrize("validator", sorted(VALIDATORS)) +def test_a_changed_projection_moves_every_validator_with_it(monkeypatch, validator) -> None: + """The failure #1029 describes, staged. + + Re-map ``insufficient_evidence`` in the one table. The verifier then emits + the new verdict, so every validator must accept it and refuse the old one. + A validator holding a copy of the table would refuse every receipt. + """ + + build, contradiction = VALIDATORS[validator] + monkeypatch.setitem(_DECISION_TO_VERDICT, "insufficient_evidence", "human_review_required") + + accepted = build("insufficient_evidence", "human_review_required") + assert accepted.merge_verdict == "human_review_required" + with pytest.raises(ValidationError, match=contradiction): + build("insufficient_evidence", "insufficient_evidence") + + +#: The one module allowed to state the decision -> merge verdict table. +_AUTHORITY = Path("src/agents_shipgate/schemas/verifier.py") +#: Trees that ship or run the engine. ``harness/adoption`` is left out on +#: purpose: its scorer is an independent oracle that never reads the +#: agents-shipgate schema (see the ``harness/adoption/scorer/rules.py`` header). +_SCANNED_TREES = (Path("src/agents_shipgate"), Path("scripts")) + + +def _names_a_merge_verdict(node: ast.AST) -> bool: + if isinstance(node, ast.Constant): + return node.value in MERGE_VERDICTS + if isinstance(node, (ast.Tuple, ast.List)): + return any(_names_a_merge_verdict(element) for element in node.elts) + return False + + +def _decision_to_verdict_literals(source: str) -> list[int]: + """Lines of dict literals with a decision key whose value names a verdict.""" + + return [ + node.lineno + for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.Dict) + and any( + isinstance(key, ast.Constant) + and key.value in CANONICAL + and _names_a_merge_verdict(value) + for key, value in zip(node.keys, node.values, strict=True) + ) + ] + + +def test_the_copy_guard_recognises_both_shapes_of_copy() -> None: + assert _decision_to_verdict_literals('{"passed": "mergeable"}') == [1] + assert _decision_to_verdict_literals('{"blocked": ("blocked", False)}') == [1] + assert _decision_to_verdict_literals('{"passed": "allow", "mergeable": 1}') == [] + + +def test_no_dict_literal_outside_the_verifier_maps_a_decision_to_a_merge_verdict() -> None: + """A copied table keeps validating the old mapping after the authority moves. + + Call ``map_merge_verdict`` or ``merge_verdict_for`` instead (#1029). + """ + + offenders = [ + f"{path.relative_to(REPO_ROOT).as_posix()}:{line}" + for tree in _SCANNED_TREES + for path in sorted((REPO_ROOT / tree).rglob("*.py")) + if path.relative_to(REPO_ROOT) != _AUTHORITY + for line in _decision_to_verdict_literals(path.read_text(encoding="utf-8")) + ] + assert offenders == [] + # The guard must still see the table it protects, or it has gone blind. + assert _decision_to_verdict_literals((REPO_ROOT / _AUTHORITY).read_text(encoding="utf-8")) From 2f85ad7ab47c22c989c1e5aecb5d318bd9e343a6 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 18 Oct 2026 23:14:17 +0800 Subject: [PATCH 2/2] test(verify): a change ignoring a control no suppression can waive still fails CI The verify path reads the same blockers: report.json, verifier.json and the process exit all report exit 20 for an advisory fail_on gate whose only critical blocker is a suppressed mandatory control. --- tests/test_verify_pr_controlled_inputs.py | 51 +++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/test_verify_pr_controlled_inputs.py b/tests/test_verify_pr_controlled_inputs.py index a22c57ddc..5163d64a8 100644 --- a/tests/test_verify_pr_controlled_inputs.py +++ b/tests/test_verify_pr_controlled_inputs.py @@ -612,3 +612,54 @@ def test_dropping_fail_on_beside_a_blocking_tool_still_fails_ci(tmp_path: Path) assert payload["merge_verdict"] == "blocked" assert result.exit_code == 20, result.output assert _fail_policy(out)["fail_on"] == [] + + +def test_ignoring_a_control_no_suppression_can_waive_still_fails_ci(tmp_path: Path) -> None: + """#1029: a change that ignores its own blocker cannot exit 0 under fail_on. + + The change suppresses both critical findings the refund tool raises. The + approval finding is waivable and leaves the gate. The rollback control is + a mandatory current-surface control, so the decision keeps it a blocker, + and the exit code now reads that blocker too: in ``report.json``, in + ``verifier.json`` and in the process exit. + """ + + repo = _sample_repo(tmp_path) + _set_ci(repo, mode="advisory", fail_on=["critical"]) + _add_bound_refund_tool(repo) + _commit(repo, "advisory base failing on critical, with a refund tool") + _git(repo, "checkout", "-q", "-b", "pr") + manifest = repo / "shipgate.yaml" + manifest.write_text( + manifest.read_text(encoding="utf-8") + + "\nchecks:\n ignore:\n" + + "".join( + f" - check_id: {check_id}\n" + " tool: stripe.create_refund\n" + " reason: accepted for this test\n" + for check_id in ( + "SHIP-POLICY-APPROVAL-MISSING", + "SHIP-ACTION-DESTRUCTIVE-ROLLBACK-MISSING", + ) + ), + encoding="utf-8", + ) + _commit(repo, "ignore the refund tool's critical findings") + out = tmp_path / "out" + + result, payload = _verify(repo, "--base", "main", "--head", "HEAD", out=out) + + assert payload["merge_verdict"] == "blocked" + blockers = {item["check_id"] for item in payload["release_decision"]["blockers"]} + assert "SHIP-ACTION-DESTRUCTIVE-ROLLBACK-MISSING" in blockers + assert "SHIP-POLICY-APPROVAL-MISSING" not in blockers + failing = { + "ci_mode": "advisory", + "fail_on": ["critical"], + "new_findings_only": False, + "would_fail_ci": True, + "exit_code": 20, + } + assert _fail_policy(out) == failing + assert payload["release_decision"]["fail_policy"] == failing + assert result.exit_code == 20, result.output