fix(signal): kill phantom findings and un-hide real ones across patterns, breaking, perf, semgrep and coverage

docs/gate-playbook.md

@@ -17,6 +17,12 @@ they must not parse stdout.
 Use `prview gate --json` when CI needs a machine-readable summary, artifact
 paths, or SARIF path discovery. Pass/fail still comes from the process exit code.
 
+Exit `3` covers every way the run can end without a trustworthy verdict — the
+review failing to execute, and the pack's `00_summary/MERGE_GATE.json` being
+missing, unparsable, or stamped with a `schema_version` this build cannot read.
+Plain `prview --ci` uses the same code for the same conditions: it never
+re-derives a verdict when the gate artifact cannot be read.
+
 ## Breaking-change escalation
 
 A genuine breaking API change in the diff — a removed public symbol, a changed
@@ -43,7 +49,11 @@ which command you run. Two contract lines, deliberately distinct:
   hard failure (`BLOCK` or a broken quality gate); a `CONDITIONAL` verdict —
   including a breaking-only `CONDITIONAL` — exits `0`, exactly as it does for any
   other `CONDITIONAL` cause. This is the historical review contract and does not
-  change with breaking-change escalation.
+  change with breaking-change escalation. Warning-level checks are advisory and
+  do not break the quality gate, so a warnings-only run exits `0`; add
+  `--fail-on-warnings` to opt into exit `1` for them. Both `--ci` exits hold
+  whatever preset the run resolves to — `--ci --update` is still strict, and an
+  `--update` run with no new commits takes its exit from the pack it reused.
 * **`prview gate`** — the contractual enforcement path. `CONDITIONAL` exits `1`,
   and `prview gate --strict` exits `2` (see the exit-code contract above).
 

docs/mcp.md

@@ -200,10 +200,60 @@ The decision surface is normalized so callers read one vocabulary:
   clean `PASS`. A permissive flag on disk can never override a block/hold signal.
 
 If the stored gate emits contradictory signals (for example `allow_merge: true`
-alongside a block recommendation), the most conservative signal wins and a
-`core_inconsistency` note is appended to `caveats`. Legacy gate tokens (`ALLOW`,
-`HOLD`) written by older cores are still recognized on read and folded into the
-`PASS` / `CONDITIONAL` surface rather than failing loud.
+alongside a block recommendation, or a clean approval alongside
+`quality_pass: false` — the contract permits `PASS` only when quality passes),
+the most conservative signal wins and a `core_inconsistency` note is appended to
+`caveats`. The note reports a
+disagreement the pack actually states — the textual axes against the published
+verdict, `allow_merge` against the flag published — so a self-consistent
+`BLOCK` pack (`verdict: "BLOCK"`, `merge_recommendation: "block"`,
+`allow_merge: false`) raises no caveat at all. The CLI `--json` surface
+reconciles the same way through the same ranking
+(`gate::rank_from_verdict` / `gate::rank_from_merge_rec`), so the two surfaces
+cannot disagree about a contradictory pack. Legacy gate tokens (`ALLOW`,
+`APPROVE`, `HOLD`) written by older cores are still recognized on read and folded
+into the `PASS` / `CONDITIONAL` surface rather than failing loud. That fold is
+`gate::canonical_verdict`, shared by this adapter, the CLI summary and
+`prview gate`, and it ignores case: a stored `"pass"` reads as `PASS` on every
+surface instead of approving on one and normalizing to `BLOCK` on another.
+
+Anything the adapter could not read is named rather than dropped, and every such
+case sets `normalized: true`:
+
+- `unknown_verdict:` / `unknown_merge_recommendation:` — the field was present
+  but outside the known vocabulary, so it was ignored when deriving the decision.
+  A verdict that could not be ranked — outside the vocabulary, or simply absent
+  while another signal is stated — is substituted with `BLOCK`, and that
+  substitution governs the axes published beside it: `merge_recommendation`
+  reads `block` and `allow_merge` `false`, whatever the pack claimed. This is
+  the CLI's rule, applied here so the two readers cannot answer the same bytes
+  differently. `storage_corrupt` is reserved for a decision block stating NONE
+  of `verdict`, `merge_recommendation` and `allow_merge`; a signal that is
+  present but unrankable — including a lone `allow_merge` — is a decision the
+  pack gave, and it is normalized with a caveat rather than called corrupt.
+- `unreadable_<field>:` — the field was present with the wrong JSON type
+  (`merge_recommendation: 7`, `allow_merge: "false"`, `quality_pass: "false"`,
+  `analysis_status: 7`, `blocking_issues: "Clippy"`). Emitted for every axis in
+  the ranking table of `docs/contracts/merge_gate.md`. A wrongly typed field is
+  not an absent one: it is ignored for ranking, but it is named, and the
+  decision is normalized conservatively around it. The pack is
+  `storage_corrupt` only when no signal was stated at all.
+- `unknown_analysis_status:` — the field is a string outside
+  `complete` / `degraded` / `incomplete`. Like `unknown_merge_recommendation:`,
+  it cannot rank, so it is excluded from the reconciliation and named rather
+  than dropped in silence.
+- `schema_forward_compat:` — the pack's `schema_version` is a newer MINOR of a
+  known MAJOR; it is read, and fields this build does not know are ignored. An
+  unknown MAJOR is `storage_corrupt`, and so is a `schema_version` that is
+  present but not a `MAJOR.MINOR` string. A pack with no `schema_version` at all
+  is pre-2.1 and is accepted silently, like the `ALLOW`/`HOLD` tokens — including
+  the pre-2.1 shape that carries its signals at the root instead of under
+  `decision`. A pack that STATES a `schema_version` and still has no `decision`
+  object is `storage_corrupt`, and so is a schema-less pack whose root is not an
+  object at all (an array, a scalar, `null`) — that root states no decision, it
+  is not a decision missing every field. Both readers apply those rules from one
+  place (`gate::select_decision_object`), so a pack the CLI reads is never one
+  the MCP adapter calls corrupt.
 
 Completed response:
 
@@ -329,7 +379,7 @@ fields (e.g. `retry_after_ms`, `active_run_id`, `run_id`).
 | `artifact_missing` | The requested artifact does not exist within the run, is not UTF-8 text, or would escape the run directory. |
 | `tool_missing` | A required external tool is unavailable. |
 | `storage_locked` | Another review is already running for this repo branch. Carries `active_run_id` and `retry_after_ms`. |
-| `storage_corrupt` | `MERGE_GATE.json` is missing, invalid, has no recognizable decision, or an explicit `run_id` is ambiguous in storage. |
+| `storage_corrupt` | `MERGE_GATE.json` is missing, invalid, carries a `schema_version` with an unknown MAJOR, states no decision signal at all, or an explicit `run_id` is ambiguous in storage. |
 | `stale_run` | The run is still in progress or its process died before completing. Carries `retry_after_ms` while running. |
 
 ### Retrying

docs/usage.md

@@ -290,6 +290,39 @@ introduced.
 | `--no-color` | Disable ANSI colors |
 | `--no-zip` | Skip ZIP creation |
 | `--no-dashboard` | Skip HTML dashboard generation |
+| `--soft-exit` | Always exit 0, whatever the checks found |
+| `--fail-on-warnings` | With `--ci`: also exit 1 when any check reports warnings |
+
+### `--ci` exit codes
+
+`--ci` is the strict variant of the plain review run: it exits `1` on a `BLOCK`
+verdict or a broken quality gate, and `0` otherwise. Warning-level signals — a
+formatter delta, an unmaintained-crate advisory, lint warnings — are advisory:
+they keep the verdict at `CONDITIONAL` and surface as review caveats, but they
+do not fail the process. Add `--fail-on-warnings` to opt into exit `1` for them;
+the flag requires `--ci` and does not affect `prview gate`, whose exit codes come
+from the gate contract (see `docs/gate-playbook.md`).
+
+`--fail-on-warnings` counts the artifact pack's check list, not the CLI's own.
+The artifact run generates further checks — `public_api_diff`, `unsafe_audit`,
+`ghost_refs` and the synthetic `heuristics_loctree` — which reach
+`MERGE_GATE.json` and the dashboard but never the in-memory report the plain
+tally is built from. The `--json` summary states both numbers:
+`checks_summary.warned` is what the CLI ran, `checks_summary.warned_in_pack` is
+the complete count the flag keys off, and it is always the larger of the two.
+
+Strictness follows the `--ci` you typed, not the preset label the run reports.
+`--update` outranks `--ci` when the execution preset is resolved, so
+`prview --ci --fail-on-warnings --update` publishes `mode.execution_mode:
+"update"` — and reading strictness off that label made both `--ci` exits
+(`!quality_pass` and the warning hardening) silently inert for exactly the
+combination CI jobs use.
+
+An `--update` run that finds no new commits reuses the previous pack, and its
+exit code is derived from that pack like any other run's: a reused `BLOCK` or a
+reused warning under `--fail-on-warnings` exits non-zero rather than reporting a
+green second invocation over an artifact nothing re-checked. `--soft-exit`
+remains the one way to ask for `0` regardless.
 
 ## Examples
 
@@ -338,6 +371,15 @@ it carries the verdict, `output_dir`, a short `checks_summary`, `top_failures`,
 on disk, especially the canonical `RUN.json` and `MERGE_GATE.json` pair, plus
 `PR_REVIEW.md`.
 
+The verdict fields (`verdict`, `allow_merge`, `quality_pass`,
+`merge_recommendation`, `analysis_status`) are read from the run's
+`00_summary/MERGE_GATE.json` and from nowhere else. If that artifact is missing,
+unparsable, or stamped with a `schema_version` this build cannot read, prview
+reports an execution error and exits `3` instead of re-deriving a verdict —
+including on `--update` runs that re-read an earlier pack. A newer MINOR schema
+within a known MAJOR is accepted and reported through the optional `caveats`
+array, which also carries any verdict the reader had to normalize.
+
 ## Output
 
 Artifacts are written to `$PRVIEW_HOME/runs/<repo>/<branch>/<run_id>/`

src/artifacts/ai_index.rs

@@ -64,8 +64,10 @@ pub(crate) fn generate_ai_index(
     )?;
     writeln!(
         md,
-        "- Coverage signal: {}/{} changed code files ({}%)",
-        coverage.covered_count, coverage.total_source, coverage.pct
+        "- Coverage signal: {}/{} changed code files ({})",
+        coverage.covered_count,
+        coverage.total_source,
+        crate::artifacts::signal::format_coverage_pct(coverage.pct)
     )?;
     let gate_path = Path::new("00_summary/MERGE_GATE.json");
     if dir.join(gate_path).exists()

src/artifacts/dashboard/assets.rs

@@ -2759,8 +2759,13 @@ const JS_SUFFIX: &str = r##"
             }
 
             if (quality.coverage) {
-                var pct = Math.round(quality.coverage.heuristic_ratio * 100);
-                comment += '**Coverage heuristic:** ' + pct + '% (' + quality.coverage.matched + '/' + quality.coverage.total + ')\n\n';
+                // heuristic_ratio is null when nothing was measured (0 changed
+                // source files) - do not round null into a 0%/100% claim.
+                var ratio = quality.coverage.heuristic_ratio;
+                var covLabel = (ratio === null || ratio === undefined)
+                    ? 'not measured'
+                    : Math.round(ratio * 100) + '%';
+                comment += '**Coverage heuristic:** ' + covLabel + ' (' + quality.coverage.matched + '/' + quality.coverage.total + ')\n\n';
             }
 
             var hotspots = (diff.files || [])

src/artifacts/dashboard/mod.rs

@@ -557,14 +557,15 @@ fn build_html(input: BuildHtmlInput<'_>) -> String {
         )
     };
 
-    let coverage_summary = if ctx.coverage.total_source > 0 {
-        i18n_template(
+    let coverage_summary = match ctx.coverage.pct {
+        Some(pct) => i18n_template(
             "summary.coveragePct",
-            &format!("{}% coverage", ctx.coverage.pct),
-            &[("pct", ctx.coverage.pct.to_string())],
-        )
-    } else {
-        escape_html("N/A")
+            &format!("{}% coverage", pct),
+            &[("pct", pct.to_string())],
+        ),
+        // Nothing was measured (no changed source files) — say so, do not
+        // borrow a number the heuristic never produced.
+        None => escape_html("N/A"),
     };
 
     let commit_count = diff.map(|d| d.commits.len()).unwrap_or(0);
@@ -927,12 +928,12 @@ fn build_html(input: BuildHtmlInput<'_>) -> String {
             );
         }
     }
-    if ctx.coverage.total_source > 0 {
-        if ctx.coverage.pct < 80 {
+    if let Some(pct) = ctx.coverage.pct {
+        if pct < 80 {
             let _ = write!(
                 nav,
                 "<a href=\"#section-coverage\"><span data-i18n=\"nav.coverage\">Coverage</span> <span class=\"nav-badge badge-warn\">{}</span></a>",
-                ctx.coverage.pct
+                pct
             );
         } else {
             let _ = write!(

src/artifacts/dashboard/sections.rs

@@ -1027,18 +1027,21 @@ pub(super) fn build_action_center(
     }
 
     let cov = &ctx.coverage;
-    if cov.total_source > 0 || cov.non_code_count > 0 {
-        if cov.pct < 80 {
+    // `pct == None` means no changed source files were evaluated. A diff of
+    // pure non-code changes used to fall through here and emit a "Coverage:
+    // 100%" chip out of a 0/0 ratio — emit nothing instead.
+    if let Some(cov_pct) = cov.pct {
+        if cov_pct < 80 {
             push_card(
                 &mut cards,
                 "#section-coverage",
-                if cov.pct < 50 {
+                if cov_pct < 50 {
                     "alert-error"
                 } else {
                     "alert-warning"
                 },
                 r#"<span data-i18n="section.coverage">Coverage</span>"#.to_string(),
-                escape_html(&format!("{}%", cov.pct)),
+                escape_html(&format!("{}%", cov_pct)),
                 i18n_template(
                     "message.changedCodeWithoutMatchingTests",
                     "Changed code without matching tests",
@@ -1055,7 +1058,7 @@ pub(super) fn build_action_center(
                     i18n_template(
                         "chip.coverageOk",
                         "Coverage: {pct}%",
-                        &[("pct", cov.pct.to_string())],
+                        &[("pct", cov_pct.to_string())],
                     )
                 ),
             );
@@ -1974,13 +1977,14 @@ pub(super) fn build_breaking_section(ctx: &DashboardContext) -> String {
 
 pub(super) fn build_coverage_section(ctx: &DashboardContext) -> String {
     let cov = &ctx.coverage;
-    if cov.total_source == 0 {
+    // No changed source files => nothing measured => no coverage section at all.
+    let Some(cov_pct) = cov.pct else {
         return String::new();
-    }
+    };
 
     // Coverage % is a metric, not a verdict: keep color only for the negative
     // signal (below threshold = "what is wrong?"), neutralize the rest.
-    let pct_color = if cov.pct < 50 {
+    let pct_color = if cov_pct < 50 {
         "var(--warn)"
     } else {
         "var(--fg)"
@@ -2049,7 +2053,7 @@ pub(super) fn build_coverage_section(ctx: &DashboardContext) -> String {
     </div>
 </div>"#,
         color = pct_color,
-        pct = cov.pct,
+        pct = cov_pct,
         coverage_detail = i18n_template(
             "message.coverageDetail",
             &format!(

src/artifacts/dashboard/tests.rs

@@ -123,7 +123,7 @@ fn mock_ctx() -> DashboardContext {
         check_gates: vec![],
         breaking: vec![],
         coverage: super::super::CoverageDelta {
-            pct: 100,
+            pct: Some(100),
             total_source: 1,
             covered_count: 1,
             uncovered: vec![],
@@ -372,7 +372,7 @@ fn test_header_merge_chip_shows_review_caveat() {
         risk_level: BreakingRisk::High,
     }];
     ctx.coverage = super::super::CoverageDelta {
-        pct: 11,
+        pct: Some(11),
         total_source: 43,
         covered_count: 5,
         uncovered: vec![],
@@ -509,7 +509,7 @@ fn test_merge_decision_card_review_caveats() {
         risk_level: BreakingRisk::High,
     }];
     ctx.coverage = super::super::CoverageDelta {
-        pct: 11,
+        pct: Some(11),
         total_source: 43,
         covered_count: 5,
         uncovered: vec![],

src/artifacts/dashboard/trends_tests.rs

@@ -95,7 +95,7 @@ fn mock_ctx() -> DashboardContext {
         check_gates: vec![],
         breaking: vec![],
         coverage: super::super::CoverageDelta {
-            pct: 100,
+            pct: Some(100),
             total_source: 1,
             covered_count: 1,
             uncovered: vec![],

src/artifacts/merge_gate.rs

@@ -245,7 +245,7 @@ pub(super) fn generate_merge_gate(input: MergeGateInput<'_>) -> Result<()> {
         .count();
 
     let gate = json!({
-        "schema_version": "2.1",
+        "schema_version": crate::gate::MERGE_GATE_SCHEMA_VERSION,
         "generated_at": chrono::Local::now().to_rfc3339(),
         "bridge_stage": config.bridge_stage,
         "target": resolved_target.name,
@@ -285,6 +285,7 @@ pub(super) fn generate_merge_gate(input: MergeGateInput<'_>) -> Result<()> {
             "quality_failure_details": quality_failures.details.iter().map(|detail| json!({
                 "name": detail.name,
                 "classification": detail.classification.as_str(),
+                "origin": detail.origin.as_str(),
             })).collect::<Vec<_>>(),
             "decision_reason": decision.reason,
             "review_caveats": all_review_caveats,
@@ -500,7 +501,7 @@ mod tests {
         CoverageDelta {
             total_source: 0,
             covered_count: 0,
-            pct: 0,
+            pct: None,
             uncovered: Vec::new(),
             covered: Vec::new(),
             non_code_count: 0,
@@ -1132,6 +1133,45 @@ mod tests {
         );
     }
 
+    #[test]
+    fn preexisting_only_rustfmt_keeps_strict_gate_exit_zero() {
+        // Regression guard for the warning→failure cut: the gate exit contract is
+        // unchanged. The pre-existing-only rustfmt pack is a PASS, and `prview
+        // gate --strict` must still exit 0 on it — the same artifact the adapter
+        // in `gate.rs` reads, run through the same verdict → exit mapping.
+        use crate::gate::{GateVerdict, gate_exit_code};
+
+        let gate = run_gate_with_rustfmt_warning(false);
+        let verdict = GateVerdict::try_from(
+            gate["decision"]["verdict"]
+                .as_str()
+                .expect("verdict is a string"),
+        )
+        .expect("verdict is contract vocabulary");
+
+        assert_eq!(verdict, GateVerdict::Pass);
+        assert_eq!(gate_exit_code(verdict, true), 0);
+        assert_eq!(gate_exit_code(verdict, false), 0);
+    }
+
+    #[test]
+    fn introduced_warning_keeps_strict_gate_exit_two() {
+        // The other half of the contract: an in-diff warning stays CONDITIONAL,
+        // so `--strict` still exits 2. Warnings became honest, not toothless.
+        use crate::gate::{GateVerdict, gate_exit_code};
+
+        let gate = run_gate_with_rustfmt_warning(true);
+        let verdict = GateVerdict::try_from(
+            gate["decision"]["verdict"]
+                .as_str()
+                .expect("verdict is a string"),
+        )
+        .expect("verdict is contract vocabulary");
+
+        assert_eq!(verdict, GateVerdict::Conditional);
+        assert_eq!(gate_exit_code(verdict, true), 2);
+    }
+
     #[test]
     fn introduced_rustfmt_warning_in_diff_is_not_downgraded() {
         // In-diff formatting warnings belong to the change: no downgrade.