Skip to content

fix(checks): run every check on the reviewed snapshot and record scanned-tree provenance - #20

Merged
m-szymanska merged 73 commits into
mainfrom
fix/substrate-truth
Aug 22, 2026
Merged

fix(checks): run every check on the reviewed snapshot and record scanned-tree provenance#20
m-szymanska merged 73 commits into
mainfrom
fix/substrate-truth

Conversation

@m-szymanska

@m-szymanska m-szymanska commented Aug 22, 2026

Copy link
Copy Markdown
Member

Substrate truth: every check judges the reviewed commit, and proves it

Part 1 of a 2-PR campaign attacking the root cause behind prview's recurring regressions: the truth about what was scanned lived in N places and none of them was authoritative. This PR makes the scanned substrate correct and auditable.

What was broken

  1. Pytest ran on the local checkout (config.repo_root), not the reviewed snapshot. Reviewing a PR while another branch was checked out reported the local branch's test failures against the PR. Ruff/Mypy/JS were fixed in July (7d32744); pytest was the one check left behind.
  2. All six cargo checks ran at cargo_cache_root (the local tree) for the same historical reason: build cache. A --pr pack could combine the target's diff with build/clippy/test/fmt verdicts from unrelated local code — the likely root cause of the 2026-07-24 remote-substrate-mutation incident.
  3. Cargo cache keys couldn't tell substrates apart: the cached-result lookup runs before the snapshot is materialised, so a --pr run could hit an entry stored by a local run and serve the local checkout's verdict as the PR's.
  4. No artifact could prove what was scanned. CheckProvenance carried cwd only — no tree SHA, no cleanliness. This is the "tree provenance" gap from the PR #46 artifact audit.

What this PR does

  • Pytest resolves its cwd through plan_check_run → shared target snapshot, like every other language check (fix/pytest-on-pr-head).
  • Cargo checks run in the snapshot with the build cache preserved via CARGO_TARGET_DIR~/.prview/cargo-target/<repo>, passed to the child process only. Local reviews (target == HEAD) are byte-for-byte unaffected. Cache keys use the resolved target commit when it differs from HEAD (fix/cargo-on-reviewed-snapshot).
  • Every check's provenance now records target_sha (commit whose tree was scanned) and tree_state (snapshot / local-clean / local-dirty), resolved from the directory the command actually ran in — one resolver, no per-check duplication (fix/provenance-target-sha). Additive, no schema bump; old packs parse unchanged.
  • Docs: new architecture.md sections "Where checks run" and "Check provenance".

Verification

  • Each cut was falsified before merge: reverting the fix makes its regression test fail (recorded in the branch reports).
  • Full gates on the integrated branch: 1221 lib + 81 integration tests, 0 failures; clippy -D warnings clean; fmt clean.
  • Dogfood: a release build of this branch reviewing its own tree emits target_sha == HEAD, tree_state: local-clean in RUN.json, gate .result.json and report.json.
  • One semantic (non-textual) merge conflict surfaced and was resolved with its own test update: the pytest branch pinned "cargo stays out of the shared snapshot", the cargo branch moved it in — the contract test now asserts the integrated truth.

Deliberate decisions (flagged, not hidden)

  • rustfmt/cargo_audit are not added to check_scans_target_snapshot (the pre-existing-findings downgrade list), even though they now scan the snapshot. Widening that list makes the gate more permissive — that's verdict semantics, owned by the upcoming decision-core work, not a side effect of a substrate fix.
  • Known trade-offs: first --pr run after this change compiles cold into the new cache dir; the cache dir has no GC yet; concurrent runs on different commits share it and rely on cargo's own locking (in-run cargo checks were already serialized).

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

  • Substrate manifest (feat/substrate-manifest): the pack now proves its substrate instead of merely recording it per check. 00_summary/PROVENANCE.json states the resolved target SHA and worktree state (clean + status_digest) for the whole run; cache entries gain a <key>.prov.json sidecar recording what tree produced them; pack sanity requires the manifest's required_files. Cleanup removes sidecars with their entries.
  • RUSTSEC-2026-0213: ammonia bumped 4.1.3 → 4.1.4 (lockfile-only; Security Audit was red on main too).

Integrated gates after the follow-up: 1312 tests, 0 failures; clippy -D warnings clean; fmt clean.

  • Review followup (round 1) (fix/review-followup-substrate): all 8 first-round threads addressed — an external cargo_root off-HEAD now skips the cargo checks with an explicit reason instead of scanning the operator's unrelated checkout (no verdict beats a wrong verdict); cache keys discriminate the cargo root (commit-<sha>-root:<rel>); a failed git status yields unknown tree state, never a false local-clean; off-HEAD python checks isolate uv into ~/.prview/uv-env/<repo> instead of mutating the operator's .venv; a mutated snapshot loses its clean certification (snapshot-dirty); a moved Cargo.toml resolves against the snapshot. Submodule init is deferred with a documented limitation (network-bound, mutates the superproject's .git/modules). One model underneath: repo_relative_cargo_root() feeds the snapshot mapping, the cache key, and the skip decision, so they cannot diverge. Details in the individual thread replies.

Integrated gates: 1324 tests, 0 failures; clippy -D warnings clean; fmt clean.

  • Review followup (round 2) (fix/review-followup-substrate-2): PROVENANCE.json records the merge-base the diff actually used (not the base tip); the worktree digest fingerprints changed bytes (streamed sha256 of the dirty subset), so two runs differing only in content no longer share a digest; --watch refreshes worktree provenance every iteration; cache entries publish atomically as a single file (staging + rename, legacy entries still readable); AI_INDEX.md documents PROVENANCE.json in the pack reading order. Known candidate follow-up flagged, not silently patched: RUN.json freshness.base_sha has the same tip-vs-merge-base issue (no consumers in-repo).

  • Review followup (round 3) (fix/review-followup-substrate-3): uv environments keyed per reviewed substrate (target SHA token, conservative pruning: 3 freshest kept, nothing used within 24h deleted); cargo eligibility asks the reviewed commit's tree (path_exists_at_commit) instead of the local profile, so a target that dropped its last Cargo.toml skips with a reason instead of six missing-manifest failures; cache keys are filesystem-safe (commit-<sha>-root-<hash>, no slashes or colons — the nested-root shape previously failed every write, so those checks silently recomputed each run).

  • Review followup (round 4) (fix/review-followup-substrate-4): a cargo root moved to a different directory in the reviewed commit is rediscovered from the git tree (mapped root → repo root → exactly-one manifest within 2 levels; ambiguity skips with the candidates named instead of guessing), and a symlinked cargo root can never escape the snapshot (git-tree resolution cannot traverse symlinks, plus a canonicalizing containment check as defence in depth).

  • Review followup (round 5) (fix/review-followup-substrate-5): cargo results for targets without a committed Cargo.lock are day-stamped in the cache key (the Cargo audit pattern) instead of cached indefinitely by commit — bounded staleness without paying dependency resolution before the lookup; nested repositories contribute gitlink:<head>:<clean|dirty> to the worktree digest instead of a literal dir, so materially different submodule states no longer share a digest.

Final integrated gates: 1343 tests, 0 failures; clippy -D warnings clean (verified on 1.93/1.95/1.97); fmt clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr

vetcoders-agents and others added 9 commits August 22, 2026 13:56
PytestCheck spawned `uv run pytest` / `pytest` with `config.repo_root` as
its working directory. With a PR or remote target, repo_root still holds
whatever branch happens to be checked out locally, so pytest ran the
local branch's tests and reported their failures against the PR under
review — a false failure from unrelated code even when the PR's own
tests were green (observed reviewing Memex PR #1 against a local
develop).

Ruff, Mypy and the JS checks were moved onto the target snapshot in
7d32744; Pytest was the single check left behind. It now resolves its
cwd through plan_check_run() like its siblings, and is registered in
uses_shared_scan_dir() so a Python run reuses the one run-wide worktree
instead of materialising a second. create_worktree_snapshot symlinks
.venv into the snapshot, so the installed environment is preserved.

For a local review the target resolves to HEAD and the plan returns
repo_root, leaving that path byte-for-byte unchanged.

provenance.cwd reported repo_root as well, claiming a directory the run
never used; it now reports the actual run dir.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
The rule that checks judge the reviewed commit rather than the local
checkout was implemented but never written down, which is how the Pytest
instance stayed invisible. Document plan_check_run()'s resolution, the
shared-snapshot set, and the two deliberate opt-outs (semgrep's own
worktree, cargo's build-cache root), plus a changelog entry for the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…uild cache

Cargo check, Clippy, Rustfmt, Cargo test, Cargo audit and Cargo geiger ran
at `cargo_cache_root` — the LOCAL working tree — while every other language
check resolves its cwd through `plan_check_run` and scans the reviewed
target snapshot. In `--pr`/`--remote` mode that meant a pack combined the
target's diff with Rust verdicts produced from whatever branch happened to
be checked out locally (the 2026-07-24 remote-only regression).

The build cache was the reason for the shortcut: a snapshot is a throwaway
temp dir, so compiling in it starts from zero every run. `plan_cargo_run`
keeps the cache by pointing `CARGO_TARGET_DIR` at a per-repo shared
directory (`~/.prview/cargo-target/<repo>`) — passed to the cargo child
process only, so it cannot leak into concurrent checks, and kept out of the
operator's own `target/` so prview never overwrites a locally built binary.
A local review (target == HEAD) is byte-for-byte unchanged: same cwd, empty
environment override.

Cache keys had the same substrate bug one level down. `load_cached_result`
runs before the shared snapshot is materialised, so a key derived from the
scan dir would read local and write reviewed — and a `--pr` run could hit
the entry an earlier local run stored under that working-tree hash and
serve the local checkout's verdict. Cargo keys now resolve the target
commit directly and key on it whenever it differs from HEAD.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
Record the reviewed-snapshot substrate rule in architecture.md: how
plan_check_run resolves a check's cwd, which checks share the run-wide
snapshot, semgrep's deliberate opt-out, and the cargo build-cache
redirection plus its concurrency and cache-key consequences.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…hot)

# Conflicts:
#	CHANGELOG.md
#	docs/architecture.md
#	src/checks/mod.rs
…ation

The pytest branch's contract test asserted cargo checks stay out of the
shared scan dir; the cargo-on-reviewed-snapshot branch moved them in.
The integrated truth is the union: all plan_check_run-resolved checks
share the run-wide snapshot, semgrep remains the single opt-out.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
CheckProvenance carried `cwd` but no evidence of WHICH tree the gate read,
so an artifact pack could not prove a check saw the reviewed commit rather
than an operator's uncommitted working tree.

Add two additive, optional fields resolved from the directory the command
actually ran in, by a single `resolve_scan_substrate(cwd, repo_root)`:

- `target_sha`: commit whose tree was scanned (the snapshot's detached
  commit, or the repo HEAD for an in-place scan)
- `tree_state`: `snapshot` | `local-clean` | `local-dirty`

Deriving both from the real cwd — instead of re-deriving the plan — keeps
one source of truth and makes provenance follow any future change in where
a check runs. Every gate that builds provenance is covered: the
ProvenanceBuilder fills the fields for cargo/semgrep, the literal
constructions in the python and typescript checks chain
`with_scan_substrate` onto the same helper.

ProvenanceBuilder gains a `repo_root` field; `build_with_repo_root(Option)`
splits into `build` (cwd verbatim) and `build_repo_relative_cwd` (cwd
repo-relative) so one repo root serves both the substrate lookup and the
path rendering. Rendered `cwd` values are unchanged.

Fields surface in `20_quality/<gate>.result.json`, `full-checks.log`,
`00_summary/RUN.json` and `report.json`. Serde `default` +
`skip_serializing_if` keeps old packs readable and emits no null keys, so
no artifact schema_version bump is required.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…et-sha)

# Conflicts:
#	CHANGELOG.md
#	docs/architecture.md
#	src/checks/python.rs
Copilot AI lite review requested due to automatic review settings August 22, 2026 12:40

@claude claude 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 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 AI 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.

Pull request overview

This PR runs checks against the reviewed snapshot and records scanned-tree provenance.

Changes:

  • Moves Pytest and Cargo checks onto reviewed snapshots.
  • Preserves Cargo caching via CARGO_TARGET_DIR.
  • Adds target_sha and tree_state to artifacts and reports.
  • Updates tests, documentation, and changelog.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Summary
src/config/mod.rs Adds shared Cargo cache path resolution.
src/cli/mod.rs Updates provenance-related help text.
src/checks/typescript.rs Records substrate provenance.
src/checks/semgrep.rs Supplies repository context for provenance.
src/checks/python.rs Runs Pytest in the reviewed snapshot.
src/checks/mod.rs Adds shared substrate resolution; cleanliness failures must not be reported as clean provenance. Moderate, 3 votes.
src/checks/cargo.rs Cargo checks may fall back to an unrelated local checkout off-HEAD. Critical, 2 votes. Cache keys also omit the configured Cargo root. Critical, 3 votes.
src/artifacts/verdict.rs Preserves verdict downgrade behavior.
src/artifacts/tests.rs Tests provenance artifact behavior.
src/artifacts/report.rs Adds substrate fields to reports.
src/artifacts/mod.rs Emits provenance in generated artifacts.
docs/architecture.md Documents check execution and provenance.
CHANGELOG.md Records the changes.
Suppressed comments (3)

docs/architecture.md:257

  • “Every non-cached check” is stronger than the implementation: runtime spawn errors, Semgrep worktree failures, and Cargo geiger's virtual-manifest/timeout paths return non-cached results with provenance: None (for example, src/checks/cargo.rs:732-740). Qualify this as every non-cached check that actually executes a command so consumers do not require provenance on skipped or unavailable results.
Every non-cached check records a `CheckProvenance` alongside its result:

src/checks/mod.rs:119

  • Excluding ignored entries means an ignored test_*.py (or another file collected by a broad check) can be present in repo_root, be scanned by Pytest, and still yield LocalClean; the emitted target_sha then falsely claims the scanned bytes exactly match the commit. Either include ignored files in the conservative classification or weaken the LocalClean contract so it does not promise byte-for-byte identity.
    let mut opts = git2::StatusOptions::new();
    opts.include_untracked(true)
        .include_ignored(false)
        .include_unmodified(false);

src/checks/python.rs:650

  • The guard checks for either executable, but the implementation always prefers uv when it is present. If uv is installed while pytest is not, this fixture still runs uv run pytest from a temporary directory with no pyproject.toml or virtual environment and can fail before testing the scan directory. Make the fixture provide the selected launcher with a usable pytest environment, or skip when that exact invocation is unavailable.
    async fn test_pytest_runs_in_scan_dir_not_repo_root() {
        if which::which("pytest").is_err() && which::which("uv").is_err() {
            return;

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

Comment thread src/checks/cargo.rs Outdated
Comment thread src/checks/cargo.rs
Comment thread src/checks/mod.rs Outdated

@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: d5e5ccdcc6

ℹ️ 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/python.rs Outdated
Comment thread src/checks/cargo.rs
Comment thread src/checks/mod.rs Outdated
Comment thread src/checks/cargo.rs Outdated
Comment thread src/checks/cargo.rs Outdated
Copilot AI review requested due to automatic review settings August 22, 2026 13:03

Copilot AI 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.

Pull request overview

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

Suppressed comments (9)

src/artifacts/report.rs:735

  • Cached results still arrive from load_cached_result with provenance: None, so this tuple emits no target_sha/tree_state (or cache key) whenever a check is served from cache. A PR run can therefore produce a cached gate/report with no evidence of which substrate was scanned, defeating the auditability goal; persist provenance in the cache or reconstruct it when loading the cached result.
                        prov.target_sha.clone(),
                        prov.tree_state.map(|state| state.as_str()),
                        if prov.hard_fail_signatures.is_empty() {
                            None
                        } else {

src/checks/cargo.rs:75

  • Because this branch is taken in an off-HEAD run, it leaves Cargo on the local cargo_root even though the target snapshot exists. off_head_target_commit still keys the result by the reviewed commit, and the provenance resolver classifies this external git directory as snapshot, so a result from an unrelated local tree can be cached/reported as the PR's. Fail the check (or otherwise mark it unscannable) when the configured root cannot be mapped into the snapshot instead of falling back.
    let Some(cwd) = snapshot_cargo_root(&local_root, &config.repo_root, &plan.scan_dir) else {
        // A cargo root configured outside the repo cannot exist inside a
        // snapshot of that repo, so there is nothing to redirect to: keep the
        // previous local-tree behaviour rather than invent a path.
        return Ok(CargoRun {

src/checks/cargo.rs:136

  • The cache-safety change is not exercised by the current tests: test_cargo_run_uses_shared_build_cache_off_head uses a non-git repo plus scan_dir_override, so off_head_target_commit returns None and the new commit-based branch is never reached. Add a real git fixture with HEAD != target and assert the cargo/clippy/rustfmt/audit keys differ from the local key and contain the target commit, otherwise the reported PR/local cache collision can regress undetected.
    if let Some(commit) = reviewed_substrate_key(config) {
        return commit;
    }

src/checks/cargo.rs:1511

  • Despite its name and assertions, this test invokes RustfmtCheck.run, not CargoCheck.run, so it never exercises the new cargo check snapshot path at lines 191-195. Rename it to describe Rustfmt or switch the invocation to CargoCheck (and keep separate coverage for both) so a regression in the check implementation cannot pass under the wrong test name.
    async fn test_cargo_check_runs_in_scan_dir_not_repo_root() {

src/checks/cargo.rs:156

  • Using only the reviewed commit as the Cargo cache key drops the configured cargo root/workspace from the cache identity. Two configurations in the same repository can analyze different Cargo.toml roots at the same commit (for example, the repository root versus a workspace member), but Cargo check, Clippy, Rustfmt, audit, and geiger will now share a key and can return one workspace's verdict for another. Preserve a stable cargo-root/workspace component alongside the target commit (including the audit key).
/// The reviewed tree is fully determined by the target commit, so the commit id
/// IS the content key — no file hashing needed, and no chance of colliding with
/// a local-tree key written by an earlier local run (which would serve the local
/// checkout's verdict for a PR).

src/checks/mod.rs:122

  • An error from repo.statuses is converted to false, so the caller records LocalClean and claims the scanned bytes exactly match target_sha even though cleanliness was never verified. That makes the new audit signal actively misleading on status/read errors; propagate an unknown substrate (leave both fields unset) rather than treating an error as clean.
    repo.statuses(Some(&mut opts))
        .map(|statuses| !statuses.is_empty())
        .unwrap_or(false)

src/checks/mod.rs:99

  • is_external is only a path comparison, but externality is not equivalent to being an ephemeral snapshot. An external Cargo root can be a real unrelated Git repository, while a snapshot created under TMPDIR inside the repo is classified as local. Both cases produce incorrect tree_state/target provenance; identify snapshots from the plan/worktree metadata or expected target commit rather than path location alone.
    let is_external =
        crate::paths::normalize_to_repo_relative(&cwd.display().to_string(), repo_root).is_external;

    let tree_state = if is_external {
        TreeState::Snapshot

src/checks/mod.rs:52

  • local-clean claims the scanned bytes are exactly target_sha, but the dirty check explicitly excludes ignored files. Commands such as pytest -v, ruff check ., and Cargo can read ignored/generated source files, so a Git-clean working tree can still differ from the commit being reported. Either account for relevant ignored content or weaken this state to tracked-tree cleanliness.
    /// The repo's own working tree with no uncommitted changes — the scanned
    /// bytes are exactly `target_sha`.
    LocalClean,
    /// The repo's own working tree carrying uncommitted changes — the scanned
    /// bytes are NOT exactly `target_sha`.
    LocalDirty,

src/config/mod.rs:1004

  • prview_home() returns PRVIEW_HOME verbatim. If that environment variable is relative, this relative CARGO_TARGET_DIR is resolved by Cargo against the snapshot cwd, so each ephemeral snapshot gets its own throwaway cache instead of the shared per-repo cache. Normalize or reject relative PRVIEW_HOME before passing this path to the child process.
    pub fn cargo_build_cache_dir(&self) -> PathBuf {
        prview_home()
            .join("cargo-target")
            .join(cache_namespace_from_root(&self.repo_root))

vetcoders-agents and others added 5 commits August 22, 2026 15:20
A cache hit returned `provenance: None`, so the fastest runs -- the ones
where every gate is served from cache -- were the only ones with no audit
trail at all: no command, no cwd, no target_sha, no tree_state. The
substrate work of this line stopped exactly where the run stopped
executing.

The provenance is now serialized next to the cache entry
(`<key>.prov.json`) and replayed on a hit. The replayed record describes
the run that populated the entry; `CheckResult::cached` is what marks the
row as a replay rather than a fresh execution.

The cache stores the blob opaquely and never interprets it, so the
provenance schema stays owned by `checks`. Entries written before the
sidecar existed -- and blobs that no longer parse -- replay with an
unknown provenance instead of failing the run, so no cache invalidation
is needed. Cleanup no longer counts sidecars as cache entries (which
would have evicted live entries) and removes them by suffix rather than
`with_extension`, which would orphan them for any key carrying a dot.
The cached-result lookup runs before the shared target snapshot exists,
so a key derived from the local working tree would let a `--pr` run hit
the entry a previous local run stored -- serving the local checkout's
verdict as the reviewed commit's. Ruff, Mypy, TypeScript, ESLint and
Stylelint already key on the target commit id whenever it differs from
HEAD, mirroring what `reviewed_substrate_key` does for the cargo checks;
nothing asserted it.

The test proves the three claims that matter: an off-HEAD key names the
analysed commit, a local run and a run on a fetched target never share an
entry, and two different targets do not collide with each other.
Per-check provenance answers "what did this gate read". A reviewer
holding only the pack still had to reconstruct the run's substrate from
those scattered rows. PROVENANCE.json answers "what did this pack judge",
once: the target commit, the diff baseline, the commit checked out
locally, whether the working tree was clean when the run started, and one
row per check naming the directory, commit and tree state it read plus
whether that row is a cache replay.

Working-tree cleanliness was already frozen before any check ran (R4-19)
but was never written to the pack. It now travels with a digest of the
status from the SAME read, so two differently-dirty runs are
distinguishable and the pack can never claim a clean tree next to a
fingerprint of uncommitted changes. `capture_worktree_clean` is folded
into `capture_worktree_provenance` for that reason -- one read, two
facts, no way for them to disagree.

Purely additive: no existing pack file changed shape. The manifest hashes
it like any other artifact, and the sanity required_files check now
requires it, so a pack that silently loses it fails its own validation.
… gap

Records the two behaviour changes in the architecture and usage docs and
the changelog: cache hits now replay the provenance of the run that
filled the entry, and every pack carries a pack-level PROVENANCE.json.

Also names the limitation instead of leaving it implied. Provenance is an
observation a check WRITES, not a constraint the type system enforces:
the Check trait still hands each implementation a &Config and trusts it
to resolve its own directory and report what it read. Making the
substrate a parameter -- a CheckContext whose run() cannot look elsewhere
-- is the 0.8 cut. Until then the guarantee is "every check reports where
it ran", not "no check can run anywhere else".
Adds PROVENANCE.json to the artifact pack summary, cache provenance
sidecars, and required-files sanity so every pack states what tree it
was judged against.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
Copilot AI review requested due to automatic review settings August 22, 2026 13:30

@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: 81a21e3651

ℹ️ 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/artifacts/mod.rs Outdated
Comment thread src/artifacts/verdict.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/cache/mod.rs Outdated

Copilot AI 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.

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

Suppressed comments (10)

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

src/checks/mod.rs:99

  • is_external is the only condition for Snapshot, so any external Git repository is reported as the reviewed snapshot. For example, the out-of-repository cargo-root fallback above can execute in another repository, whose unrelated HEAD is then emitted as a snapshot target_sha. Validate the external repository against the resolved target, or carry the snapshot identity into this resolver, instead of inferring it from path location alone.
    let is_external =
        crate::paths::normalize_to_repo_relative(&cwd.display().to_string(), repo_root).is_external;

    let tree_state = if is_external {
        TreeState::Snapshot

src/artifacts/mod.rs:625

  • This adds a new required Artifact Pack v1 file and schema, but the docs/contracts set has no contract for PROVENANCE.json describing its version, fields, or timing. Architecture and usage prose do not provide a versioned consumer contract; add one alongside this change so pack consumers do not have to reverse-engineer the new artifact.
    // 00_summary/PROVENANCE.json — pack-level substrate record. Written before
    // RUN.json/MANIFEST so the manifest hashes it like any other pack file.
    let t = Instant::now();
    generate_provenance_json(ProvenanceJsonInput {
        dir: &summary_dir,

src/artifacts/mod.rs:1154

  • resolved_bases are the original base tips, but the callers compute diff_bases = resolve_diff_bases(...) and generate the pack's diffs from those merge-base refs. When a base branch has advanced, this records the base tip rather than the commit the pack actually diffed against, so PROVENANCE.json's base_sha is not an auditable diff baseline. Pass the diff-baseline refs into this generator while retaining the display/base-tip refs separately.
        "base_sha": resolved_bases.first().map(|b| b.commit_id.as_str()),

src/cache/mod.rs:104

  • The cache key is published before the newly added output/provenance sidecars. A concurrent prview can observe the status file and return a cached result while *.prov.json is absent or still belongs to another write, so the promised cache provenance can be missing or mismatched (and the output sidecar has the same race). Publish a complete entry atomically under a per-entry lock, or otherwise make the key visible only after all sidecars are committed.
        // Write provenance if present, and drop a stale one otherwise so a
        // re-run without provenance can never replay the previous run's.
        match provenance {
            Some(provenance) => {
                fs::write(sidecar(&cache_dir, key, PROVENANCE_SUFFIX), provenance)?;
            }
            None => {
                let _ = fs::remove_file(sidecar(&cache_dir, key, PROVENANCE_SUFFIX));

src/checks/cargo.rs:75

  • snapshot_cargo_root can return None for a configured cargo root outside repo_root, and this branch silently runs the check against local_root. That breaks the reviewed-snapshot invariant: a --pr/--remote run can still report the local checkout's cargo result. Reject or skip this configuration (or materialise an equivalent snapshot) instead of falling back to the local tree.
    let Some(cwd) = snapshot_cargo_root(&local_root, &config.repo_root, &plan.scan_dir) else {
        // A cargo root configured outside the repo cannot exist inside a
        // snapshot of that repo, so there is nothing to redirect to: keep the
        // previous local-tree behaviour rather than invent a path.
        return Ok(CargoRun {

src/checks/cargo.rs:159

  • reviewed_substrate_key keys every off-HEAD run by the target commit, but plan_cargo_run can fall back to local_root when snapshot_cargo_root returns None. In that case this key is attached to a result produced by the local checkout, so a later run can replay unrelated local contents under the reviewed commit's key. Only use the commit key when the cargo root is snapshot-mappable, or disable caching for this case.
fn reviewed_substrate_key(config: &Config) -> Option<String> {
    off_head_target_commit(config).map(|commit| format!("commit-{commit}"))
}

src/checks/cargo.rs:1529

  • This regression test invokes RustfmtCheck, so it never exercises the Cargo check path described by its name and docstring. A regression in CargoCheck could pass while this test remains green; use CargoCheck with a compile-failing local fixture, or rename this test and add a separate CargoCheck case.
        let result = RustfmtCheck.run(&config).await.expect("rustfmt run");

src/checks/cargo.rs:135

  • When the target is off HEAD, this replaces the content hash with only the target commit. That drops the configured cargo_cache_root identity, so runs against the same commit from a workspace root and a member (or two different configured members) share one result even though Cargo scans different manifests/packages and can produce different verdicts. Include a stable cargo-root/manifest identity in the off-HEAD key as well.
    if let Some(commit) = reviewed_substrate_key(config) {
        return commit;

src/checks/mod.rs:156

  • This resolver is called only after build_with_cwd_display has awaited the command. A local check that creates a non-ignored file, or a concurrent worktree change, can therefore be recorded as local-dirty even though it started from a clean tree, making the per-check substrate disagree with the tree that was scanned. Capture the substrate before spawning and carry it through the result.
    pub fn with_scan_substrate(mut self, cwd: &Path, repo_root: &Path) -> Self {
        let substrate = resolve_scan_substrate(cwd, repo_root);
        self.target_sha = substrate.target_sha;
        self.tree_state = substrate.tree_state;

src/checks/mod.rs:122

  • unwrap_or(false) maps a status-read failure to LocalClean, so provenance asserts that the checked-out HEAD was clean when the repository could not be inspected. For an audit record, preserve an unknown state (for example, return no tree state) rather than reusing the gate's permissive fallback.
    repo.statuses(Some(&mut opts))
        .map(|statuses| !statuses.is_empty())
        .unwrap_or(false)

Comment thread src/artifacts/mod.rs
vetcoders-agents and others added 2 commits August 22, 2026 15:45
resolve_scan_substrate treated every directory outside repo_root as an
untouched snapshot of the reviewed commit, and an unreadable git status
as a clean tree. Both are claims the code never checked:

- a snapshot is mutable — cargo generating an untracked Cargo.lock leaves
  the pack certifying bytes that already changed;
- an external directory (a cargo_root configured outside the repo) is a
  different checkout entirely, not a snapshot of the target;
- a statuses() failure (index lock, permissions, malformed repo) resolved
  to local-clean, asserting an exact commit match precisely when nothing
  could be verified.

Snapshots are now confirmed against the repo's common git dir and their
own status (snapshot / snapshot-dirty, ignoring the node_modules and
.venv symlinks prview itself creates), a foreign checkout is labelled
foreign, and an unreadable status records no tree_state at all — the
same visibly-unknown the non-git case already used.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
The snapshot move left three ways for a cargo verdict to describe a tree
the review does not:

- a cargo_root outside the repo cannot exist in a snapshot of that repo,
  and the fallback ran cargo at the local path anyway — a foreign tree's
  verdict filed under the reviewed commit. Those runs now skip with a
  reason naming the unreachable root; plan_cargo_run fails loudly instead
  of resolving, so no caller can reach the old silent path.
- config.profile.cargo_root describes the LOCAL checkout, so a branch
  that moved the crate had a stale path projected into the snapshot and
  cargo failed on a missing manifest. The mapped root is now validated
  and falls back to the snapshot root when that carries a manifest.
- the reviewed-substrate cache key was the commit alone, so the same
  commit checked from the workspace root and from a configured member
  collided and a later run could serve the other root's verdict. The
  repo-relative cargo root now travels in the key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
vetcoders-agents and others added 4 commits August 22, 2026 18:38
The 24-hour age floor was read and acted on without synchronisation, so
one review could stat an environment just before another refreshed its
marker and then delete the directory after that other review's `uv run`
had already started. Age cannot answer a question about concurrency.

Marking and pruning are now one critical section, taken under a
`.prview-prune.lock` at the environment root through the existing atomic
lock (create-new plus owner liveness). The lock is opportunistic: a root
held by another live review is left to it, and this run only records its
own use — which is what protects it from the next sweep. Pruning also
re-reads each candidate immediately before removing it, so a mark that
landed outside the lock still wins.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
`gitlink:<head>:<clean|dirty>` still collided where it mattered most: a
submodule parked on one commit with two different sets of uncommitted
edits fingerprinted identically, while cargo and every other check that
reads the vendored tree compiled different bytes in each run.

A dirty nested repository is now fingerprinted the way the superproject
is, recursively over its own dirty subset only — the same bound that
makes the top-level walk affordable, and reached only for a directory
the superproject already reported dirty. The recursion stops after three
levels of nesting and falls back to the bare `dirty` marker, so the
degraded path is exactly today's behaviour and never weaker.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…ompleteness, uv prune lock, dirty gitlink digest)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr

@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

prview-rs/src/checks/mod.rs

Lines 426 to 430 in 6a5141f

match run_command_with_timeout(
"uv",
&["sync", "--quiet"],
&config.repo_root,
CHECK_TIMEOUT_SECS,

P2 Badge Pre-sync the reviewed Python environment

On the first off-HEAD review of a Python commit, this pre-sync still runs in config.repo_root without the commit-specific UV_PROJECT_ENVIRONMENT that plan_python_run later assigns. The installed uv sync --help identifies the command as updating the current project's environment, so it warms the checkout's environment rather than the reviewed target's; each subsequent uv run must build the cold target environment inside its own 300-second timeout, and the parallel Python checks can spend that budget waiting for the same environment synchronization. Materialize the shared snapshot and pre-sync its commit-scoped environment before starting the per-check timers.

ℹ️ 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/python.rs
Comment thread src/checks/cargo.rs
Comment thread src/checks/mod.rs Outdated
Comment thread src/checks/cargo.rs
Comment thread src/artifacts/mod.rs Outdated
Comment thread src/checks/cargo.rs Outdated
vetcoders-agents and others added 7 commits August 22, 2026 19:40
A check running in a vendored checkout, a submodule or an in-repo
symlink to another clone was classified from its lexical position:
below `repo_root`, therefore this repository's local tree. But
`Repository::discover` had already resolved the OTHER repository, so
the row paired that project's `HEAD` with a `tree_state` asserting the
reviewed repository's own working tree — both fields wrong at once, and
in the direction that certifies rather than doubts.

Identity is now checked first, and such a directory is `foreign`
wherever it sits. The external case reaches the same answer by the same
test, so the two paths no longer disagree about what counts as proof.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
A gate ruled out during eligibility — tests disabled, a tool absent —
went into `skipped_checks` and nowhere near PROVENANCE.json, so the
manifest that exists to account for every gating signal simply omitted
it. To a consumer that is indistinguishable from a check that was never
scheduled, which is the opposite conclusion about the same run.

Each configured check now has a row. `skipped` is null for one that
ran and carries the reason for one that did not; a skipped row's
substrate fields are all null, because nothing was read.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
ruff, mypy and pytest read `pyproject.toml`, and uv resolves the
dependency set from it and `uv.lock`. A reviewed commit that tracks
either as a link to an external file therefore had every Python gate
configure itself from another project — installed from another lockfile
— while provenance recorded an exact `snapshot` scan and the cache filed
the verdict under the reviewed commit. Neither `--no-project` nor
`--locked` is passed, so nothing downstream re-asks the question.

Both files are now resolved against the tree being judged before the
plan is returned, the same refusal the Cargo manifest guards make, and
for the local checkout too. Escape is the target, not symlinks: metadata
linked to a real file inside the tree resolves back inside and still runs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
Discovery accepted any single manifest within two levels as the project
that moved. A commit that DELETES the Rust project while keeping an
unrelated crate in reach — an `examples/demo`, a fixture crate — sent
every cargo gate there and filed a green verdict for a project the
reviewed commit no longer contains. Checked out normally that commit is
not even a Rust project: profile detection never looks at `examples/`.

The lone candidate must now match the configured project's identity:
`[package] name`, or the member list for a virtual workspace root that
defines no crate of its own. Identity comes from the local manifest, the
same source the mapped candidate came from and the only statement of
which crate the review is about. A real move (root crate into `backend/`,
workspace root one level down) still resolves; no identity to compare
against is a skip with a reason, not a guess.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
The freshness stamp asked only whether a `Cargo.lock` exists. Existence
is not a pin: a target that adds a dependency without regenerating the
lock still has cargo resolve that part of the graph from the registry —
none of check/clippy/test/geiger pass `--locked`, which is what would
assert otherwise — while the key promised the commit described the run
completely, so a later semver-compatible release changed the truth and
every subsequent review replayed the old verdict until eviction.

The lock is now checked against the manifest: every declared dependency
must already appear in its package list, renames followed to the name
the lock records. A lock the manifest outgrew stamps like a missing one.
Name-level and deliberately partial — a member's own manifest is not
read, a bumped requirement whose name is still locked reads as covered —
because under-reporting is exactly today's behaviour, while a false
positive costs one cache miss a day. Anything unparsable counts as
covered, for the same reason git's unanswerable lookups already do.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
The root and its manifest being contained says nothing about what that
manifest declares. An absolute `path` dependency — or a relative one
climbing out of the snapshot, or resolving through a symlink — had cargo
compile a directory the reviewed commit does not contain, while
provenance reported a `snapshot` scan and the verdict was filed under
the reviewed commit: the symlinked-root escape, one level further in.

Off-`HEAD` runs now resolve every local path the cargo root manifest
names — dependencies, dev, build, `[workspace.dependencies]`,
`[target.*]`, `[patch]`, `[replace]` — against the snapshot and refuse
the ones that leave it, naming the dependency. Local reviews keep
running: a path dependency on a sibling checkout is an ordinary setup,
and a local run claims nothing about a commit's contents. Static and
scoped to that one manifest by choice — resolving the real graph means
`cargo metadata`, a network-capable second resolve for each of six gates.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…y-stamp, repo identity, manifest evidence, eligibility provenance, path-dep containment)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr

@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: cc4107a232

ℹ️ 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/cargo.rs Outdated
Comment thread src/checks/cargo.rs Outdated
Comment thread src/checks/python.rs
Comment thread src/artifacts/verdict.rs Outdated
Comment thread src/checks/cargo.rs Outdated
Comment thread src/artifacts/mod.rs
vetcoders-agents and others added 8 commits August 22, 2026 21:08
`build_skipped_check` derived the id with a naive slug — a fourth copy
of the normalisation `crate::check_id` exists to be the canon of. It
agrees with the alias table for most names and disagrees exactly where
an alias exists, so the same configured gate appeared as `typescript`
when it was skipped and `tsc` when it ran (`cargo_check`/`cargo`,
`vitest`/`tests`). Both ids reach the artifacts —
`REPORT.json.checks_skipped[]` and the skipped rows in
`PROVENANCE.json.checks[]` — leaving a consumer unable to pair a skip
with the gate it belongs to. The id now comes from `check_id_from_name`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
Containment resolved the cargo directory and its `Cargo.toml`, but not
its `Cargo.lock`. Cargo follows a symlinked lockfile even under
`--locked`, so a reviewed commit tracking its lock as a link to an
external file had the whole dependency graph resolved from another
project's pins — while provenance recorded an exact `snapshot` scan and
the verdict was cached under the reviewed commit. The tree-level lookup
already treats a non-regular lock as absent; this is the same refusal on
the materialised bytes, where cargo actually reads it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
Git names files in bytes. `StatusEntry::path()` gives up on anything
that is not UTF-8, so every such entry was rendered as one literal
`<non-utf8>` line and its content looked up at a path that does not
exist — reported as `absent`. Two runs dirtying different
unrepresentable names, or the same one with different bytes in it,
therefore produced the same `worktree.status_digest`, which exists
precisely to tell two substrates apart.

The path now comes from `path_bytes()`: unrepresentable names are
labelled by the hash of their bytes and resolved through an OS-native
path so their content contributes too. Windows has no byte-path API to
rebuild from, so there the content stays unknown while the name still
distinguishes the entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
The freshness test was name-level, so a requirement the lock can no
longer satisfy read as pinned: `serde = "1"` bumped to `"2"` over a lock
still holding 1.x leaves cargo to resolve that dependency from the
registry when it runs — nothing here passes `--locked` — while the key
promised the commit described the run completely.

Requirements are now matched against the locked versions with `semver`,
cargo's own parser and already in the dependency graph, so this adds no
crate to the build. Unreadable requirements and versions count as
satisfied, like every other unanswerable question in this path. A
`[patch]` redirecting a dependency outside its stated requirement now
reads as uncovered — one extra cache miss a day, against a permanent
wrong key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
`cargo check` at a workspace root builds its members, and a member
declares its own dependencies — which the containment guard never read.
A member free to name an absolute path outside the snapshot had cargo
compile foreign source under an exact `snapshot` provenance row and the
reviewed commit's cache key, the same escape the root manifest was
already held to.

Every manifest within three levels of the cargo root is now checked, via
a bounded walk that never enters a symlinked directory (the snapshot
links `node_modules` in) and skips `target/` and `.git/`; a member
manifest that is itself a link out of the snapshot is refused with them.
No subprocess, no registry, no resolve — the earlier `cargo metadata`
argument was about the true dependency graph and never defended leaving
the members on disk unread.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
The uv-environment sweep is opportunistic housekeeping: a review that
cannot take the lock marks outside it, so its mark can land between the
sweeper's final re-read and its `remove_dir_all`. That is a deliberate
trade, not an oversight, and nothing said so where the next reader looks.

Recorded with its terms: the window is one `remove_dir_all` wide, opens
only for an environment idle for a day, outside the working set, and
being started at that instant, and costs a loud `uv` failure on one gate
rather than a verdict attributed to the wrong substrate. Closing it means
marking under the lock, i.e. every off-HEAD Python review waiting on
another process's housekeeping — the fix to reach for if the failure is
ever actually seen.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
`OsStr::from_bytes` already gives `Path::join` what it needs; the extra
`to_os_string` trips `clippy::unnecessary_to_owned` under `-D warnings`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…, non-UTF-8 paths, lock containment, canonical skip IDs)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr

@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

prview-rs/src/checks/mod.rs

Lines 481 to 482 in a3bc2dd

let mut config = config.clone();
let _shared_snapshot = share_target_snapshot(&mut config, &runnable_checks);

P2 Badge Isolate checks that can mutate the shared snapshot

When a test, build script, or tool writes into its checkout, every concurrently running and subsequent check now observes that mutation because all runnable checks share this single mutable worktree. For example, a pytest fixture or Cargo build script that rewrites a tracked config/source file can make a sibling linter judge bytes that are neither the reviewed commit nor its own clean setup, producing order-dependent gate results; recording snapshot-dirty afterward exposes the contamination but does not make the verdict accurate. Use isolated worktrees for potentially mutating checks or reset/verify the shared tree before each check consumes it.

ℹ️ 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/cargo.rs
Comment thread src/artifacts/verdict.rs Outdated
Comment thread src/artifacts/verdict.rs
vetcoders-agents and others added 4 commits August 22, 2026 22:15
`execute_live_check`'s error-provenance fallback only covers the `Err`
branch, so a check that handles its own failure and returns `Skipped`
walks past it. Cargo geiger does that twice: the ten-minute timeout is
degraded to a skip rather than a gate error, and a virtual workspace
manifest is caught by a `cargo metadata` pre-flight. Both rows reached
PROVENANCE.json with null `cwd`, `target_sha` and `tree_state` — claiming
nothing was scanned when a cargo command had just read the reviewed tree.

Both arms now build provenance from the directory that command ran in.
`ProvenanceBuilder` takes `exit_code: Option<i32>` instead of borrowing an
`Output`, so a run with no exit status can still describe its substrate;
`None` there says only the status is unknown, which is the truth. A null
substrate now means what it should: nothing was read (semgrep, when the
snapshot could not be materialised).

Making the virtual-manifest arm reachable exposed a second bug: the
pre-flight tested a `root_package` key that `cargo metadata
--format-version 1` does not emit, so it was always false and every
virtual workspace paid a full geiger scan before cargo refused the
manifest. It now asks whether the manifest in geiger's own directory
appears among the workspace's packages, which keeps a member directory —
a concrete package inside a virtual workspace — scanned as before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…ad budget

A dirty symlink was fingerprinted by the pathname it names. That is what
git stores as the link's content, so it stays the link's identity — a link
retargeted at identical bytes is still a different tree — but everything
the checks read through it lives at the far end, and all of that could
change between two runs while the digest swore the substrate was
identical. The resolved file is now fingerprinted too: a directory is
recorded without being descended into (an absolute link can leave the repo
entirely), a dangling link reads `absent`, and a device or fifo is never
opened.

The reading is now bounded. This capture happens before the first check
starts, and `recurse_untracked_dirs` means an untracked dataset, model
checkpoint or vendored bundle in the dirty subset was hashed whole:
gigabytes of reading in front of a review nobody had started. One capture
may now hash 256 MiB in total — measured on this crate's release build,
256 MiB of sha256 takes ~1 s (0.94/1.55/1.14 s over three passes) — shared
across every entry and every nested repository the walk descends into. A
file that does not fit what is left is described as `stat:<len>:<mtime>`
instead: deliberately a different word from `blob:`, because it is not a
content hash. Two runs where an oversized file changed while keeping both
its size and its mtime collide — a far narrower window than the constant
"too big" marker the alternative would have used, which would have made
every large file equal to every other one. A refusal leaves the allowance
intact, so the small entries after a huge one are still hashed in full.

Entries are ordered BEFORE their content is read, not after: the budget is
spent in iteration order, so a stable order is what keeps one unchanged
tree digesting the same way twice. The key is the same `<codes> <path>`
prefix the rendered lines were sorted by, so nothing about the rendering
changes — and ordinary review-sized dirt is nowhere near the bound, so no
existing digest changes either.

A fifo, socket or device node in the dirty subset is no longer opened at
all. Git lists one like any other untracked entry, and a reader with no
writer blocked the whole run.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…k reach fingerprint, bounded dirty hashing)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
…dup)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr

# Conflicts:
#	CHANGELOG.md
@m-szymanska
m-szymanska merged commit e3f9b1a into main Aug 22, 2026
15 checks passed

@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: 48679c7121

ℹ️ 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 CHANGELOG.md
config, and other non-source files now count toward the snapshot), so the
`LOCTREE` heuristics stats (`total_files`, `total_loc`, `by_language`) report
higher, broader numbers than under 0.8 for the same tree.
||||||| 5177ace

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 Remove the leftover merge marker

This conflict marker and the duplicated ### Changed block below it are now part of the published changelog. git diff --check reports this exact line as a leftover conflict marker, so resolve the duplicated section before shipping the documentation.

Useful? React with 👍 / 👎.

Comment thread src/checks/cargo.rs
///
/// Members live a level or two down (`crates/core`, `services/api/worker`);
/// past that the walk costs more than it proves.
const MEMBER_MANIFEST_DEPTH: usize = 3;

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 Traverse every declared workspace member

When a workspace member is four or more directories below the Cargo root, this fixed-depth walk never inspects its manifest, so an absolute or symlink-escaping path dependency there still compiles foreign source while provenance reports an exact snapshot. Fresh evidence beyond the earlier workspace-manifest finding is the current hard-coded depth of 3; Cargo accepted and compiled a member at a/b/c/d in an offline check. Enumerate the actual workspace members instead; Cargo documents that cargo metadata --no-deps outputs workspace members without fetching dependencies.

Useful? React with 👍 / 👎.

Comment thread src/checks/cargo.rs
Ok(false) => Ok(None),
Err(err) => Err(err),
};
let manifest = read(at(&relative, "Cargo.toml")).map_err(|_| ())?;

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 member manifests in stale-lock detection

When a workspace member changes a direct dependency requirement without regenerating the root Cargo.lock, this reads only the Cargo-root manifest, so lock_covers_manifest can still classify the substrate as pinned and omit the daily freshness stamp. The Cargo commands do not pass --locked—which Cargo defines as asserting that the lockfile remains unchanged—so the first run may resolve from the moving registry and then cache that result permanently by commit. Fresh evidence beyond the prior requirement-comparison finding is that the current implementation still never reads member manifests here.

Useful? React with 👍 / 👎.

Comment thread src/checks/mod.rs
false if borrows_local_dependencies(&repo, consumable) => {
TreeState::SnapshotBorrowedDeps
}
false => TreeState::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.

P2 Badge Refuse source symlinks that escape snapshots

When the reviewed commit tracks an absolute source symlink such as src/lib.rs pointing outside the repository, the snapshot remains status-clean and this classifies it as exact even though Cargo follows the link and compiles the external bytes; changing those bytes does not alter the commit-based cache key. Fresh evidence beyond the manifest and dirty-symlink fixes is that a clean tracked source symlink never reaches the dirty-path fingerprint or the manifest/lock containment checks. Detect escaping tracked symlinks or classify the substrate as non-hermetic instead of snapshot.

Useful? React with 👍 / 👎.

Comment thread src/checks/mod.rs
working_tree_is_dirty(&repo, SNAPSHOT_SCAFFOLDING).map(|dirty| match dirty {
true => TreeState::SnapshotDirty,
false if borrows_local_dependencies(&repo, consumable) => {
TreeState::SnapshotBorrowedDeps

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 Disable downgrades for borrowed dependency snapshots

When an off-HEAD JavaScript review links the checkout's node_modules, this state correctly records snapshot-borrowed-deps, but CleanComparison::applies_to still whitelists ESLint and Stylelint solely by check ID via check_scans_target_snapshot and never consults the check's provenance. A local plugin or rule version can therefore create findings in unchanged files that are downgraded as pre-existing, potentially allowing a PASS for a dependency-changing target. Fresh evidence beyond the earlier substrate-label fix is that the new label is serialized but is not used by the downgrade predicate; require an exact snapshot state before applying that downgrade.

Useful? React with 👍 / 👎.

Comment thread src/artifacts/mod.rs
Comment on lines +1240 to +1241
"target_sha": prov.and_then(|p| p.target_sha.as_deref()),
"tree_state": prov.and_then(|p| p.tree_state).map(|s| s.as_str()),

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 Record the tree scanned by ghost-reference checks

When a remote or off-HEAD review produces a ghost_refs warning, that synthetic check is included in all_checks with provenance: None, so these fields are emitted as null even though generate_ghost_refs actually walks repo.path()—the operator's local checkout, not the reviewed target snapshot. This can make a target deletion's gating signal depend on unrelated local files while the mandatory provenance row gives no way to detect it. Fresh evidence beyond the Loctree provenance fix is the separate ghost_refs result in src/artifacts/signal/ghost_refs.rs, which still scans a real tree without recording it; run it on the target substrate or populate its actual local-tree provenance.

Useful? React with 👍 / 👎.

m-szymanska pushed a commit that referenced this pull request Aug 22, 2026
…trate)

Merge, not rebase: the 44 resolved review threads on this PR are anchored
to their commits.

One conflict, CHANGELOG.md, and it was a both-added `[Unreleased]`: the
two campaigns wrote the section independently from the same empty base.
Resolved as a union, every bullet from both sides kept verbatim and
re-sectioned as Added / Changed / Fixed / Security -- 3 Added (1 from the
verdict line, 2 from substrate), 11 Changed, 37 Fixed (24 + 13), 1
Security. The ammonia entry is main's single canonical one, not a second
copy. The loctree-bump bullet, inherited by both sides from the merge
base, is deduplicated back to one.

Main's copy also carried a committed conflict marker inside `[Unreleased]`
-- a literal `||||||| 5177ace` line followed by a duplicate `### Changed`
block. Resolving this file into a state that still contained a stray
marker was not an option, so the marker and its duplicate block are gone.
The released history below `[Unreleased]` is byte-identical to main,
including #19's expanded 0.6.0 loctree entry.

Everything else auto-merged. Cargo.lock and Cargo.toml come out
byte-identical to main (ammonia 4.1.4): this branch changes no
dependency. The shared code and docs carry both behaviours -- the
substrate/provenance work and the verdict-vocabulary, needle-edge,
declaration-cap and test-context work sit in different functions and
different sections.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
m-szymanska pushed a commit that referenced this pull request Aug 22, 2026
The conflict resolution in 48679c7 kept the diff3 base section
(||||||| 5177ace and a duplicated loctree 'Changed' bullet) inside the
retained content. Remove the marker and the duplicate; no entry lost.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nqgpf1YwXw2m3vqC8Ur8nr
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