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

[m-szymanska]

Verdict & signal truth: stop fabricating findings, stop hiding real ones

Part 2 of the 2-PR campaign (part 1: #20, substrate truth). Five surgical cuts, each independently falsified before merge, all attacking the same failure family: prview's signals lied in both directions — phantom findings that erode trust, and real signals silently suppressed.

The cuts

1. Pattern scan word boundaries (fix/pattern-word-boundaries): TODO/FIXME/HACK/XXX matched inside identifiers and words (mastodon → TODO). Plain-word needles now require word boundaries; 9 regression tests.
2. Phantom removed symbols for non-fn types (fix/breaking-phantom-nonfn): remove+re-add pairing only worked for pub fn (three independent fn-only gates, plus a drifted duplicate of PUB_SYMBOL_TYPES). An unchanged pub struct moved within a file was reported as a breaking removal. Pairing now covers all tracked symbol kinds; identical re-adds are silenced, changed declarations become ChangedSignature. Genuine removals stay reported (guard test).
3. Per-hit test context in perf detection (fix/perf-test-context-per-hit): one #[cfg(test)] anywhere in a hunk classified all hits as test-only, dropping production hot-path signals from perf_regression_suspected and the risk score (the historical argon2 shape). Context now opens at its marker and closes on brace balance, per hit line; every ambiguity resolves toward production.
4. Semgrep tool errors are not code regressions (fix/semgrep-exit2-tool-error): exit ≠ 0 without a findings payload (config error, exit 2) was Failed — indistinguishable from real findings. Now Skipped with an explicit reason (exit code + stderr excerpt). Fail-closed is preserved: at Block severity the policy engine treats it exactly like a missing tool.
5. Skip-as-zero residuals (fix/skip-as-zero-residual): a 0/0 coverage scan reported 100%; skipped heuristics serialized as zeros indistinguishable from a clean scan. Coverage is now Option end-to-end (not measured, card/chip omitted), report.json carries measured/not_measured_reason and heuristics status/skip_reason/total_files, with counters omitted on skip. A real 0/N stays a genuine 0%. Verified E2E on sandbox repos with a release build (non-code diff vs. code-without-tests diff).

Verification

• Full gates on the integrated branch: 1239 lib + 81 integration tests, 0 failures; clippy -D warnings clean; fmt clean. json_contract 18/18, mcp_contract 24/24 — no consumer contract broken (report.json change is additive; heuristic_ratio may now be null, called out in the CHANGELOG).
• Each branch report documents its falsification run (test fails on reverted logic / old semantics scaffold).

Scope notes

• pub use re-export detection deliberately not added — it was never a tracked symbol type, so there was no phantom to fix; adding detection is a new (noisy) finding class. A contract test pins this so it can't sneak back in as a phantom.
• The deeper verdict-pipeline redesign (advisory→failure conflation, semantic invariants, completeness/confidence from the PR #46 audit) is a separate decision-core line landing next — this PR is the signal-level groundwork.

Follow-up cuts (pushed after the initial review round)

6. Warnings are not failures (fix/warning-not-failure): the P0 advisory→failure conflation from the PR #46 audit. QualityFailureOrigin { Failure, Warning } separates the two classes; has_new_failures counts only failure-origin entries, so a Warnings-status baseline check without located findings can no longer flip quality_pass=false and fabricate "N quality checks failed". --ci gets an explicit --fail-on-warnings escape hatch (requires --ci; outside CI the flag never hardens a local run). Pre-existing-findings degradation is untouched — both contract tests pass unchanged. Runtime proof on a real repo: semgrep PartialParsing warnings → quality_pass=true, exit 0, zero "failed" wording in the pack.
7. Readers stop guessing (fix/read-hardening): killed the fourth parallel verdict derivation (fallback_merge_gate_summary re-deriving allow_merge from the recommendation). build_cli_json_summary is now fail-loud: an unreadable/absent MERGE_GATE.json is an execution error (exit 3), not an invented verdict; unknown verdict/recommendation values surface as explicit caveats instead of being silently dropped; schema mismatches follow the MAJOR/MINOR convention documented in docs/contracts/merge_gate.md.
8. Review followup (fix/review-followup-signal): all 6 bot review threads addressed with verify-first regression tests (each new test fails on the pre-fix code) — removed-line handling in the perf test-context tracker (2 threads), context-matched proximity pairing, kind-aware signature grouping, module-scope pairing, and multi-line non-fn declaration accumulation (2× P1). Details in the thread replies.

Integrated gates after all follow-ups: 1353 tests, 0 failures; clippy -D warnings clean; fmt clean.

9. Review followup round 2 (fix/review-followup-signal-2): strict merge-gate schema parsing aligned with tools/validate_merge_gate.py (exact known set, fail-loud on malformed versions); trailing // comments no longer open test context or shift brace depth in the perf tracker; forward-schema MCP reads marked normalized; non-string schema_version values rejected instead of silently read as legacy; cross-hunk module scope deferred with a measured basis (173-commit corpus: the proposed rule would resurrect 4 phantom removals and lose every genuine signature change — recorded in the scopes_may_pair doc comment).

⚠️ Contract changes (consumer-visible, deliberate)

• MERGE_GATE.json schema 2.1 → 2.2 (additive MINOR): quality_failure_details[] entries carry "origin": "failure" | "warning"; only origin: failure may fail the quality gate. Allowlisted in the validator, documented in docs/contracts/merge_gate.md.
• report.json schema 1.0 → 2.0 (MAJOR): heuristic_ratio became nullable and the loctree counters became omittable — a 1.0 decoder stops parsing some packs, and this repo defines MINOR as "unknown fields ignored". Stamping this MINOR would repeat the "0/0 is 100%" lie at the schema level. No in-repo reader consumes the stamp; the bump is declarative for external consumers.

Final integrated gates: 1363 tests, 0 failures; clippy -D warnings clean; fmt clean. Live pack verified: validate_merge_gate.py → OK, schema 2.2, a warning-origin semgrep entry rendered alongside quality_pass: true.

10. Review followup round 3 (fix/review-followup-signal-3): duplicate public declarations pair one-to-one (additions are consumed; two-pass — identical matches first, then changed signatures); braces inside string/char literals no longer close the perf tracker's test scope (raw/byte strings and '{' covered, per-line best-effort named); schema versions must be canonically spelled (02.02/+2.2 no longer read as 2.2), matching the validator's exact strings; the validator itself requires a usable origin on every quality_failure_details entry at schema ≥ 2.2 (5-shape mutation test in the harness); wrongly typed MCP decision signals (including allow_merge) are named in caveats instead of silently dropped.

11. Versioned packs must carry a decision object (fix/review-followup-signal-4): read_merge_gate_summary's root-as-decision fallback is narrowed to unversioned legacy packs only — a pack that declares schema_version without a decision object is a corrupt artifact (exit 3), matching the validator and the MCP reader. Contract tests pin both directions; docs/contracts/merge_gate.md states the rule.

Final integrated gates: 1374 tests, 0 failures; clippy -D warnings clean; fmt clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr

[claude]

⚠️ Code review skipped — your organization has reached its monthly code review spending cap.

An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.

Once the cap resets or is raised, reopen this pull request to trigger a review.

[Copilot]

Pull request overview

This PR improves the accuracy and trustworthiness of prview’s reviewer-facing signals by eliminating known sources of phantom findings and by ensuring real findings aren’t silently suppressed. It tightens pattern matching, corrects breaking-change pairing semantics across more symbol kinds, refines perf test-context classification, distinguishes semgrep tool/config errors from code regressions, and prevents “0/0” coverage/heuristics from being rendered as a perfect/clean result.

Changes:

• Make multiple signals “truth-preserving”: fix substring false positives in pattern scan, fix phantom breaking removals for non-fn public items, and classify perf hits’ inline Rust test context per-hit (not per-hunk).
• Treat semgrep non-zero exits without a findings payload as tool/config errors (Skipped with an actionable reason) rather than code failures.
• Make coverage/heuristics “skip vs measured” explicit end-to-end (Option-based coverage pct, report/dashboard rendering changes, and contract/test updates).

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/regression/perf.rs	Refines perf regression detection to classify inline Rust test context per hit line and track mixed/prod/test proximity hits.
src/checks/semgrep.rs	Distinguishes semgrep tool/config failures from genuine code findings; formats skip reasons for policy plumbing.
src/artifacts/verdict.rs	Updates coverage caveat logic to handle optional coverage percent (unmeasured vs low coverage).
src/artifacts/tests.rs	Adjusts artifact tests for coverage pct becoming optional and adds regression guards for “0/0 not measured”.
src/artifacts/signal/risk.rs	Updates risk-signal test scaffolding for optional coverage pct.
src/artifacts/signal/patterns.rs	Adds word-boundary matching for plain-word needles to prevent substring false positives; adds regression tests.
src/artifacts/signal/coverage.rs	Makes coverage percent optional to prevent 0/0 rendering as 100%; adds shared formatter and tests.
src/artifacts/signal/breaking.rs	Expands remove+re-add pairing to all tracked public symbol kinds; adds regression tests and docs alignment.
src/artifacts/report.rs	Makes report.json reflect “measured vs skipped” for heuristics and “measured vs not measured” for coverage, avoiding skip-as-zero.
src/artifacts/pr_review.rs	Updates PR review warnings to handle optional coverage percent correctly.
src/artifacts/merge_gate.rs	Updates merge-gate test scaffolding for optional coverage percent.
src/artifacts/dashboard/trends_tests.rs	Adjusts dashboard trend tests for optional coverage percent.
src/artifacts/dashboard/tests.rs	Adjusts dashboard rendering tests for optional coverage percent.
src/artifacts/dashboard/sections.rs	Omits coverage chips/sections when coverage is unmeasured (no pct), preventing fabricated 100% UI.
src/artifacts/dashboard/mod.rs	Updates dashboard summary/nav rendering for optional coverage percent.
src/artifacts/dashboard/assets.rs	Updates PR-comment generator JS to render null coverage ratios as “not measured”.
src/artifacts/ai_index.rs	Uses shared optional-coverage formatter so AI index never reports 0/0 as 100%.
docs/architecture.md	Documents new breaking/coverage/heuristics semantics (pairing, relocated symbols, and “not measured” coverage).
CHANGELOG.md	Records signal-truth fixes and the additive report.json schema update (nullable heuristic_ratio).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1f0af7243

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/artifacts/signal/patterns.rs:86

• contains_word_bounded advances search_from = start + 1 and then slices haystack[search_from..]. Because start is a byte offset, start + 1 can land in the middle of a UTF-8 codepoint, which will panic at runtime when slicing. Consider iterating with haystack.match_indices(needle) (or doing the search on haystack.as_bytes()), so all offsets remain valid UTF-8 boundaries.

    let hbytes = haystack.as_bytes();
    let mut search_from = 0;
    while let Some(rel) = haystack[search_from..].find(needle) {
        let start = search_from + rel;
        let end = start + needle.len();

        let left_ok = start == 0 || !is_word_byte(hbytes[start - 1]);
        let right_ok = !needs_right_boundary || end == hbytes.len() || !is_word_byte(hbytes[end]);

        if left_ok && right_ok {
            return true;
        }
        search_from = start + 1;
    }

src/regression/perf.rs:403

• added_line_test_context only treats //... as a comment. A block comment like /* #[cfg(test)] */ still matches INLINE_RUST_TEST_CONTEXT_PATTERN (it searches substrings), which can incorrectly open test context and potentially mute/shift subsequent production hits. If the intent is “commented-out markers do not open it”, block comments likely need to be excluded (e.g., track in_block_comment and skip marker detection while inside it, or at least ignore lines starting with /*).

        // Comments (including doc comments) never open test context — a doc
        // comment mentioning `#[cfg(test)]` must not mute a production hit.
        if trimmed.starts_with("//") {
            if is_added {
                flags.push(in_test);
            }
            continue;
        }

        // Only the outermost marker opens the context, so nested `#[test]`
        // attributes do not reset the enclosing `mod tests` brace tracking.
        if !in_test && INLINE_RUST_TEST_CONTEXT_PATTERN.is_match(trimmed) {
            in_test = true;
            depth = 0;
            seen_open = false;
        }

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d94ebdc143

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tools/validate_merge_gate.py:39

• schema_at_least() uses str.isdigit(), which accepts non-ASCII Unicode digits (e.g. "٢.٢"). That would make tools/validate_merge_gate.py treat such a schema_version as canonical and >= 2.2, while the Rust reader rejects it (gate::canonical_u32 requires ASCII digits via is_ascii_digit()), reintroducing validator/reader drift. Consider enforcing ASCII digits explicitly (e.g. p.isascii() and p.isdigit()), or use a [0-9]+ check for each component.

[chatgpt-codex-connector]

💡 Codex Review

prview-rs/src/regression/perf.rs

Line 69 in f8f77bb

r"#\[\s*(?:cfg\s*\([^]]*\btest\b[^]]*\)|(?:[\w:]+::)*test|rstest)\s*\]|\bmod\s+tests\b",

Do not treat negated test cfgs as test scope

This predicate matches any cfg(...) containing the word test, including production-only guards such as #[cfg(not(test))] and mixed guards such as #[cfg(any(test, feature = "bench"))]. Added loops and queries under those guards are therefore classified as test-only and omitted from perf_regression_suspected and the downstream risk score even though they compile in non-test builds. Recognize cfg semantics rather than the presence of the token alone, and add a negated-cfg regression test.

AGENTS.md reference: AGENTS.md:L47-L47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4dc55cfa05

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve cfg_attr-based configuration guards

When an unchanged non-function declaration is re-emitted while its guard changes from #[cfg_attr(feature = "a", cfg(unix))] to the Windows equivalent, this filter discards both attributes because it recognizes only literal #[cfg(. Both declarations therefore receive cfg_guard: None, exact pairing consumes the addition, and the removal finding disappears even though Config is no longer available on Unix when feature a is enabled. Include cfg_attr forms capable of applying cfg in the guard identity and add a regression test for this pairing case.

AGENTS.md reference: AGENTS.md:L47-L47

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Serialize quality-failure origins in report.json

This new field distinguishes advisory Warnings entries from actual failed checks, but build_report still maps each detail into GateQualityFailureDetail using only name and classification (src/artifacts/report.rs:692-698). For a warning such as an introduced Rustfmt delta, report.json therefore contains introduced_quality_failures alongside quality_pass: true without the origin needed to explain that state, even though the parallel MERGE_GATE.json entry is now unambiguous. Propagate origin into the report payload and document/test that contract addition.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep line comments from swallowing declaration tails

Fresh evidence after the literal/comment scanner fix is that joining physical declaration lines with spaces changes // semantics. If a moved or re-emitted multiline public declaration has a parameter line ending in // comment, declaration_complete treats every subsequently appended line as part of that comment, never sees the closing ) or {, and absorbs body lines until finalization; a body-only rewrite can then be emitted as a phantom ChangedSignature. Preserve physical line-comment boundaries or scan each continuation line with state, and cover a multiline signature containing a trailing comment.

AGENTS.md reference: AGENTS.md:L47-L47

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a6848e820e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Compare public bodies before canceling non-function removals

When a body-bearing declaration is fully removed and re-added with the same opener, this exact-match pass drops the removal even though SymbolDecl.text ends at the opening { and excludes the body. For example, moving or re-emitting pub enum Mode while replacing variant Old with New produces identical declaration texts, and no other scanner records the removed variant, so a real downstream API break disappears entirely; public struct-field and trait-item changes have the same failure mode. Compare the relevant public body or avoid exact-canceling body-bearing declarations when their bodies differ, with a regression test for an enum variant removal.

AGENTS.md reference: AGENTS.md:L47-L47

Useful? React with 👍 / 👎.