feat(probe-pin): probe-manifest runner that pins comment claims to measured evidence

[lgray]

🤖 AI text below 🤖

Summary

Adds probe-pin, a standalone workspace tool that turns behavioural prose in code comments into a transcription of measured evidence. It runs a TOML probe manifest — each probe bind-mounts a mutated copy of a source file over the real path inside an unprivileged mount namespace, runs a target test binary, reads libtest JSON records — and renders the resulting verdict table into a digest-stamped comment block that --check can re-verify for drift.

The tool eats its own dogfood: its verification matrix is executed by running probe-pin on itself, and cargo probe-pin check on its own manifest is part of the gate battery below.

Files changed

• crates/probe-pin/ — new crate: manifest schema + validation, mutant materialization, namespace isolation, libtest-record verdicts, digest/render/splice/check, CLI (src/, 9 modules, 2,686 lines)
• crates/probe-pin/tests/ — Tier-1 verification matrix and Tier-2 isolation suite (86 tests), plus fixture manifests
• docs/probe-pin.md — usage, schema, isolation model, and the tool's stated non-guarantees
• .cargo/config.toml — one alias line (probe-pin)
• Tiltfile — probe-pin-check resource so --check has an executor, and probe-pin-e2e so the Tier-2 suite runs in the venue that has the capability CI lacks
• Cargo.lock — purely additive

Root Cargo.toml is not edited: the workspace globs members = ["crates/*"] (Cargo.toml:5), so the crate joins automatically. No build.rs.

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer

Plan → review-engine-plan (3 rounds) → implement → review-impl → fix → review-impl → fix → review-impl → class-closure pass. Where a pipeline leg is card-parser-specific (card-data projection, parse-diff), it is replaced by this tool's own gate battery — disclosed rather than skipped, because the crate contains zero engine or parser code.

The third review round is why the last commit exists, and the reason is worth stating rather than burying: rounds 1–3 each found instances of a single defect class — the tool renders a green row, or exits 0, for a run it never measured — and each round fixed the fields named while the next round found more in different fields. That pattern is evidence the class was not bounded by point review, so the remedy was a closure pass rather than a fourth round. See Scope Expansion.

CR references

None. probe-pin contains no game logic; a CR annotation here would be false provenance.

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.
• Gate A output below is for the current committed head.
• Final review-impl below is stated for the current committed head.
• Both anchors cite existing analogous code at the same seam.

All measured after the rebase onto upstream/main cc7668062, at head 8f2836349:

• cargo fmt --all -- --check — exit 0
• cargo clippy --workspace --all-targets -- -D warnings — exit 0, zero warning lines
• cargo test -p probe-pin — exit 0: pure_logic 68 passed / 1 ignored, isolation_e2e 0 passed / 17 ignored, 0 failed
• cargo test -p probe-pin --test isolation_e2e -- --ignored — exit 0: 17 passed, 0 failed (the Tier-2 suite actually executed, locally, where user namespaces work — proof the #[ignore] hides no failure)
• cargo probe-pin check crates/probe-pin/tests/fixtures/dogfood.toml — exit 0 (the tool run against its own manifest)
• git diff --numstat upstream/main..HEAD — 31 files, 7,792 insertions, 0 deletions, 0 binary files (positive control: a synthesized NUL file returns - -)

Ignored tests, disclosed in detail because in this crate an ignored test is exactly the defect the tool exists to catch. There are 18, of two entirely different kinds:

• 1 in pure_logic — ppfixture_ignored, a probe target whose body must never run. It is the fixture that drives the execution-floor verification below, not a skipped assertion.
• 17 in isolation_e2e — the whole Tier-2 suite, because CI cannot execute it. See CI Failures below; it is measured, not assumed, and the suite is verified green locally with -- --ignored.

Neither is a runtime capability check, deliberately: a test that silently no-ops when the namespace is missing reports as passed, which is this PR's own BLOCKER wearing a test harness. An ignored test reports as ignored.

Two-sided verification. Every row of the verification matrix has a DROP arm and a TRIVIALIZE arm, and each arm is stated in the test's own doc comment with what it must flip. A row whose DROP arm does not flip its assertion is vacuous — and in a tool whose thesis is that evidence must be measured rather than asserted, a vacuous test is self-refuting.

Scope of that claim, stated precisely rather than as one headline number: the full mutant matrix was executed against the rows that existed at the time, at the head where each was written. The rows added by the last two commits were each measured individually at their own head — every new guard reverted and its test observed to fail with the specific diagnostic it exists to produce, every mutated source restored and verified byte-identical with sha256sum -c. The matrix has not been re-run wholesale at the current head; cargo test -p probe-pin has, and is green.

Gate A

$ ./scripts/check-parser-combinators.sh "$(git rev-parse upstream/main)"
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=8f283634934b04b848122956d8211b8aa54df914 base=cc76680623e4d4628dc0291e88cc9c2d99bf32ff

The base is passed explicitly. The script defaults to git merge-base origin/main HEAD (:48), and origin here is a fork whose main lags; that default resolved to a different, shorter range. The run above is against the authoritative base.

Disclosure, because a green line here would otherwise be read as more than it is: scripts/check-parser-combinators.sh scopes to SCOPE='crates/engine/src/parser' (:51). This PR touches no file under that path, so the gate passes on an empty range. It is not-applicable-but-green, not evidence of combinator purity. The combinator mandate does not apply to this crate: its find is literal byte substitution, and the anchor lint is a denylist over user-authored manifest text, not parsing dispatch.

Anchored on

• crates/engine-inventory-gen/Cargo.toml:2 and src/main.rs:1 — the workspace's existing standalone tooling-crate pattern: version.workspace/license.workspace/edition 2021 manifest shape, and a //! header naming the invocation that regenerates its output. probe-pin mirrors both.
• Tiltfile:199 — the existing local_resource lint pattern (list-form cmd, separate CARGO_TARGET_DIR, narrow deps, TMP_IGNORE, auto_init, allow_parallel, labels). probe-pin-check follows it field for field, extending ignore because TMP_IGNORE is a filename glob (**/*.tmp.*) that does not match a tmp/ directory.

Honest scope note on Gate B: this is a new crate, so there is no prior art inside crates/probe-pin/. The two citations above are the nearest same-class analogs in the repository, and the plan traced the first end-to-end before adopting its conventions. Flagging rather than citing something closer-looking but unrelated.

Final review-impl

Round 3 was NOT clean — 1 BLOCKER, 3 MAJOR, 3 MINOR at head 08924b0ad, all reproduced against the built binary. Stating that plainly rather than reporting only the pass that followed.

Every one of the green-row findings was the same class. The remedy was therefore an authorized class-closure pass (final commit), not a fourth review round, and its acceptance gate was three measurements:

• (a) each of the four execution surfaces refuses a hostile value, two-sided — hostile aborts naming the offending construct, benign still passes. Measured for argv, env, path and anchor.
• (b) the execution floor fires on a zero-run — the BLOCKER's exact repro, flipped: an #[ignore]d pinned test now aborts instead of rendering a green row. Measured for both the #[ignore] and --bench routes, with the benign arm still passing.
• (c) the filesystem-write producer census is complete — every producer proven to route through the containment guard or to be structurally outside manifest control, with the enumerating command published so the census is reproducible.

All three measured clean at the committed head, independently re-run by the driver with exit codes captured before any pipe.

Maintainer review — marker injection (resolved at head 9104cef64)

matthewevans blocked on: a generated block can inject a second marker. Two acceptable remedies were named; both are implemented, because measuring the first exposed a residual the second closes.

Manifest-derived fields were already refused at the head the review was written against, verified by re-running the four attack strings through the built binary — each aborts at exit 2 before any write: anchor = "\nPROBE-PIN:END" (the review's exact string) and anchor = "boom\nsecond line" abort naming the control character; marker = "PROBE\nPIN" and a projection sentence containing a marker tag abort naming the tag.

That coverage is field-shaped, and the same field-vs-surface reasoning that drove the class-closure pass applies here: two producers reach a rendered line without passing through a manifest field — an instrument's --version output, and the provenance paths lifted from the target's own panic text. Neither can be refused in advance. So block::splice now re-runs locate over its own spliced output and returns an abort rather than a block whose marker span the next check could not find. The property is unconditional at the write surface instead of conditional on field coverage.

Regression test write_can_never_produce_a_block_its_own_check_cannot_locate asserts both halves the review asked for: manifest::validate_block_text refuses the injected marker, and splice refuses to return an unlocatable block. Two-sided — deleting the postcondition line makes it fail at exit 101 naming the injected second END; the mutated source was restored and re-verified with sha256sum -c.

The census that premise rests on is now executable, not asserted. "Unconditional at the write surface" is only true while splice is the sole route to the pinned file's bytes. Measured: exactly two filesystem write producers exist in src/ — the scratch mutant, which is never a rendered block, and the pinned file, whose content argument is literally the splice(..)? call. The guard is inside the only write's argument, not merely on its path. every_write_of_the_pinned_file_routes_through_splice pins that, and a second fs::write of output.file now fails it rather than silently falsifying this paragraph.

Four arms, each flipping a different assertion: dropping splice from the write argument → exit 101 on the property; adding a third producer → exit 101 at found 3; breaking the search patterns → exit 101 on the zero-census positive control. The fourth arm did not flip, and the comment says so: the comment-strip's DROP arm left the test green, because this comment's own first draft mis-cited manifest.rs's backticked prose mention as a matching hit when every producer pattern requires an open paren. The strip is disclosed as defensive, and its value re-measured against the input it actually defends — a doc comment quoting a call site, green with the strip and found 3 without.

Second review round (resolved at head 815aaf279, census widened at 8f2836349)

Five findings; four real, one declined with evidence. Each was verified against a run before anything was edited.

• Rows are keyed on probe identity. firing_cell decided "this is the control" from mounts_reached == 0 while mutation_cell decided the same fact from probe.mutations.is_empty() — two discriminators for one concept, agreeing only because validate refuses an empty mutation file list. Latent, and the digest pins the same count, so nothing would have disagreed with a lying row. Rendered output byte-identical: the committed block is unchanged in git and check exits 0.
• An outcome-less probe gets a visible unmeasured row, matching what digest already records for the same input. Skipping it showed N-1 rows for N declared probes while check stayed CLEAN, because the committed copy omitted the same row.
• A blank [target].filter is refused. Measured with filter_match = "substring": the run widens to the whole target binary and is caught only because a widened test happens to fail — reported as a broken control, not as the manifest defect. With exact it matches nothing and the execution floor blames the target for a typo in the manifest.
• probe-pin-e2e gets its own CARGO_TARGET_DIR, having shared one with probe-pin-check while both watch crates/probe-pin/ and both allow parallel runs. The contention is cross-process, so the in-process mutex in the suite does not span it.

Declined, with measurement: a finding that validate admits identity mutations. It does; the refusal is not in validate. find == replace, repeat = 0 and text = "" each abort at exit 2 on "the materialized mutant is byte-identical to the original" — one guard after materialization, covering the class regardless of mutation kind, and find = "" is refused by the occurrence gate. The proposed fix would have added narrower per-variant checks at a worse surface, which is the field-enumeration shape this crate already repaired twice.

One of my own fixes was wrong and the suite caught it. The first attempt at the missing-row finding used unreachable!, as suggested. It panicked in the projection-surface test, which calls render with &[] outcomes deliberately — render is public and that input is legitimate, so the invariant belongs to main's call order, not to render. The visible unmeasured row closes the actual finding without breaking correct use. A second test listed "" among benign filter values; it now asserts which guard refuses it, so the benign list still proves the operand rule is discriminating.

The writer census was widened after an independent re-run. A reviewer ran it with a wider producer set (fs::copy, fs::rename added) and got the same answer — but the agreement was luck: neither appears in any src/ module today, so a future fs::copy(tmp, out_path) would have written the pinned file while the census still reported "exactly 2". The patterns are now live rather than decorative: injecting an fs::copy into block.rs fails the census at found 3, naming the module, where before the widening it was invisible.

Claimed parse impact

None. No engine or parser source is touched, so no card parse is affected.

Scope Expansion

Not "None" — disclosed in full.

The delivered crate is 6,990 LOC (2,686 src/ + 4,304 tests/) against a planned ~1,600. The estimate was wrong, not the code: the dominant drivers are rustfmt physical lines against logical-line estimates, verbatim failure-message texts, and two-sided arms on every verification row. Tests were deliberately not trimmed to hit the estimate — cutting evidence to match a projection is the exact failure this tool exists to prevent.

Thirteen items were built outside the original plan's inventory. An independent review adjudicated them 4 necessary, 9 defensible seams, 0 scope creep. The two load-bearing ones:

• isolate::exit_rc — required. timeout re-raises the child's signal, so ExitStatus::code() is None on an abort, making the plan's own rc-134 contract unreachable without a 128+signal mapping. Measured 134/139/101/124.
• The mount script's indexed loop — the plan's script body was written for one mutant/target, but a five-file mount is an expressible probe. Closed with ${!m} indirection, no eval; "$@" still reaches the target as exactly its argv (verified under spaces, *, $HOME, quotes, and empty strings).

One planned mechanism was not shipped (isolate::capability()): the pipeline has no step for it and the namespace-unavailable abort is reachable through the ordered classification.

The class-closure pass (final commit)

Rounds 1–3 each fixed the fields a reviewer named, and the next round found the same defect in a different field. The closure diagnosis: the crate validated FIELDS, but every defect entered through a SURFACE. Fields are unbounded and grow with the schema; surfaces are few and each has exactly one construction site. That is why a gate written for [target].args missed [target].filter, and a path guard written for mutation.file missed [output].file.

The four surfaces a manifest value can reach are argv, env, filesystem path, and anchor semantics. Validation moved to each surface's construction site.

The general guarantee is the execution floor. observe took test_count from the suite-started record and discarded passed/failed/ignored from the terminal record, so the "this run measured nothing" guard was structurally blind to a run whose every selected test was skipped — a normal run and an all-ignored run both report test_count 1. The floor is now passed + failed == 0. It caught --bench without --bench ever being enumerated, and it closes the route that needed no manifest change at all: a plain #[ignore] on the pinned test, which kept the digest, the block and the Tilt resource green indefinitely.

Deliberately not done: adding --bench (or --ignored, --list, --logfile) to the reserved-flag list. Enumerating hostile flags is the denylist shape that had already failed twice in this crate. The argv work instead closes the field gap structurally and constrains filter by positive shape — a filter is a substring, a projection path is a directory, so neither may begin with -.

Two findings surfaced by the closure work itself and fixed in it:

• A live false green. [[projection]].paths = ["-h"] made ast-grep print its help, which probe-pin counted into a rendered "named at 30 sites." sentence at exit 0. Same argv surface, different construction site — closed by the same positive shape rule, which is the argument for writing it that way.
• Path containment was the wrong shape, not merely under-applied. is_workspace_relative was lexical, so a symlink whose every component is an ordinary name escaped it — measured: the block spliced to a destination outside the workspace at exit 0. Replaced with a realpath assertion matching the pattern isolate::scratch_dir already used, closing the probe-id, [output].file and mount-target producers together. Not reachable in this repo today (4 tracked symlinks, all in-tree), so this is hardening — but it is the shape that let three consecutive rounds each patch one more field.

Validation Failures

None.

CI Failures

One disclosed risk materialized on the first CI run and has been remedied; one remains open.

1. RESOLVED — the Tier-2 suite cannot run on GitHub runners. This was disclosed here as a risk with the remedy deliberately deferred until a log existed to name the cause. The log exists: actions run 31646292055 — unshare --map-root-user --mount fails with write failed /proc/self/uid_map: Operation not permitted, and 14 of the 15 tests then in the suite failed for that one reason, across all four shards. The Rust (fmt, clippy, test, coverage-gate) job is an aggregator reporting RUST_TEST_RESULT: failure, not an independent failure.

   Remedied by #[ignore] on the suite, with the cause and the local invocation recorded in the file header and docs/probe-pin.md. Ignored whole rather than per failing test: the lone survivor (proj_missing_both_paths) passes only because its manifest is refused at validation before unshare is reached — a property of that fixture, not of the test. Splitting on "does this manifest happen to abort early" is a boundary that moves silently the next time a fixture or refusal order changes.

   Two controls keep the accommodation from decaying: the Tilt probe-pin-e2e resource runs the suite with --ignored in the local venue, which has the capability, so "ignored in CI" means "run elsewhere" and not "never run"; and pure_logic::tier2_suite_is_uniformly_ignored_with_the_measured_reason pins the suite size, the attribute on every test, and the exact reason text, so a new Tier-2 test cannot silently join and the ignored set cannot silently widen. Two-sided: removing one attribute and rewording one reason each fail naming isolation_e2e.rs:130; adding an 18th test fails the size assertion instead.

   What CI still covers: all 68 executed pure_logic tests — schema, validation, verdict, digest, drift, and every surface refusal — but never a real mount. Stated plainly rather than left implied.

2. probe-pin-check cannot be verified from a worktree. Tilt watches the main checkout, so a Tiltfile edit in a worktree is not what Tilt evaluates — that is a "cannot answer", not a failure. Verified instead by Starlark parse plus running the resource's cmd directly (exit 0). Closing the Tilt-side gate is owned by me, post-merge.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6124919c-0849-4c5b-9a79-613094bb22ae

📥 Commits

Reviewing files that changed from the base of the PR and between 2a47fca and 815aaf2.

📒 Files selected for processing (4)

• Tiltfile
• crates/probe-pin/src/block.rs
• crates/probe-pin/src/manifest.rs
• crates/probe-pin/tests/pure_logic.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• Tiltfile
• crates/probe-pin/src/block.rs

---

📝 Walkthrough

Walkthrough

The PR adds the probe-pin crate and CLI. It validates manifests, isolates mutated test runs, evaluates probe verdicts, generates digest-stamped blocks, detects drift, and adds fixtures, tests, documentation, Cargo wiring, and Tilt automation.

Changes

Probe-pin execution and validation

Layer / File(s)	Summary
Manifest contracts and execution preflight crates/probe-pin/Cargo.toml, crates/probe-pin/src/lib.rs, crates/probe-pin/src/manifest.rs, crates/probe-pin/src/target.rs	Defines manifest schemas, validation rules, error types, workspace checks, and test-binary resolution.
Mutation materialization and isolated execution crates/probe-pin/src/mutate.rs, crates/probe-pin/src/isolate.rs, crates/probe-pin/src/project.rs	Applies sequential mutations, creates bind mounts, controls child arguments and environment, fingerprints files, and runs projection tools.
CLI orchestration and probe verdicts crates/probe-pin/src/main.rs, crates/probe-pin/src/verdict.rs	Runs control and mutation probes, parses libtest output, evaluates expectations, checks probe discrimination, and reports failures.
Digest, block rendering, and drift checks crates/probe-pin/src/block.rs, crates/probe-pin/src/main.rs	Generates deterministic marked blocks, compares committed output, classifies drift, and controls safe splicing.
Fixtures, end-to-end coverage, and project wiring crates/probe-pin/tests/*, docs/probe-pin.md, .cargo/config.toml, Tiltfile	Adds validation and isolation fixtures, integration tests, usage documentation, a Cargo alias, and automated Tilt checks.

Estimated code review effort: 5 (Critical) | ~120 minutes

Mergeability Score: 🟠 High · up to 815aa

The current implementation can still produce misleading pinned evidence or reject valid projects: no-op mutations may be reported as measured mutants, malformed markers can break comment extraction, symlinked worktrees can fail incorrectly, and parallel test execution can contend on shared fixtures. Because these issues affect the correctness and reliability of generated claims, the change is not merge-ready until they are fixed or explicitly accepted.

Sequence Diagram(s)

sequenceDiagram
  participant ProbePinCLI
  participant isolate
  participant TestBinary
  participant verdict
  ProbePinCLI->>isolate: Execute control and mutation probes
  isolate->>TestBinary: Run with mounts, arguments, and environment
  TestBinary-->>isolate: Return output and exit status
  isolate-->>ProbePinCLI: Return Run result
  ProbePinCLI->>verdict: Parse output and evaluate expectations
  verdict-->>ProbePinCLI: Return verdict or mismatch

Loading

Suggested labels: enhancement, quality

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the new probe-manifest runner and its evidence-pinning purpose.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 9

🧹 Nitpick comments (5)

crates/probe-pin/src/mutate.rs (1)

62-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Bound Prepend::repeat at validation time.

text.repeat(*repeat as usize) allocates text.len() * repeat bytes. The schema accepts any u32, so a manifest declaring a large repeat with a non-trivial text terminates probe-pin through allocation failure. That path produces no Abort, so the operator gets no named reason and no remedy. Every other hostile manifest value in this crate is refused by name before a target runs.

Add the product bound to manifest::validate, next to the existing per-mutation empty-file-list refusal, and keep the cast off the manifest value.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/mutate.rs` around lines 62 - 75, Add validation in
manifest::validate for Mutation::Prepend::repeat using the product bound needed
to prevent text.repeat allocation failures, alongside the existing
empty-file-list validation. Reject oversized values with a named error and
remedy, and update the mutation execution path to use the validated bounded
value without casting the raw manifest repeat.

crates/probe-pin/src/block.rs (2)

138-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Replace the _ arms with exhaustive matches over Verdict.

mounts_reached and provenance both use _ => over Option<&Verdict>. A new Verdict variant compiles silently and serializes as null in both fields, so the digest stops pinning its payload. Match Some(Verdict::Pass { .. }), Some(Verdict::Fail { .. }) and None explicitly, as the verdict field above already does.

As per coding guidelines: "wildcard _ match arms where the enum is known and an exhaustive match would let the compiler catch missing variants" is a finding.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/block.rs` around lines 138 - 145, Replace the wildcard
arms in the mounts_reached and provenance matches with explicit Option<&Verdict>
cases: handle Some(Verdict::Pass { .. }), Some(Verdict::Fail { .. }), and None.
Preserve the current extracted values and None results while making future
Verdict variants compiler-visible, matching the exhaustive handling used by the
verdict field.

Source: Coding guidelines

---

362-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The Instrument variant list is duplicated here and in used_instruments.

instrument_lines hard-codes [Instrument::Toolchain, Instrument::AstGrep]. A new Instrument variant compiles without a change here. Its committed // instrument <tool> = <version> line is then never parsed, changed_instruments never sees a move for it, and write_refusal silently stops covering it. Expose one authoritative list on the enum and read it from both call sites, so the compiler reports a missing entry.

♻️ Proposed refactor

// in the module that defines `Instrument`
impl Instrument {
    pub const ALL: [Instrument; 2] = [Instrument::Toolchain, Instrument::AstGrep];
}

-            [Instrument::Toolchain, Instrument::AstGrep]
-                .into_iter()
+            Instrument::ALL
+                .into_iter()
                 .find(|i| i.tool() == name)
                 .map(|i| (i, version.to_string()))

As per coding guidelines: "any new helper that duplicates an existing building block" is a finding.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/block.rs` around lines 362 - 375, Add an authoritative
`Instrument::ALL` collection in the enum’s implementation containing every
variant, and update `instrument_lines` to iterate over it instead of its
hard-coded variant array. Reuse the same collection in `used_instruments`,
preserving both callers’ existing behavior so adding a new variant requires
updating one compiler-checked list.

Source: Coding guidelines

crates/probe-pin/tests/pure_logic.rs (1)

1729-1736: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The reach guard asserts libtest's exact JSON spacing.

run.stdout.contains("\"test_count\": 1") requires one space after the colon. libtest's JSON format is an unstable surface, reached here through -Z unstable-options. If a future libtest emits compact JSON, this reach guard fails with "libtest must still report test_count 1, or this test pins nothing" — a message that reads as a regression in the execution floor when the real cause is a formatter change. The synthetic stream helper in this same file already writes the compact form "test_count":{n}, so both spellings exist in the file.

Read the field instead of matching bytes.

♻️ Proposed fix

     for (name, run) in [("ignored", &ignored), ("--bench", &benched)] {
+        let started = run
+            .stdout
+            .lines()
+            .filter_map(|l| serde_json::from_str::<serde_json::Value>(l).ok())
+            .find(|v| v["type"] == "suite" && v["event"] == "started")
+            .unwrap_or_else(|| panic!("{name}: no suite-started record: {}", run.stdout));
         assert!(
-            run.stdout.contains("\"test_count\": 1"),
+            started["test_count"] == 1,
             "{name}: libtest must still report test_count 1, or this test pins nothing: {}",
             run.stdout
         );

This needs serde_json as a dev-dependency of the crate; verdict::observe already parses these records, so an exported helper would avoid the direct dependency.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/tests/pure_logic.rs` around lines 1729 - 1736, Update the
reach-guard assertions in the loop over “ignored” and “--bench” to parse
run.stdout as JSON and inspect the numeric test_count field instead of matching
formatted bytes. Add the required serde_json dev-dependency, or reuse an
exported helper such as verdict::observe if that avoids the direct dependency,
while preserving the existing success and test_count validation.

crates/probe-pin/tests/fixtures/astgrep_six.sh (1)

1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove or use astgrep_six.sh in a test. The fixture is not referenced by the supplied tests. pure_logic::proj_missing uses astgrep_broken.sh and astgrep_three.sh, while pure_logic::instrument_skew and pure_logic::write_refusal generate the six-match case in skew_arms().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/tests/fixtures/astgrep_six.sh` around lines 1 - 7, Remove
the unused astgrep_six.sh fixture, or add it to a test that specifically
exercises its six-match behavior. Prefer removal unless an existing test can
reuse it without duplicating the cases generated by pure_logic::instrument_skew
and pure_logic::write_refusal through skew_arms().

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/probe-pin/src/block.rs`:
- Around line 178-180: Make the anchor rendering path marker-safe before it
reaches cell: validate anchors to reject control characters and PROBE-PIN marker
text, or escape them so they cannot create extra lines or marker tags. Update
the anchor-specific validation/rendering logic, while preserving existing claim
and id handling and ensuring run --write emits exactly one END marker.

In `@crates/probe-pin/src/isolate.rs`:
- Around line 54-89: Define a separate constant for the unconditional owned flag
sequence in addition to OWNED_FLAGS, while keeping --exact in OWNED_FLAGS as the
conditional flag. Update argv to iterate over the new unconditional constant
instead of duplicating the inline list, preserving the existing conditional
--exact insertion and argument order.

In `@crates/probe-pin/src/main.rs`:
- Around line 101-118: Canonicalize the workspace root before comparing it with
the canonicalized manifest path in the manifest_rel flow. Update the root
obtained from target::workspace_root once, preserve the existing containment
error handling, and use that canonical root for subsequent root.join and
current_dir operations.

In `@crates/probe-pin/src/manifest.rs`:
- Around line 414-420: Extend validate to apply producer-level checks to every
Projection: require sentence to contain the "{count}" placeholder, require id to
be a plain name, reject duplicate projection ids, reject empty or
whitespace-only anchors as applicable to projection paths, and reject empty
paths. Keep these validations before any target execution and align their
behavior with the existing probe validation rules and error handling.
- Around line 105-110: Update Output validation to reject marker values whose
trimmed content is empty, while preserving non-empty markers as provided.
Implement this in the existing validate flow for Output, before block::locate
constructs structural tags, using the existing validation error conventions.

In `@crates/probe-pin/src/verdict.rs`:
- Around line 41-75: Update observe to reserve and check a distinct run.rc value
for script-level failures before the stream/marker classification, returning the
typed Abort variant for script execution failure with the probe and trimmed
stderr. Ensure this check covers non-zero script exits with empty stdout,
including missing PATH commands or bash failing before exec, and preserves the
existing mount, timeout, and isolation precedence.

In `@crates/probe-pin/tests/fixtures/projection.toml`:
- Around line 14-16: Update the [output].file entries in
crates/probe-pin/tests/fixtures/projection.toml#L14-L16 and
crates/probe-pin/tests/fixtures/treewrite.toml#L20-L22 to use separate paths
under crates/probe-pin/tests/fixtures/tmp/, ensuring neither fixture writes the
committed dogfood_block.md; no other changes are required.

In `@crates/probe-pin/tests/isolation_e2e.rs`:
- Around line 576-593: Add an assertion after the chained replacements building
base and before tmp_manifest that verifies base differs from the original
dogfood manifest content, matching the existing edit != base reach-guard
pattern. Keep the current benign-arm execution and assertion unchanged.
- Around line 86-101: Add a probe-pin-serial test group with max-threads = 1 in
.config/nextest.toml and assign the probe-pin isolation tests that invoke
probe_pin_bin to it. Update the comment near SERIAL to clarify that the mutex
only serializes tests within a process, while the nextest group provides
cross-process serialization.

---

Nitpick comments:
In `@crates/probe-pin/src/block.rs`:
- Around line 138-145: Replace the wildcard arms in the mounts_reached and
provenance matches with explicit Option<&Verdict> cases: handle
Some(Verdict::Pass { .. }), Some(Verdict::Fail { .. }), and None. Preserve the
current extracted values and None results while making future Verdict variants
compiler-visible, matching the exhaustive handling used by the verdict field.
- Around line 362-375: Add an authoritative `Instrument::ALL` collection in the
enum’s implementation containing every variant, and update `instrument_lines` to
iterate over it instead of its hard-coded variant array. Reuse the same
collection in `used_instruments`, preserving both callers’ existing behavior so
adding a new variant requires updating one compiler-checked list.

In `@crates/probe-pin/src/mutate.rs`:
- Around line 62-75: Add validation in manifest::validate for
Mutation::Prepend::repeat using the product bound needed to prevent text.repeat
allocation failures, alongside the existing empty-file-list validation. Reject
oversized values with a named error and remedy, and update the mutation
execution path to use the validated bounded value without casting the raw
manifest repeat.

In `@crates/probe-pin/tests/fixtures/astgrep_six.sh`:
- Around line 1-7: Remove the unused astgrep_six.sh fixture, or add it to a test
that specifically exercises its six-match behavior. Prefer removal unless an
existing test can reuse it without duplicating the cases generated by
pure_logic::instrument_skew and pure_logic::write_refusal through skew_arms().

In `@crates/probe-pin/tests/pure_logic.rs`:
- Around line 1729-1736: Update the reach-guard assertions in the loop over
“ignored” and “--bench” to parse run.stdout as JSON and inspect the numeric
test_count field instead of matching formatted bytes. Add the required
serde_json dev-dependency, or reuse an exported helper such as verdict::observe
if that avoids the direct dependency, while preserving the existing success and
test_count validation.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b1551b4-7187-450f-af2d-d2620a6ab516

📥 Commits

Reviewing files that changed from the base of the PR and between 479ad39 and ed64153.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (30)

• .cargo/config.toml
• Tiltfile
• crates/probe-pin/Cargo.toml
• crates/probe-pin/src/block.rs
• crates/probe-pin/src/isolate.rs
• crates/probe-pin/src/lib.rs
• crates/probe-pin/src/main.rs
• crates/probe-pin/src/manifest.rs
• crates/probe-pin/src/mutate.rs
• crates/probe-pin/src/project.rs
• crates/probe-pin/src/target.rs
• crates/probe-pin/src/verdict.rs
• crates/probe-pin/tests/fixtures/astgrep_broken.sh
• crates/probe-pin/tests/fixtures/astgrep_six.sh
• crates/probe-pin/tests/fixtures/astgrep_three.sh
• crates/probe-pin/tests/fixtures/collide.toml
• crates/probe-pin/tests/fixtures/compiled.toml
• crates/probe-pin/tests/fixtures/control_expect_fail.toml
• crates/probe-pin/tests/fixtures/control_fails.toml
• crates/probe-pin/tests/fixtures/dogfood.toml
• crates/probe-pin/tests/fixtures/dogfood_block.md
• crates/probe-pin/tests/fixtures/p7.toml
• crates/probe-pin/tests/fixtures/p8.toml
• crates/probe-pin/tests/fixtures/prod.txt
• crates/probe-pin/tests/fixtures/projection.toml
• crates/probe-pin/tests/fixtures/treewrite.toml
• crates/probe-pin/tests/fixtures/unsorted_ids.toml
• crates/probe-pin/tests/isolation_e2e.rs
• crates/probe-pin/tests/pure_logic.rs
• docs/probe-pin.md

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

strip_prefix compares a canonicalized manifest path against a non-canonical root, so a symlinked workspace path aborts a valid run.

target::workspace_root returns the parent of the cargo locate-project output without canonicalizing it. Line 107 canonicalizes the manifest path. If any component of the workspace path is a symlink, the two strings do not share a prefix, strip_prefix returns Err, and the pipeline aborts with "manifest … is not inside the workspace root". That statement is false, and no manifest change can satisfy it.

The other two containment assertions in this crate canonicalize both sides: isolate::scratch_dir canonicalizes the scratch dir and the workspace, and manifest::resolve_contained canonicalizes base. Apply the same rule here. Canonicalizing root once also keeps root.join(&m.output.file) and the ast-grep current_dir(root) consistent with it.

🐛 Proposed fix

     // 2. workspace root
-    let root = target::workspace_root()?;
+    // Canonicalized at the source: `manifest_rel` below and `resolve_contained` both compare
+    // against it after canonicalizing the other side, and a symlinked worktree path otherwise
+    // fails a prefix comparison that is actually satisfied.
+    let root = target::workspace_root()?;
+    let root = root.canonicalize().unwrap_or(root);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let root = target::workspace_root()?;
	// Workspace-relative or nothing: this string reaches the digest AND the BEGIN line, so an
	// absolute path stamps a machine-specific value into a committed artifact — the exact
	// reproducibility the digest exists to provide (docs/probe-pin.md: "Excluded: … absolute
	// paths"), and the treatment `is_workspace_relative` already gives every other path key.
	let manifest_rel = manifest_path
	.canonicalize()
	.ok()
	.and_then(|p| p.strip_prefix(&root).ok().map(Path::to_path_buf))
	.with_context(|| {
	format!(
	"probe-pin: manifest {} is not inside the workspace root {}. Its path is stamped into the block's BEGIN line and into the digest, so an absolute one would pin a block that no other checkout can reproduce. Move the manifest into the workspace and name it relative to the root. Aborting.",
	manifest_path.display(),
	root.display()
	)
	})?
	.display()
	.to_string();
	// 2. workspace root
	// Canonicalized at the source: `manifest_rel` below and `resolve_contained` both compare
	// against it after canonicalizing the other side, and a symlinked worktree path otherwise
	// fails a prefix comparison that is actually satisfied.
	let root = target::workspace_root()?;
	let root = root.canonicalize().unwrap_or(root);
	// Workspace-relative or nothing: this string reaches the digest AND the BEGIN line, so an
	// absolute path stamps a machine-specific value into a committed artifact — the exact
	// reproducibility the digest exists to provide (docs/probe-pin.md: "Excluded: … absolute
	// paths"), and the treatment `is_workspace_relative` already gives every other path key.
	let manifest_rel = manifest_path
	.canonicalize()
	.ok()
	.and_then(|p| p.strip_prefix(&root).ok().map(Path::to_path_buf))
	.with_context(|| {
	format!(
	"probe-pin: manifest {} is not inside the workspace root {}. Its path is stamped into the block's BEGIN line and into the digest, so an absolute one would pin a block that no other checkout can reproduce. Move the manifest into the workspace and name it relative to the root. Aborting.",
	manifest_path.display(),
	root.display()
	)
	})?
	.display()
	.to_string();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/main.rs` around lines 101 - 118, Canonicalize the
workspace root before comparing it with the canonicalized manifest path in the
manifest_rel flow. Update the root obtained from target::workspace_root once,
preserve the existing containment error handling, and use that canonical root
for subsequent root.join and current_dir operations.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect how [output].marker is consumed when constructing and matching block markers.
fd -t f 'block.rs' crates/probe-pin/src --exec rg -n -C4 'marker'
rg -n -C3 'marker' crates/probe-pin/tests

Repository: phase-rs/phase

Length of output: 20028

---

Reject blank [output].marker values. block::locate constructs {marker}:BEGIN and {marker}:END without normalization, so blank or whitespace-only markers produce ambiguous structural tags and can match unrelated lines. Reject markers whose trimmed value is empty during validate.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/manifest.rs` around lines 105 - 110, Update Output
validation to reject marker values whose trimmed content is empty, while
preserving non-empty markers as provided. Implement this in the existing
validate flow for Output, before block::locate constructs structural tags, using
the existing validation error conventions.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for an existing nextest configuration and any current test-group usage.
fd -H -t f 'nextest.toml' . ; rg -n 'test-group|test-groups|max-threads' -- .config .cargo 2>/dev/null

Repository: phase-rs/phase

Length of output: 175

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- nextest config ---'
cat -n .config/nextest.toml
printf '%s\n' '--- cargo config ---'
cat -n .cargo/config.toml
printf '%s\n' '--- isolation test structure ---'
wc -l crates/probe-pin/tests/isolation_e2e.rs
rg -n 'probe_pin_bin|#\[test\]|fixtures/tmp|cargo test|CARGO_TARGET_DIR|root\(' crates/probe-pin/tests/isolation_e2e.rs crates/probe-pin -g '*.rs'
printf '%s\n' '--- package metadata ---'
rg -n 'name\s*=\s*"probe-pin"|nextest|test-all' Cargo.toml crates .cargo .config -g '*.toml' -g '*.md'

Repository: phase-rs/phase

Length of output: 25162

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- tested function names and helper call sites ---'
python3 - <<'PY'
import re
from pathlib import Path
p = Path("crates/probe-pin/tests/isolation_e2e.rs")
lines = p.read_text().splitlines()
for i, line in enumerate(lines):
    if line.strip() == "#[test]":
        j = i + 1
        while j < len(lines) and not lines[j].strip():
            j += 1
        m = re.search(r'fn\s+([A-Za-z0-9_]+)\s*\(', lines[j])
        if m:
            print(f"{i+1}: {m.group(1)}")
PY
printf '%s\n' '--- helper documentation and nearby implementation ---'
sed -n '70,108p' crates/probe-pin/tests/isolation_e2e.rs
printf '%s\n' '--- test bodies that invoke the helper ---'
sed -n '300,465p' crates/probe-pin/tests/isolation_e2e.rs
sed -n '460,730p' crates/probe-pin/tests/isolation_e2e.rs
printf '%s\n' '--- exact nextest-related repository references ---'
rg -n -C 3 'nextest|test-groups|test-group|probe-pin-serial' . -g '!target' -g '!node_modules'

Repository: phase-rs/phase

Length of output: 38277

---

🌐 Web query:

cargo nextest documentation test groups max-threads process per test

💡 Result:

In cargo-nextest, you can control the concurrency of specific sets of tests using test groups and the max-threads parameter [1][2]. Configuration Setup To implement this, you define test groups in your.config/nextest.toml file, specifying a maximum number of concurrent threads allowed for that group [1][2]: [test-groups] my-group = { max-threads = 4 } Assigning Tests After defining the group, you assign tests to it using per-test overrides in the same configuration file. You use a filter string to target specific tests [1][2]: [[profile.default.overrides]] filter = 'test(/^my_test_prefix::/)' test-group = 'my-group' Important Considerations - Concurrency Limits: The max-threads setting for a test group acts as a logical semaphore, limiting the number of tests in that group running concurrently to the specified number [1][2]. - Scope of Exclusion: Test groups provide mutual exclusion only among tests within that specific group [3][4]. They do not prevent tests in the group from running concurrently with tests that are not part of the group [1][3]. - Threads-Required: If you need to manage how many threads a single test consumes (e.g., for heavy tests), you can use the threads-required per-test override [1][5]. This is distinct from test groups; threads-required sets the weight of a single test against the global or group-specific thread pool [1][5]. A test can simultaneously belong to a test group and have a threads-required value assigned to it [1][2]. For more detailed information, refer to the official nextest documentation on test groups [1].

Citations:

• 1: https://nexte.st/docs/configuration/test-groups/
• 2: https://github.com/nextest-rs/nextest/blob/main/site/src/docs/configuration/test-groups.md
• 3: Bug: serial tests start to run before other finish nextest-rs/nextest#2310
• 4: Feature request: Allow tests from different groups to not be interspersed nextest-rs/nextest#2054
• 5: https://nexte.st/docs/configuration/threads-required/

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- nested cargo command and environment handling ---'
sed -n '1,115p' crates/probe-pin/src/target.rs
rg -n -C 8 'Command::new|cargo|CARGO_TARGET_DIR|current_dir|env_clear|envs' crates/probe-pin/src crates/probe-pin/tests/isolation_e2e.rs
printf '%s\n' '--- read-only coverage check for the proposed nextest filter ---'
python3 - <<'PY'
import re
from pathlib import Path
text = Path("crates/probe-pin/tests/isolation_e2e.rs").read_text()
tests = []
for m in re.finditer(r'#\[test\]\s*fn\s+([A-Za-z0-9_]+)\s*\(', text):
    name = m.group(1)
    body_start = m.end()
    next_test = re.search(r'\n#\[test\]', text[body_start:])
    body = text[body_start:body_start + (next_test.start() if next_test else len(text))]
    if 'probe_pin_bin(' in body:
        tests.append(name)
print("tests invoking probe_pin_bin:", tests)
pattern = re.compile(
    r'^(?:mount_reach|pad_reach|tree|drift|no_tests_selected_ordering|'
    r'wiring_|execution_floor_|dogfood_check|proj_missing_both_paths|streams_are_separate)'
)
print("covered by proposed regex:", [n for n in tests if pattern.search(n)])
print("not covered:", [n for n in tests if not pattern.search(n)])
print("helper call expressions:", len(re.findall(r'\bprobe_pin_bin\(', text)) - 1)
PY

Repository: phase-rs/phase

Length of output: 31743

---

Serialize the probe_pin_bin tests under nextest.

SERIAL is process-local. Nextest runs each test in a separate process, so the tests that call probe_pin_bin do not share this lock. Each child inherits CARGO_TARGET_DIR and runs cargo test -p probe-pin --test pure_logic --no-run, which causes concurrent Cargo-build contention. Add a probe-pin-serial test group with max-threads = 1 in .config/nextest.toml, and update the comment to distinguish this cross-process guarantee from the in-process mutex.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/tests/isolation_e2e.rs` around lines 86 - 101, Add a
probe-pin-serial test group with max-threads = 1 in .config/nextest.toml and
assign the probe-pin isolation tests that invoke probe_pin_bin to it. Update the
comment near SERIAL to clarify that the mutex only serializes tests within a
process, while the nextest group provides cross-process serialization.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add a reach guard for the three base edits.

base is built from dogfood.toml by three chained replace calls. None is asserted to have applied. If dogfood.toml's formatting drifts by one space, all three become no-ops and base equals the committed manifest. The reach guard on line 593 still passes, because the unedited dogfood.toml also exits 0. Both hostile arms then abort on the original manifest and stay green for a reason unrelated to the "mutant leaves the census passing" shape this test pins.

The loop below already asserts edit != base for exactly this reason. Apply the same rule here.

💚 Proposed fix

+    let original =
+        std::fs::read_to_string(root().join("crates/probe-pin/tests/fixtures/dogfood.toml"))
+            .unwrap();
+    assert_ne!(
+        base, original,
+        "the base edits must apply, or both hostile arms pin the unedited manifest"
+    );
     // the reach guard: this manifest must be GREEN when the target really runs, or every arm
     // below aborts for a reason that has nothing to do with the floor
     let (_, arg) = tmp_manifest("floor_base.toml", &base);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let base = std::fs::read_to_string(root().join("crates/probe-pin/tests/fixtures/dogfood.toml"))
	.unwrap()
	// a probe whose mutant leaves the census passing: the shape that renders a green `pass`
	// row, which is what a skipped run forges
	.replace(
	" find = \"SITE-TWO marker beta\\n\"\n replace = \"\"",
	" find = \"alpha\"\n replace = \"gamma\"",
	)
	.replace(" text = \"marker\"\n count = 1", " text = \"gamma\"\n count = 1")
	.replace(
	" outcome = \"fail\"\n anchor = [\"CENSUS VIOLATED: expected 2 sites, got 1\",\n \"text: \\\"SITE-ONE marker alpha\\\\n\\\"\"]",
	" outcome = \"pass\"",
	);
	// the reach guard: this manifest must be GREEN when the target really runs, or every arm
	// below aborts for a reason that has nothing to do with the floor
	let (_, arg) = tmp_manifest("floor_base.toml", &base);
	let (rc, err) = probe_pin_bin(&["run", &arg], &[]);
	assert_eq!(rc, 0, "the benign arm must pass: {err}");
	let original =
	std::fs::read_to_string(root().join("crates/probe-pin/tests/fixtures/dogfood.toml"))
	.unwrap();
	let base = std::fs::read_to_string(root().join("crates/probe-pin/tests/fixtures/dogfood.toml"))
	.unwrap()
	// a probe whose mutant leaves the census passing: the shape that renders a green `pass`
	// row, which is what a skipped run forges
	.replace(
	" find = \"SITE-TWO marker beta\\n\"\n replace = \"\"",
	" find = \"alpha\"\n replace = \"gamma\"",
	)
	.replace(" text = \"marker\"\n count = 1", " text = \"gamma\"\n count = 1")
	.replace(
	" outcome = \"fail\"\n anchor = [\"CENSUS VIOLATED: expected 2 sites, got 1\",\n \"text: \\\"SITE-ONE marker alpha\\\\n\\\"\"]",
	" outcome = \"pass\"",
	);
	assert_ne!(
	base, original,
	"the base edits must apply, or both hostile arms pin the unedited manifest"
	);
	// the reach guard: this manifest must be GREEN when the target really runs, or every arm
	// below aborts for a reason that has nothing to do with the floor
	let (_, arg) = tmp_manifest("floor_base.toml", &base);
	let (rc, err) = probe_pin_bin(&["run", &arg], &[]);
	assert_eq!(rc, 0, "the benign arm must pass: {err}");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/tests/isolation_e2e.rs` around lines 576 - 593, Add an
assertion after the chained replacements building base and before tmp_manifest
that verifies base differs from the original dogfood manifest content, matching
the existing edit != base reach-guard pattern. Keep the current benign-arm
execution and assertion unchanged.

[github-actions]

Generated for head 8f283634934b04b848122956d8211b8aa54df914.

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Blocking: generated blocks can inject a second marker

Expect::Fail.anchor is validated only for blank and line-number forms; it still accepts a value such as "\nPROBE-PIN:END" (manifest.rs:529-545). block::cell escapes only |, and render writes that anchor directly into the generated table (block.rs:178-180, 211-216, 266-285).

run --write compares the old block then splices the generated text without validating that generated text (main.rs:214-245). The next invocation uses locate, which requires exactly one :BEGIN and one :END (block.rs:303-326). Therefore a manifest can cause run --write to create a block that its own next check cannot locate (MarkerNotUnique), breaking the regenerable/re-measurable artifact guarantee.

Please make every manifest-derived rendered field marker-safe (at minimum, reject control/newline characters and marker tags where they can reach generated lines, including marker and projection text as applicable), or validate the generated block's marker structure before writing. Add a regression test proving marker/newline injection is rejected and that run --write cannot produce an uncheckable block.

[lgray]

🤖 AI text below 🤖

@matthewevans — addressed at 2a47fca62. Both remedies you named are implemented, because measuring the first exposed a residual only the second closes.

Your review is pinned to ce428ff47, which predates c1df7cb6c. Manifest-derived rendered fields are already refused there. I re-ran your exact strings through the built binary rather than reasoning about the diff — each aborts at exit 2, before any write:

manifest input	outcome
anchor = "\nPROBE-PIN:END" (your string)	exit 2 — carries the control character '\n'
anchor = "boom\nsecond line"	exit 2 — same
marker = "PROBE\nPIN"	exit 2
[[projection]] sentence = "PROBE-PIN:END {count}"	exit 2 — contains the marker tag

That coverage is field-shaped, which is the failure mode this crate has already had to repair once: three review rounds each patched the field a reviewer named, and the next round found the same defect in a different field. The same reasoning applies to your finding. Two producers reach a rendered line without passing through a manifest field, so no advance validation covers them:

• an instrument's --version output, rendered into the provenance line;
• the provenance paths lifted from the target's own panic text.

So I took your second form as well. block::splice now re-runs locate over its own spliced output and returns an abort rather than a block whose marker span the next check could not find:

pub fn splice(text: &str, marker: &str, file: &Path, block: &str) -> Result<String, Abort> {
    let (begin, end) = locate(text, marker, file)?;
    let spliced = format!("{}{block}{}", &text[..begin], &text[end..]);
    locate(&spliced, marker, file)?;   // refuse what the next check could not find
    Ok(spliced)
}

Field validation stays as the early, specific refusal; the postcondition is what makes "run --write cannot produce an uncheckable block" unconditional instead of contingent on field coverage.

Regression test — pure_logic::write_can_never_produce_a_block_its_own_check_cannot_locate, asserting both halves you asked for: manifest::validate_block_text refuses the injected marker, and splice refuses to return an unlocatable block.

Non-vacuity, measured rather than asserted: deleting the postcondition line makes the test fail at exit 101, and the panic names the injected second END —

panicked at crates/probe-pin/tests/pure_logic.rs:1380:
a block carrying a second END must never be returned for writing:
"above\n// PROBE-PIN:BEGIN new\n// | p | m | pass | pass | // PROBE-PIN:END | — |\n// PROBE-PIN:END\nbelow\n"

The mutated source was restored and re-verified byte-identical with sha256sum -c. Worth recording that the test's first draft was wrong in a way the binary's exit code hid: it called manifest::validate(&m), which returns Ok for "\nPROBE-PIN:END" — the guard is validate_block_text, a separate function reached from main.rs. Retargeted at the function that actually refuses.

Gate battery at this head, post-rebase onto upstream/main cc7668062: cargo fmt --all --check 0 · cargo clippy --workspace --all-targets -D warnings 0 · cargo test -p probe-pin 0 (pure_logic 64 passed / 1 ignored) · --test isolation_e2e -- --ignored 0 (17 passed) · cargo probe-pin check …/dogfood.toml 0.

[lgray]

🤖 AI text below 🤖

Follow-up at 9104cef64 — the premise under my previous comment is now measured rather than asserted.

I claimed the splice postcondition makes the guarantee "unconditional at the write surface". That is only true while splice is the sole route to the pinned file's bytes, and I had stated that without measuring it. Census of src/, counted on call sites rather than on the word:

producer	writes	routed through splice
mutate.rs:93	the mutant, into the scratch dir	n/a — never a rendered block
main.rs:232	the pinned file	yes — the content argument is block::splice(..)?

Two producers, no others. So the guard sits inside the only write's argument, not merely on its path. pure_logic::every_write_of_the_pinned_file_routes_through_splice pins that, so a second fs::write of output.file fails a test instead of quietly falsifying the sentence in the docs and in this thread.

Four arms, each flipping a different assertion — dropping splice from the write argument (exit 101, property), adding a third producer (exit 101, found 3), breaking the search patterns (exit 101, zero-census positive control).

The fourth arm did not flip, and I'd rather record that than drop it. The test strips comment lines before counting, and my first draft of its doc comment justified that by claiming a raw grep finds a third hit in manifest.rs. It doesn't: that mention is `fs::write` in backticks with no open paren, and every producer pattern requires the paren — I had carried the number over from an earlier grep with a different pattern and never re-measured under the instrument I shipped. Deleting the strip left the test green. It is now disclosed as defensive, with its value measured against the input it actually defends: a doc comment quoting a call site, green with the strip and found 3 without.

Gate battery at this head: fmt --check 0 · clippy --workspace --all-targets -D warnings 0 · cargo test -p probe-pin 0 (pure_logic 65 passed / 1 ignored) · --test isolation_e2e -- --ignored 0 (17 passed) · cargo probe-pin check …/dogfood.toml 0 · Gate A PASS head=9104cef64 base=cc7668062 (vacuous — 0 of 31 changed files are under crates/engine/src/parser; stated in the body as not-applicable-but-green).

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

crates/probe-pin/src/block.rs (1)

381-394: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

The hand-written variant array defeats the compiler on a new Instrument.

Line 388 enumerates [Instrument::Toolchain, Instrument::AstGrep] by hand. Add a third variant to Instrument and this code still compiles. The new variant's // instrument <tool> = <version> line then resolves to None, instrument_lines drops it, and changed_instruments never reports it.

The consequences are both in the classification this module owns:

• check reports DriftCause::Code for a run where only that instrument moved. The report on line 494 then tells the operator the block is stale.
• write_refusal never fires for that instrument, so run --write re-stamps across the change it cannot attribute — the exact case lines 460-462 say it must refuse.

used_instruments on line 294 carries the same pair. Give Instrument one authoritative list and use it in both places, so a new variant is a compile error and not a silent hole.

♻️ Proposed direction

// in the module that defines `Instrument`
impl Instrument {
    /// Every variant, exhaustively. The `match` is what makes a new variant a compile error.
    pub const ALL: &'static [Instrument] = &[Instrument::Toolchain, Instrument::AstGrep];

    #[allow(dead_code)]
    fn exhaustiveness_guard(self) -> &'static str {
        // A new variant fails to compile here, which is the point.
        match self {
            Instrument::Toolchain => "toolchain",
            Instrument::AstGrep => "ast-grep",
        }
    }
}

-            [Instrument::Toolchain, Instrument::AstGrep]
-                .into_iter()
+            Instrument::ALL
+                .iter()
+                .copied()
                 .find(|i| i.tool() == name)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/block.rs` around lines 381 - 394, Define one
authoritative exhaustive Instrument list, with a match-based guard that requires
updating when variants are added, and replace the hand-written variant pairs in
both instrument_lines and used_instruments with that list. Preserve the existing
tool-name matching and ensure every Instrument variant is considered for
classification and write refusal.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/probe-pin/src/block.rs`:
- Around line 214-220: Update firing_cell to determine control status from
probe.mutations.is_empty(), matching mutation_cell, rather than from
Verdict::Pass { mounts_reached: 0 }; keep mounts_reached only for reporting
reached files and preserve the existing failure-anchor output.
- Around line 257-260: Update render’s probe/outcome handling so every declared
probe has the same explicit unmeasured representation as digest, rather than
silently skipping probes with no matching outcome. Prefer enforcing a complete
probe/outcome pairing at the render boundary, or abort clearly when a declared
probe lacks an outcome; ensure the rendered table cannot contain fewer rows than
manifest.probes.

In `@crates/probe-pin/src/manifest.rs`:
- Around line 567-571: Update the validation loop over positional_argv_values in
validate to reject [target].filter when its value is empty after trimming,
alongside the existing leading-dash operand check. Emit a manifest-validation
error for the blank filter while preserving the current rejection for values
beginning with '-'.
- Around line 623-628: Update mutation validation in
crates/probe-pin/src/manifest.rs at lines 623-628 and 122-126: in the existing
Mutation::Prepend arm, reject pads whose computed byte count is zero, covering
repeat = 0 and empty text; in the mutation loop’s Mutation::Replace arm, reject
an empty find value and reject find values equal to replace. These checks must
prevent zero-effect or non-targeted mutations while preserving the existing
size-cap validation.

In `@Tiltfile`:
- Around line 287-295: Prevent probe-pin-e2e and probe-pin-check from running
concurrently while sharing target/probe-pin. Update the local_resource
definitions for both resources to disable parallel execution, or assign
probe-pin-e2e a distinct CARGO_TARGET_DIR; preserve their existing commands and
dependency watches.

---

Nitpick comments:
In `@crates/probe-pin/src/block.rs`:
- Around line 381-394: Define one authoritative exhaustive Instrument list, with
a match-based guard that requires updating when variants are added, and replace
the hand-written variant pairs in both instrument_lines and used_instruments
with that list. Preserve the existing tool-name matching and ensure every
Instrument variant is considered for classification and write refusal.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a13c76eb-0cd7-4d37-b2bd-a4f04168145d

📥 Commits

Reviewing files that changed from the base of the PR and between f186aef and 2a47fca.

📒 Files selected for processing (18)

• Tiltfile
• crates/probe-pin/src/block.rs
• crates/probe-pin/src/isolate.rs
• crates/probe-pin/src/lib.rs
• crates/probe-pin/src/main.rs
• crates/probe-pin/src/manifest.rs
• crates/probe-pin/src/target.rs
• crates/probe-pin/src/verdict.rs
• crates/probe-pin/tests/fixtures/collide.toml
• crates/probe-pin/tests/fixtures/compiled.toml
• crates/probe-pin/tests/fixtures/control_expect_fail.toml
• crates/probe-pin/tests/fixtures/control_fails.toml
• crates/probe-pin/tests/fixtures/projection.toml
• crates/probe-pin/tests/fixtures/treewrite.toml
• crates/probe-pin/tests/fixtures/unsorted_ids.toml
• crates/probe-pin/tests/isolation_e2e.rs
• crates/probe-pin/tests/pure_logic.rs
• docs/probe-pin.md

🚧 Files skipped from review as they are similar to previous changes (10)

• crates/probe-pin/tests/fixtures/projection.toml
• crates/probe-pin/tests/fixtures/collide.toml
• crates/probe-pin/tests/fixtures/control_fails.toml
• crates/probe-pin/tests/fixtures/control_expect_fail.toml
• crates/probe-pin/tests/fixtures/treewrite.toml
• crates/probe-pin/src/verdict.rs
• crates/probe-pin/src/main.rs
• crates/probe-pin/src/isolate.rs
• crates/probe-pin/src/lib.rs
• crates/probe-pin/tests/fixtures/unsorted_ids.toml

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

validate checks mutation paths and the Prepend upper bound, but never checks that a mutation payload expresses a targeted, non-identity edit. Both variants therefore admit a mutant that is byte-identical to the pristine file, which is the harm the empty-file-list refusal at Line 613 names: the probe measures the unmodified tree and the block renders that verdict as a mutant's.

• crates/probe-pin/src/manifest.rs#L623-L628: in the same arm that checks MAX_PREPEND_BYTES, refuse a pad of 0 bytes, so repeat = 0 and text = "" are rejected.
• crates/probe-pin/src/manifest.rs#L122-L126: add a Mutation::Replace arm to the mutation loop that refuses an empty find (it matches at every position and rewrites the whole file) and refuses find == replace (an identity edit).

📍 Affects 1 file

• crates/probe-pin/src/manifest.rs#L623-L628 (this comment)
• crates/probe-pin/src/manifest.rs#L122-L126

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/probe-pin/src/manifest.rs` around lines 623 - 628, Update mutation
validation in crates/probe-pin/src/manifest.rs at lines 623-628 and 122-126: in
the existing Mutation::Prepend arm, reject pads whose computed byte count is
zero, covering repeat = 0 and empty text; in the mutation loop’s
Mutation::Replace arm, reject an empty find value and reject find values equal
to replace. These checks must prevent zero-effect or non-targeted mutations
while preserving the existing size-cap validation.

[lgray]

🤖 AI text below 🤖

@coderabbitai — all five verified against a run before anything was edited. Four fixed at 815aaf279; one declined with measurement.

Declined: identity mutations are already refused. The finding is right that validate does not check mutation payloads. The refusal is not in validate — it is in mutate.rs, after materialization, where it compares the mutant against the original. Every case named, run through the shipping binary:

manifest	result
find == replace	exit 2 — the materialized mutant is byte-identical to the original
prepend repeat = 0	exit 2 — same guard
prepend text = ""	exit 2 — same guard
replace find = ""	exit 2 — the occurrence gate

One guard covers the class regardless of mutation kind, including kinds not yet written. The proposed fix adds per-variant checks in validate — narrower, and enumerated per case; that is the field-shaped pattern this crate has already had to repair twice, so I'd rather not reintroduce it one surface up.

Fixed:

• firing_cell now keys on probe identity. Correct diagnosis: mounts_reached == 0 and probe.mutations.is_empty() were two discriminators for one concept. Latent today — validate refuses an empty mutation file list, so mounts.len() >= 1 — but the digest pins the same count and so would have agreed with a lying row. Rendered output is byte-identical: the committed block is unchanged in git, check exits 0.
• A blank [target].filter is refused, next to the operand check, as the empty end of the same positive-shape rule. Measured with substring: the run widens to the whole binary and exits 2 — but as "the control probe FAILED … the instrument is broken", because a widened test happened to fail. Nothing looked at the filter. Had the widened suite passed, every row would have rendered green.
• probe-pin-e2e gets its own CARGO_TARGET_DIR. Confirmed: both resources watch crates/probe-pin/, both allow_parallel, both drove cargo into target/probe-pin.

On the missing-row finding — the suggested fix was wrong, and my test suite caught it. I applied unreachable! as proposed. It panicked in projection_surface_gets_the_producer_checks_its_siblings_get, which calls block::render(&m, .., &[], ..) deliberately to inspect projection sentences. render is public and that input is legitimate; the invariant belongs to main's call order, not to this function, so enforcing it there fires on correct use.

The finding underneath it was still right, so render now emits a visible unmeasured row — the same thing digest already records for that input (rc = -1, verdict = "unmeasured"). That closes the disagreement without a panic, and a_probe_with_no_outcome_gets_a_visible_unmeasured_row pins it: restoring the skip fails at exit 101 with "the table dropped a declared probe".

A second existing test listed "" among benign filter values. Rather than delete the case, it now asserts which guard refuses it — the naming rule, not the leading-- operand rule — so the benign list still proves the operand rule is discriminating rather than blanket-rejecting the field.

Each fix has a DROP arm flipping a different assertion: reverting firing_cell → 101 "a probe WITH mutations rendered as the control"; dropping the filter refusal → 101; restoring the silent skip → 101. Mutated sources restored and re-verified with sha256sum -c.

Battery at this head: fmt --check 0 · clippy --workspace --all-targets -D warnings 0 · cargo test -p probe-pin 0 (pure_logic 68 passed / 1 ignored) · --test isolation_e2e -- --ignored 0 (17 passed) · cargo probe-pin check …/dogfood.toml 0.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@lgray, thanks for the detailed verification.

I withdraw the identity-mutation finding. The materialization guard rejects byte-identical mutants for all mutation kinds. This is broader than variant-specific validation and prevents no-op evidence from being generated.

The four fixes address the remaining findings:

• firing_cell now uses probe identity.
• Validation rejects a blank [target].filter.
• probe-pin-e2e uses a separate CARGO_TARGET_DIR.
• render shows an unmeasured row instead of silently omitting a declared probe.

The regression and DROP-arm results support each change.

---

✏️ Learnings added

Learnt from: lgray
URL: https://github.com/phase-rs/phase/pull/7315

Timestamp: 2026-08-13T00:37:55.184Z
Learning: In the Rust `probe-pin` crate, `crates/probe-pin/src/mutate.rs` rejects a materialized mutant when its bytes are identical to the original file. This common guard rejects identity mutations across mutation kinds, including `replace` where `find == replace`, `prepend` with zero repetition or empty text, and future mutation kinds that produce no file change.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

_{You are interacting with an AI system.}

[matthewevans]

Approved after current-head review: the block-marker injection path is closed at manifest validation and final splice verification; complete CI is green.