Skip to content

fix: signal truth round — merge-base diffs, remote-target parity, cache/storage/artifact consistency#14

Merged
m-szymanska merged 27 commits into
mainfrom
fix/signal-truth
Jul 8, 2026
Merged

fix: signal truth round — merge-base diffs, remote-target parity, cache/storage/artifact consistency#14
m-szymanska merged 27 commits into
mainfrom
fix/signal-truth

Conversation

@m-szymanska

Copy link
Copy Markdown
Contributor

Summary

Signal-truth round: fixes for every confirmed correctness finding from the 2026-07-07 deep-dig audit (69-agent adversarially-verified sweep — 15 findings confirmed by 3-vote quorum, 13 of them land here; the 2 packaging ones ship in a follow-up PR). One theme: prview must never report a weaker or fabricated signal — diffs, checks, cache, storage, MCP and artifacts now tell the same truth.

Merge-base diffs (A1)

  • e309841 fix(git): diff artifacts from merge-base — the artifact pipeline (run(), run_quick(), TUI) diffed two-dot from the base TIP; when the base branch advanced after fork, packs contained foreign commits as reversed hunks. Now diffs from the actual merge-base (three-dot model), consistent with scope/ and the semgrep baseline, with fallback for unrelated histories.
  • 290bdd2 fix(report): populate range merge_base from diff base — range.merge_base in report.json carried the base TIP, never the common ancestor.

Remote-target parity (A2) — P1

  • 7d32744 fix(checks): run ruff/mypy/js checks against fetched target in remote mode — in --pr/--remote, Ruff/Mypy/JS checks scanned the local working tree while the gate claimed to describe the fetched head. They now materialize an ephemeral worktree for target.commit_id (extracted WorktreeSnapshot into src/git/worktree.rs, same mechanism semgrep already used). Verified with a live probe: clean local tree + violation on fetched target → check reports the violation.

Check & cache truth (A3)

  • 40c8d5a fix(cache): key rust checks by cargo_root manifest set — stale Passed served after dependency bumps in workspaces where cargo_root != repo_root (bug #22 was fixed for cargo-audit only; now covers check/clippy/rustfmt/geiger).
  • 13665e5 fix(checks): distinguish rustfmt missing from formatting diff — missing rustfmt component reported as "formatting needed".
  • 9309fbc fix(cache): escape repo path in glob patterns — paths with glob metacharacters produced a constant empty hash (fail-open cache).
  • aca3e1c fix(checks): surface cargo audit informational warnings.
  • 48d4ff2 fix(cache): widen cache key hash to 16 bytes.

Storage & MCP truth (A4)

  • 8534fe0 fix(storage): skip corrupt index lines instead of truncating — one bad line silently dropped every later run and the next save made the loss permanent.
  • 4f81935 fix(mcp): surface in-flight runs in verdict without run_id.
  • b947aed fix(mcp): fail loud on unreadable MERGE_GATE in quick path — was returning completed/UNKNOWN with empty caveats on corrupt storage.
  • f22bcb7 fix(mcp): treat pid 0 as dead in liveness checks (immortal RUNNING lock).
  • 32c0342 fix(storage): fsync before rename in index save.
  • ca94912 fix(storage): add age signal to stale-lock detection (PID-reuse hazard).
  • 5daa417 fix(mcp): serialize run activation to close R2b TOCTOU (O_EXCL lock).

Artifact & output consistency (A5)

  • d85cf35 fix(artifacts): keep report.json verdict in sync with merge gate — report.json could carry a weaker verdict than MERGE_GATE.json on the same run.
  • 8919afd fix(signal): require module match in coverage stem strategy — same-stem test files from unrelated modules counted as high-confidence coverage.
  • 1846d6d fix(signal): use identifier-boundary match for orphaned resources — "dropdown" no longer triggers an orphaned "drop" finding.
  • 5849a91 fix(output): clarify summary when failures degraded to advisory — no more red "Some checks failed" above a green PASS verdict.
  • db8fa20 fix(output): handle analysis_status=incomplete explicitly.

Test plan

  • cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test green after every cut (final: 1260+ passed / 0 failed; tests/mcp_contract.rs 24/24, no flake).
  • Every fix ships with a red-before/green-after regression test.
  • Live probes beyond unit tests: diverged-base fixture (pack contains only own files), remote-mode Ruff against fetched target, cargo_root cache-key fixture, wire-level MCP stdio JSON on corrupt gate, prview --quick three-surface consistency (console / report.json / MERGE_GATE.json).

🤖 Generated with Claude Code

vetcoders and others added 20 commits July 6, 2026 20:00
- Add a single Repository helper that resolves diff bases through repo.merge_base().

- Use that helper before generate_diffs in standard, quick, and TUI paths.

- Preserve base display names while falling back to base tips for unrelated histories.

- Cover the advanced-base fixture where only target-owned files remain in the diff.
- Derive report.json meta.range.merge_base from the Diff base commit actually used.

- Add an artifact run verifier for an advanced-base branch where only own.rs belongs in the pack.

- Assert the Semgrep baseline helper and artifact diff base resolve to the same merge-base.

- Keep base/head display refs unchanged while reporting the true three-dot anchor.
- Hash Cargo Check, Clippy, Rustfmt, and Geiger from the configured cargo_root.

- Share the cargo_root fallback used by Cargo-based checks.

- Add member-lock cache-key regression tests for the rust check family.
- Classify missing rustfmt/cargo-fmt as Skipped instead of formatting warnings.

- Keep formatting diffs as Warnings and unexpected tool crashes as Error.

- Add a regression test for the missing-component signature.
- Escape metacharacters in the absolute cache-hash root before building glob patterns.

- Sort matched paths before hashing for deterministic traversal.

- Add a regression test for repo roots containing bracket characters.
- Check cargo-audit warnings before treating exit 0 as Passed.

- Count structured JSON warning sections and warning-line output.

- Add a regression test for advisory warnings with a clean vulnerability report.
- Expand cache digest segments from 32-bit to 128-bit prefixes.

- Add coverage for the new digest segment length.

- Update the architecture note for Rust cache key generation.
Index load used `.map_while(Result::ok)`, which stops at the first line
`BufRead::lines` returns an `Err` for (e.g. non-UTF-8). Every later run
vanished from the view and the next `register_and_prune` save persisted
that loss — permanent data loss from one bad byte. Skip only the bad line
(with a warn) and keep the rest.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`resolve_run` with no run_id was index-only, so an in-flight deep run
(registered only on completion) was invisible — the client got a silent
`run_not_found` while a review was actively producing its pack. Before
failing loud, scan the on-disk tree for a marker-bearing run matching HEAD,
mirroring the `state` tool's running-run scan so the two surfaces agree.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The quick-path `completed_body` silently substituted verdict=UNKNOWN,
blocking=[], caveats=[] and returned status=completed when `read_decision`
errored — an "empty success" the MCP contract forbids, and a state the
`verdict` tool rejects on the identical run. Propagate the `storage_corrupt`
error instead, so both surfaces agree.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pid 0 is the unknown-pid marker sentinel, and `kill(0, 0)` targets the
caller's whole process group — always succeeding, which made a pid-0
RUNNING.json marker an immortal "running" and left the R2b lock held
forever. Special-case pid 0 as dead so the marker reads as stale.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`save`/`save_to` flushed the tmp file and renamed it without an fsync, so a
power loss could leave a renamed-but-empty index. fsync the data before the
rename publishes it, and (unix) fsync the parent directory so the rename
itself is durable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stale detection keyed only on PID liveness, so a recycled PID kept an
abandoned lock alive forever. Parse the lock token's creation timestamp and
treat any lock older than one hour (far above a quick review's ~120s budget)
as stale even when some process now owns its recorded pid.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The R2b "one active run per branch" rule was a check-then-act: two
concurrent `start` calls could both see no active run and both proceed.
Serialize activation behind a per-branch O_EXCL lock file (reusing the
storage lock's atomic create-new + stale/PID-recycling handling), held
until the run's marker is on disk, closing the check-to-marker window.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Share blocking issue recording so context and merge gate bump the same merge axis.

- Route inline finding gate handling through the shared helper.

- Cover inline Block parity between report.json dashboard context and MERGE_GATE.json.
- Downgrade stem-only matches across different modules to low confidence.

- Keep same-module stem and mirrored path matches high confidence.

- Add regression for src/a/util.rs versus tests/b/util_test.rs.
- Replace substring delete-indicator matching with token-boundary matching.

- Preserve delete_user-style matches while rejecting dropdown/drop false positives.

- Add regression for dropdown.rs-style service names.
- Read the merge gate once before printing the human summary.

- Replace the raw failed-check heading with an advisory/pre-existing heading when the gate verdict is PASS.

- Add a regression for degraded raw failures that should not contradict PASS.
- Preserve explicit incomplete analysis status when reading MERGE_GATE.json.

- Keep legacy fallback only for missing or unknown analysis status values.

- Add a regression for edited gate JSON that combines PASS with incomplete.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several robustness and consistency improvements, including anchoring target diffs at the merge-base, executing checks on ephemeral git worktrees in remote mode, serializing concurrent review starts with a per-branch activation lock, and preventing index data loss by skipping unreadable lines. Feedback on these changes suggests refactoring the check execution pipeline to share a single WorktreeSnapshot across checks to avoid redundant worktree creation and lock contention. Additionally, it is recommended to use nested if let statements instead of the unstable let_chains feature for better Rust version compatibility, and to replace the custom glob escaping logic with the standard glob::Pattern::escape function.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/checks/mod.rs
Comment on lines +841 to +873
pub fn plan_check_run(config: &Config) -> Result<CheckPlan> {
let repo_root = config.repo_root.clone();
let repo = match crate::git::Repository::open(&repo_root) {
Ok(repo) => repo,
Err(_) => {
return Ok(CheckPlan {
scan_dir: repo_root,
_snapshot: None,
});
}
};

let (Ok(target), Ok(head)) = (repo.resolve_target(config), repo.head_commit_id()) else {
return Ok(CheckPlan {
scan_dir: repo_root,
_snapshot: None,
});
};

if head == target.commit_id {
return Ok(CheckPlan {
scan_dir: repo_root,
_snapshot: None,
});
}

// Ephemeral worktree
let snapshot = crate::git::create_worktree_snapshot(&repo_root, &target.commit_id)?;
Ok(CheckPlan {
scan_dir: snapshot.worktree_path.clone(),
_snapshot: Some(snapshot),
})
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Calling plan_check_run inside the run method of each individual check (Ruff, Mypy, TypeScript, ESLint, Vitest, Stylelint) means that a separate git worktree is created, checked out, and destroyed for every single check.

This introduces significant performance and reliability issues:

  1. Disk and CPU Overhead: Checking out the entire repository multiple times concurrently is extremely slow and I/O heavy.
  2. Git Lock Contention: Running git worktree remove and git worktree prune concurrently from multiple checks can cause race conditions and flaky failures due to Git index/metadata lock contention.

Recommendation:
In line with orchestrating functions directly populating provenance metadata (such as analysis paths or roots) rather than relying on individual callers to redundantly populate or re-assert these values, consider refactoring the check execution pipeline (e.g., in App::run) to create a single shared WorktreeSnapshot once, and pass its path to all checks.

References
  1. Orchestrating functions should directly populate provenance metadata (such as analysis paths or roots) rather than relying on individual callers to redundantly populate or re-assert these values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ca29088. In remote/PR mode each snapshot-backed check (Ruff, Mypy, TypeScript, ESLint, Vitest, Stylelint) now reuses ONE shared target-commit worktree materialised once per run in run_all / run_all_with_events, rather than each plan_check_run creating its own. The dispatcher owns the snapshot and keeps it alive until every check finishes via a new Config::scan_dir_override; on snapshot-creation failure the override stays unset and each check falls back to its own plan, preserving the original behaviour. Tests prove N checks reuse the override with zero additional worktrees and that a cargo-only run stays a no-op.

Comment thread src/storage/mod.rs
Comment on lines +545 to +549
if let Some(dir) = path.parent()
&& let Ok(dirf) = fs::File::open(dir)
{
let _ = dirf.sync_all();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The let_chains feature (using && inside if let) was only stabilized in Rust 1.88.0 and is restricted to the 2024 edition. To ensure compatibility with older compiler versions and the 2021 edition, it is safer to use nested if let statements here.

Suggested change
if let Some(dir) = path.parent()
&& let Ok(dirf) = fs::File::open(dir)
{
let _ = dirf.sync_all();
}
if let Some(dir) = path.parent() {
if let Ok(dirf) = fs::File::open(dir) {
let _ = dirf.sync_all();
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changing this one. The crate is on edition = "2024" (Cargo.toml), which already requires Rust 1.85+, and let-chains are stable as of 1.88 — CI builds green on this toolchain, and the companion PR #15 declares rust-version = "1.93.0". if let ... && let ... is the established pattern across the codebase (e.g. src/lib.rs, src/artifacts/pr_review.rs, src/artifacts/history.rs), so this site is consistent with the rest of the code; rewriting it to nested if let would trip clippy::collapsible_if under our -D warnings gate.

Comment thread src/cache/mod.rs Outdated

fn hash_files(repo_root: &Path, patterns: &[&str]) -> String {
let mut hasher = Sha256::new();
let escaped_root = escape_glob_literal(&repo_root.display().to_string());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Instead of implementing a custom escape_glob_literal function, you can use the standard glob::Pattern::escape function provided by the glob crate. This is more robust, idiomatic, and reduces custom code maintenance.

Suggested change
let escaped_root = escape_glob_literal(&repo_root.display().to_string());
let escaped_root = glob::Pattern::escape(&repo_root.display().to_string());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 698f57c. Replaced the hand-rolled escape_glob_literal with glob::Pattern::escape (glob = "0.3" is a direct dependency). Verified against the glob 0.3.3 source that Pattern::escape covers ?, *, [, ] and that {/} are literal to the crate (no brace expansion), so no case is weakened; the existing escaping test still passes.

Comment thread src/cache/mod.rs Outdated
Comment on lines 180 to 194
fn escape_glob_literal(path: &str) -> String {
let mut escaped = String::with_capacity(path.len());
for ch in path.chars() {
match ch {
'*' => escaped.push_str("[*]"),
'?' => escaped.push_str("[?]"),
'[' => escaped.push_str("[[]"),
']' => escaped.push_str("[]]"),
'{' => escaped.push_str("[{]"),
'}' => escaped.push_str("[}]"),
_ => escaped.push(ch),
}
}
escaped
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This custom escaping function can be completely removed if you use the standard glob::Pattern::escape function from the glob crate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 698f57c together with the sibling comment above — a single change replacing escape_glob_literal with glob::Pattern::escape, so the custom function is gone entirely.

@github-advanced-security

Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db8fa20df7

ℹ️ 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".

Comment thread src/checks/mod.rs
Comment on lines +868 to +871
let snapshot = crate::git::create_worktree_snapshot(&repo_root, &target.commit_id)?;
Ok(CheckPlan {
scan_dir: snapshot.worktree_path.clone(),
_snapshot: Some(snapshot),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Trust new snapshot-backed lint checks

When this worktree path is taken for --pr/--remote, Ruff/ESLint/Stylelint now scan the fetched target snapshot, but the artifact downgrade logic still treats only semgrep_scan as snapshot-backed (CleanComparison::check_scans_target_snapshot). In remote runs with a pre-existing out-of-diff Ruff/ESLint/Stylelint finding on the target, that finding remains unclassified and can still block the merge even though it now genuinely predates the PR diff. Please update the clean-comparison trust list (or carry this plan information into it) when enabling snapshot execution for these baseline-signal checks.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 3574ed7. check_scans_target_snapshot now returns true for ruff, eslint and stylelint alongside semgrep_scan, since after the remote-target parity change those linters run through plan_check_run and scan the target snapshot in remote mode. rustfmt and cargo_audit stay excluded because they scan the local checkout, not the snapshot. The downgrade test asserts the four snapshot-backed ids apply and the two local-checkout ids do not.

Comment thread src/lib.rs
Comment on lines +146 to +151
let diff_bases = self
.repo
.resolve_diff_bases(&target, &bases, self.config.quiet);
let diffs = self
.repo
.generate_diffs(&target, &bases, self.config.quiet)?;
.generate_diffs(&target, &diff_bases, self.config.quiet)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Compare heuristic regression from the merge base

This switches the artifact diff to the merge-base, but in remote/deep/CI runs run_heuristics_with_snapshots(&target, &bases) still snapshots bases.first().commit_id (the base tip) for regression. When the base branch has advanced with unrelated changes, the patch/report exclude those base-only files while the heuristic delta is computed against them, producing fabricated regressions or caveats. Pass the same diff_bases/merge-base commit into the snapshot regression path so all signals use the same range.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 375ea35. run_heuristics_with_snapshots is now passed diff_bases (the merge base resolved a few lines above) instead of bases (the base tip), so the heuristics/regression snapshot is anchored to the same merge base the artifact diffs already use. Added a git-backed test that builds merge-base M / target T / advanced-base B and asserts regression.base_sha equals the merge base when the merge base is passed and the base tip when the tip is passed.

Comment thread src/mcp/read.rs Outdated
Comment on lines 416 to 418
run_id: e.id.clone(),
commit: e.commit.clone(),
}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Prefer the in-flight run for the current HEAD

If a completed run for this same HEAD already exists and the user starts a new deep run, this match returns the old index entry before the new RUNNING.json scan is considered. In that common rerun-on-same-commit case, verdict without a run_id reports the stale completed pack while state reports the in-progress run, so pollers can stop early with the previous result. Check the on-disk running marker first (as state does) or compare start times before returning the indexed run.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5597cac. resolve_run without a run_id now prefers a live RUNNING run for HEAD (mirroring what the state tool reports) over the newest indexed completed pack, so on a rerun-on-the-same-commit verdict reports in_progress and a poller keeps polling instead of stopping early on the previous run's pack. With no live run it returns the indexed completed run, then falls back to any unregistered on-disk run. A unit test covers both cases (in-flight present -> in-flight; completed only -> completed). We deliberately avoided a created_at vs started_at timestamp comparison because those fields come from different code paths and are not guaranteed comparable; "prefer the live run when one exists" is robust and matches state.

Comment thread src/checks/cargo.rs Outdated

fn cache_key(&self, config: &Config) -> Option<String> {
Some(cache::rust_hash(&config.repo_root))
Some(cache::rust_hash(cargo_cache_root(config)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include workspace lockfiles in member Rust cache keys

When cargo_root is a workspace member selected by config/manifest, Cargo still walks up and uses the workspace Cargo.lock at the repo root, but this new key hashes only files under the member. A root lockfile-only dependency update can therefore reuse a cached Cargo check/Clippy/geiger result even though the dependency set being compiled changed. Include the discovered workspace lockfile, or at least a repo-root lock fallback, in the cache key for member cargo roots.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 85d4eb7. When a check runs against a workspace member whose cargo root differs from the repo root, the cache key now folds in the root Cargo.lock hash ({member_hash}-root:{root_lock_hash}), so a root-lockfile-only dependency bump invalidates the member's cached result. Applies to CargoCheck, ClippyCheck and CargoGeigerCheck; rustfmt (dependency-independent) and cargo_audit (already lock-keyed) are unchanged. A test bumps only the root Cargo.lock for a member with no own lockfile and asserts the key changes.

vetcoders-agents and others added 6 commits July 7, 2026 15:17
Replace the hand-rolled escape_glob_literal helper with the standard
glob::Pattern::escape. The crate's escape brackets exactly the chars glob
treats as metacharacters (? * [ ]); braces are literal in this crate (no
brace expansion), so the custom {}/[] handling was superfluous. Behaviour
is unchanged: the repo-root escaping test (prefix `repo[old]`) still passes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For a workspace-member cargo_root, `rust_hash` hashed only files under the
member — and a member usually has no Cargo.lock of its own. Cargo resolves
the dependency set from the workspace-root Cargo.lock, so a root-lockfile-only
dependency bump reused a stale Cargo check / Clippy / geiger result. Fold the
repo-root lockfile into the key whenever cargo_root != repo_root so such a
bump invalidates the member key. rustfmt (dep-independent) and cargo_audit
(already lock-keyed) are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The artifact diff switched to the merge-base range (diff_bases), but the
snapshot regression path still snapshotted the raw base tip. When the base
branch advanced with unrelated work, the regression delta was computed against
base-only files the patch excludes, fabricating regressions/caveats. Feed
run_heuristics_with_snapshots the same diff_bases so every signal shares one
range. Adds a test asserting the regression anchors to the base ref passed in.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rade

After A2, ruff/eslint/stylelint run through plan_check_run, which materialises
a worktree snapshot of the target and scans there in --pr/--remote mode — just
like semgrep. Their out-of-diff findings therefore genuinely predate the target
diff, but check_scans_target_snapshot still trusted only semgrep_scan, leaving a
pre-existing ruff/eslint/stylelint finding Unclassified and able to block the
merge. Add them to the trust list; rustfmt/cargo_audit stay excluded (they scan
the local checkout, a different tree). Test updated to the corrected semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed pack

resolve_run without a run_id returned the newest indexed COMPLETED run for HEAD
before considering the on-disk RUNNING marker. On a rerun-on-the-same-commit a
poller calling verdict then got the previous completed pack and stopped early,
while state reported the fresh in-flight run — the two disagreed. Prefer a live
RUNNING run for HEAD (mirroring the state tool) so verdict reports in_progress
and the poller keeps polling; fall back to the indexed run, then to any
unregistered on-disk run. Adds a unit test for the decision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In remote/PR mode each snapshot-backed check (Ruff, Mypy, TypeScript, ESLint,
Vitest, Stylelint) called plan_check_run independently, so every one created and
tore down its own ephemeral worktree of the target commit — up to six concurrent
git worktree add/remove pairs contending on the index lock and re-checking-out
the whole tree. Materialise ONE shared snapshot per run in run_all /
run_all_with_events, point checks at it via a new Config::scan_dir_override, and
keep it alive until every check finishes. On snapshot-creation failure the
override stays unset and each check falls back to its own plan (original
behaviour). Adds tests proving N checks reuse the override with zero new
worktrees and that a cargo-only run stays a no-op.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On Linux the CI Coverage job hit a "failed to spawn .../node_modules/.bin/
faketool: Text file busy (os error 26)" panic: the test writes a fresh
executable and immediately spawns it, and a parallel test's fork can transiently
inherit the write fd, so execve races the still-open descriptor. Write the
script through a scope that closes and fsyncs the file before chmod/spawn, and
wrap the spawn in a short retry loop that backs off on os error 26 (the racing
child exec's and drops the inherited fd almost immediately). Test-only
hardening; no production code changes.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db78f3ecac

ℹ️ 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".

Comment thread src/checks/mod.rs
Comment on lines +231 to +232
let mut config = config.clone();
let _shared_snapshot = share_target_snapshot(&mut config, &runnable_checks);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Build runnable checks from the target tree

In --pr/--remote runs where HEAD is not the target, this shared snapshot is created only after get_checks_for_profile and check.check_eligibility(config) have already filtered runnable_checks using config.profile and files under config.repo_root. A PR that adds package.json/tsconfig.json/pyproject.toml only on the fetched target is therefore skipped entirely, while a PR that removes one can still run tsc/eslint/ruff inside the target snapshot and fail a non-project. Please make check selection/eligibility target-aware before filtering, so remote runs select checks for the same tree they scan.

Useful? React with 👍 / 👎.

Comment thread src/tui/mod.rs
.resolve_diff_bases(&target, &bases, app.config.quiet);
let diffs = app
.repo
.generate_diffs(&target, &diff_bases, app.config.quiet)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use diff_bases for TUI regression snapshots

Fresh evidence: the TUI path now generates its artifact diff from diff_bases, but the remote heuristic regression immediately below still snapshots bases.first().commit_id and records base_sha from bases. In a remote TUI run where the base branch advanced, the pack excludes base-only files while the regression/caveats compare against the base tip, recreating the fabricated regression issue that was fixed in the non-TUI path. Use diff_bases for base_snap/base_sha here as well.

Useful? React with 👍 / 👎.

Comment thread src/checks/cargo.rs
format!(
"{}-root:{}",
base,
cache::cargo_lock_hash(&config.repo_root)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include the workspace manifest in member Cargo keys

When cargo_root is a workspace member, the new key hashes the member plus only the repo-root Cargo.lock, whereas the old repo-root rust_hash also covered the workspace Cargo.toml. Cargo still reads the workspace manifest when invoked from a member for [workspace.dependencies], [patch], resolver and workspace lint settings, so a root-Cargo.toml-only change can now reuse stale Cargo check/Clippy/geiger results. Include the root manifest in this member-root suffix, not just the lockfile.

Useful? React with 👍 / 👎.

@m-szymanska m-szymanska merged commit 853d54d into main Jul 8, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants