Code review findings for pull request #113

[sandersaares]

[Copilot speaking]

Review findings for pull request #113

This issue contains the approved findings from the review of #113 at source commit 3edf357. The report continues in comments because the complete rendered Markdown exceeds GitHub's per-body limit.

Title: Align the design comment-directive spelling with the supported syntax

[Copilot speaking]

The design says the attribute accepts exactly the text accepted by an equivalent // gamma: comment directive. The supported and documented comment form is instead an attribute-shaped comment such as // #[gamma::test_timeout_multiplier(...)]; the shorthand-looking // gamma: form is not the directive syntax. Because this paragraph defines the user-visible accepted syntax and emphasizes character-for-character equivalence, the backticked spelling reads as a contractual example rather than an informal channel name.

Location: crates/cargo-gamma-attrs-impl/docs/DESIGN.md:21-24

Reproducible reasoning: The public attribute documentation's “Relationship to the comment form” section consistently shows // #[gamma::...]. The cross-channel agreement test also constructs // #[gamma::test_timeout_multiplier({arguments})] and describes the two channels as differing only by the leading comment characters. Separately, the suppression regression tests explicitly treat shorthand without the attribute brackets as unsupported. Reading those sources together shows that // gamma: does not identify a valid literal directive form, while this design paragraph presents it in code formatting immediately before claiming exact textual equivalence.

Consequence: A reader can copy or preserve the shorthand-looking form and believe Gamma will apply a timeout override, while the tool ignores or rejects that text instead of applying the documented contract.

Recommended action: Make the design wording and accepted syntax agree. If // gamma: is only a label for the comment channel, replace the code-form spelling with “comment-form directive” or show a supported representative such as // #[gamma::test_timeout_multiplier(...)]. If the shorthand is intended as literal syntax, implement it in the comment parser and cover it in the cross-channel agreement tests before claiming exact equivalence.

References:

• crates/cargo-gamma-attrs/src/lib.rs, “Relationship to the comment form”
• crates/cargo-gamma-lib/tests/agreement.rs, the_two_channels_agree_on_timeout_multiplier_arguments

Impacted locations:

• crates/cargo-gamma-attrs-impl/docs/DESIGN.md:21-24

Reviewed in: #113

---

Title: Document or separate the workspace-wide lint policy change

[Copilot speaking]

The pull request is titled and described as an end-to-end hardening of cargo-gamma, but Cargo.toml also changes the lint policy inherited across the entire workspace: it enables empty_structs_with_brackets and too_long_first_doc_paragraph, and globally allows literal_string_with_formatting_args. This repository-wide policy change is not identified in the pull request description.

Location: Cargo.toml:211-219

Reproducible reasoning: The new entries are under [workspace.lints], so their scope is every workspace member that inherits the workspace lint table rather than only the cargo-gamma crates. The pull request description lists mutation discovery, process containment, runtime census, reporting, CLI diagnostics, and contract coverage, but does not mention changing static-verification policy for unrelated crates. Because both the title and description frame the work around cargo-gamma, a reviewer has no basis to tell whether this workspace-wide change is an intentional part of the mission or unrelated work included accidentally.

Consequence: Maintainers can merge the change believing they are reviewing only cargo-gamma hardening while also changing which constructs produce warnings or are permitted by default in future work across unrelated workspace crates.

Recommended action: If the lint policy change is intentional, add it to the pull request's motivation and substantive-change summary and explain why it belongs with the cargo-gamma work. If it is independent, move it to a separate pull request; that provides isolated policy review at the cost of repeating the small amount of validation associated with these manifest entries.

Impacted locations:

• Cargo.toml:211-219

Reviewed in: #113

---

Title: Define the attribute vocabulary before stating parser boundaries

[Copilot speaking]

The added boundary rules introduce the specialized terms "stated value", "comment directive", and "positional timeout multiplier", then refer to selectors and the reason and tag arguments, without defining that vocabulary or directing the reader to its definitions before use. The sibling public README has dedicated "The vocabulary" and "Stating a value" sections, but this design document does not establish that relationship.

Location: crates/cargo-gamma-attrs-impl/docs/DESIGN.md:17-28

Reproducible reasoning: The Purpose section only explains that this crate parses and validates inert attributes. A reader then reaches the new rules and must already know that a stated value is the expression supplied through #[gamma::value(...)], that the // gamma: form is a source-comment equivalent of an attribute directive, and that a timeout multiplier is one argument accepted alongside selector, reason, and tag arguments. Those are domain-specific concepts rather than ordinary Rust terminology. Their definitions elsewhere in the repository confirm that they require explanation, but the design document neither defines them nor links to those definitions before making contractual claims about their ordering and rejection behavior. A linear reading therefore cannot recover the precise meaning of the new rules from this document.

Consequence: A maintainer can misread "stated value" as any literal supplied to an attribute, or infer that a "positional timeout multiplier" has position-dependent semantics despite the following sentence denying positional meaning. That ambiguity makes future parser, diagnostic, and documentation changes more likely to use the terms inconsistently.

Recommended action: Add a short terminology paragraph before the boundary rules, or an explicit preceding link to the public vocabulary sections, defining the stated-value attribute, comment directive, timeout multiplier, selectors, and named reason and tag arguments. Then express the boundary rules with those established terms.

References:

• crates/cargo-gamma-attrs/README.md, "The vocabulary"
• crates/cargo-gamma-attrs/README.md, "Stating a value"

Impacted locations:

• crates/cargo-gamma-attrs-impl/docs/DESIGN.md:17-28

Reviewed in: #113

---

Title: Align empty-body eligibility with the documented semantic rationale

[Copilot speaking]

The new empty-body documentation and diagnostic claim that substituting any stated expression for an empty function body produces an identical program. That is false for valid unit-compatible expressions with observable behavior, such as panic!() or a side-effecting call returning (). The implementation rejects these annotations because collection excludes empty-body sites before consulting the stated value, so the user-facing rationale is stronger than the behavior the code establishes.

Location: crates/cargo-gamma-engine/src/ops/collect/stated.rs:54-56

Reproducible reasoning: An empty body evaluates to (), but Rust also accepts expressions that return () after observable effects and diverging expressions that coerce to the function's return type. Replacing {} with either kind can change behavior. The collector's early exclusion of an empty body is therefore the reproducible reason no stated-value mutant is produced; behavioral equivalence of every accepted expression is not. The incorrect semantic claim is repeated across implementation documentation, diagnostics, and test rationale.

Consequence: Users can be told that an observable mutant is inherently identical to the original program, while maintainers may preserve the exclusion for a false semantic reason and overlook that it is an eligibility policy or a possible future extension point.

Recommended action: Choose one contract and make collection, attribute validation, public documentation, diagnostics, and tests agree. If empty bodies remain ineligible, describe that as an eligibility rule applied before the stated expression is read, without claiming that every unit-compatible or diverging expression is behaviorally identical. If those expressions should be eligible, change collection accordingly and cover side-effecting and diverging examples. Preserve a semantic-equivalence claim only if the accepted syntax actually enforces it.

References:

• crates/cargo-gamma-attrs-impl/docs/DESIGN.md
• crates/cargo-gamma-attrs/src/lib.rs
• crates/cargo-gamma/docs/MUTATORS.md, "Stating the value yourself"
• crates/cargo-gamma-engine/src/ops/collect/collector.rs:482-513

Impacted locations:

• crates/cargo-gamma-attrs-impl/src/implementation.rs:70-75
• crates/cargo-gamma-attrs-impl/src/implementation.rs:344-347
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:54-56
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:150-154
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:457-459

Reviewed in: #113

---

Title: Validate timeout-multiplier arguments without materializing and rebuilding the whole list

[Copilot speaking]

The new timeout-multiplier path clones the attribute stream, splits every top-level argument into an allocated Vec<Vec<TokenTree>>, and then clones every non-positional argument into a second TokenStream. validate_shape immediately materializes that rebuilt stream into another token vector. This duplicates the token storage before validation even begins, although only one argument needs to be classified at a time.

Location: crates/cargo-gamma-attrs-impl/src/implementation.rs:598

Reproducible reasoning: arguments_of allocates an outer vector and a separate vector for each nonempty comma-delimited argument. validate_timeout_multiplier retains all of those vectors while it creates rest by cloning the non-positional tokens and synthesizing comma tokens. It then passes rest to validate_shape, whose existing random-access parser collects the stream into its own vector. The design contract requires top-level splitting and per-argument classification, but it does not require retaining all segments simultaneously or reconstructing their original stream. Processing each completed top-level segment as it is encountered, while carrying the stated/strict-reading state between segments, preserves the accepted syntax and duplicate detection while avoiding the new outer collection and reconstruction allocations. This path runs inside rustc for every gamma, timeout_multiplier, and test_timeout_multiplier macro expansion, so the added work scales with annotated items.

Consequence: A crate with many timeout-annotated tests performs multiple avoidable heap allocations and token clones for every macro expansion, increasing compile-time allocation pressure and latency in rustc.

Recommended action: Fold top-level splitting, positional classification, and shape validation into a streaming pass. Reuse one segment buffer (or bounded-lookahead cursor), carry multiplier-seen state across segments, and validate non-positional segments directly instead of retaining Vec<Vec<TokenTree>> and rebuilding rest. Preserve the syntax contract documented in the crate design while eliminating the duplicate token materialization.

References:

• crates/cargo-gamma-attrs-impl/docs/DESIGN.md, Boundaries

Impacted locations:

• crates/cargo-gamma-attrs-impl/src/implementation.rs:540-643

Reviewed in: #113

---

Title: Centralize newly referenced items in module-level imports

[Copilot speaking]

New code across the reviewed modules introduces dependencies inside declarations and function bodies by spelling qualified paths inline, placing use declarations inside functions or below module items, importing production siblings through super::, or bypassing a crate’s public composition path. These references do not require path-based disambiguation and can be represented consistently in each file or module import block.

Location: crates/cargo-gamma-attrs-impl/src/implementation.rs:633

Reproducible reasoning: The affected instances share one maintainability issue: newly referenced types, traits, functions, macros, constants, and modules were not incorporated into the owning module’s import surface. Module-level imports make dependencies visible in one place and avoid coupling production modules to nesting depth. Where an item is re-exported by the crate that owns the composition boundary, importing through that public crate path also keeps callers independent of the underlying dependency. Existing cfg gates and aliases can remain on the imports where platform availability or name collisions require them.

Consequence: Dependencies are scattered through signatures, declarations, and function bodies instead of being visible at module boundaries. This makes the affected modules harder to scan and creates many separate path spellings to update when items move or imports are reorganized.

Recommended action: Move the affected references into the owning file or module import block and use the imported names at call sites. In production modules, prefer crate-root paths over super::; across crates, prefer the composing crate’s public re-export over its private dependency path. Preserve necessary cfg attributes and aliases, and keep qualified call-site paths only where they genuinely disambiguate names.

Impacted locations:

• crates/cargo-gamma-attrs-impl/src/implementation.rs:633
• crates/cargo-gamma-attrs/tests/consumer.rs:75
• crates/cargo-gamma-attrs/tests/diagnostics.rs:63-111
• crates/cargo-gamma-engine/src/error.rs:255
• crates/cargo-gamma-engine/src/ops/collect/collector.rs:1334
• crates/cargo-gamma-engine/src/ops/collect/defaults.rs:261-280
• crates/cargo-gamma-engine/src/ops/collect/defaults.rs:968-980
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:115
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:2777
• crates/cargo-gamma-lib/src/ci/sarif.rs:187-195
• crates/cargo-gamma-engine/src/text.rs:212
• crates/cargo-gamma-lib/src/commands/list.rs:44-45
• crates/cargo-gamma-lib/src/commands/run.rs:800
• crates/cargo-gamma-lib/src/config.rs:1033-1051
• crates/cargo-gamma-lib/src/config.rs:1098-1105
• crates/cargo-gamma-lib/src/config.rs:1140-1146
• crates/cargo-gamma-lib/src/config.rs:1195-1200
• crates/cargo-gamma-lib/src/config.rs:1275
• crates/cargo-gamma-lib/src/discover/record.rs:501-505
• crates/cargo-gamma-lib/src/error.rs:136-142
• crates/cargo-gamma-lib/src/exec/census.rs:990-1001
• crates/cargo-gamma-lib/src/exec/census.rs:1524
• crates/cargo-gamma-lib/src/exec/measure.rs:493
• crates/cargo-gamma-lib/src/exec/sweep.rs:119
• crates/cargo-gamma-lib/src/exec/sweep.rs:1531-1571
• crates/cargo-gamma-lib/src/exec/workspace.rs:1119
• crates/cargo-gamma-lib/src/exec/workspace.rs:1277
• crates/cargo-gamma-lib/src/exec/workspace.rs:1762
• crates/cargo-gamma-lib/src/exec/workspace.rs:1769
• crates/cargo-gamma-lib/src/exec/workspace.rs:1805
• crates/cargo-gamma-lib/src/exec/workspace.rs:1832
• crates/cargo-gamma-lib/src/merge/union.rs:447
• crates/cargo-gamma-lib/tests/agreement.rs:239
• crates/cargo-gamma-lib/tests/agreement.rs:308-311
• crates/cargo-gamma-lib/tests/session.rs:389
• crates/cargo-gamma-lib/tests/session.rs:413
• crates/cargo-gamma-lib/tests/units.rs:143-146
• crates/cargo-gamma-process/src/testing.rs:99
• crates/cargo-gamma-rt/src/runtime.rs:675-701
• crates/cargo-gamma-rt/src/runtime.rs:1030-1055
• crates/cargo-gamma-rt/src/runtime.rs:2968-3035
• crates/cargo-gamma-unsafe/src/cgroup.rs:83
• crates/cargo-gamma-unsafe/src/cgroup.rs:108
• crates/cargo-gamma-unsafe/src/cgroup.rs:1604
• crates/cargo-gamma-unsafe/src/job.rs:798
• crates/cargo-gamma-unsafe/src/job.rs:817-821
• crates/cargo-gamma-unsafe/src/native_faults.rs:126
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:12-13
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:576
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:606
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:642
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:673
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:41
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:44
• crates/cargo-gamma-lib/src/exec/sync.rs:22
• crates/cargo-gamma-process/src/process_tree.rs:18
• crates/cargo-gamma-process/src/process_tree.rs:68
• crates/cargo-gamma-process/src/process_tree.rs:1058
• crates/cargo-gamma-process/src/process_tree.rs:2069
• crates/cargo-gamma-process/src/process_tree.rs:2410
• crates/cargo-gamma-process/src/process_tree.rs:2526
• crates/cargo-gamma-rt/src/runtime.rs:2835-2882

Reviewed in: #113

---

Title: Document the value rejection as a contract, not a guard implementation

[Copilot speaking]

The newly expanded public rustdoc correctly states that #[gamma::value] is rejected on const fn and empty bodies, but it explains that rule through the collector's reachability and the current run-time guard insertion mechanism. Those details belong to the attribute implementation; the user-facing contract is the accepted placement and diagnostic behavior.

Location: crates/cargo-gamma-attrs/src/lib.rs:435-439

Reproducible reasoning: Lines 435-439 describe collection reaching a value and a mutant being spliced behind a run-time guard. crates/cargo-gamma-attrs/docs/DESIGN.md defines this crate as the user-facing attribute namespace and identifies supported syntax and diagnostics as its public contract, while crates/cargo-gamma-attrs-impl/docs/DESIGN.md owns parsing and validation. The rejection can remain stable if collection or instrumentation is later reorganized, so publishing the current mechanism in the macro API documentation turns an implementation choice into an apparent promise.

Consequence: A future instrumentation or collection refactor can preserve the rejection contract while making this rustdoc false, and downstream readers may incorrectly rely on guard placement or collector behavior as stable API behavior.

Recommended action: Keep the public rustdoc focused on the requirement that value is rejected on declarations, const fn, and empty bodies. Move the collector and run-time guard explanation to the implementation crate's design documentation or an implementation comment.

References:

• crates/cargo-gamma-attrs/docs/DESIGN.md
• crates/cargo-gamma-attrs-impl/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-attrs/src/lib.rs:435-439

Reviewed in: #113

---

Title: Describe the diagnostic tests as fragment checks rather than exact-output contracts

[Copilot speaking]

The new diagnostics test module says it pins each macro's exact diagnostic, but every test only checks that stderr contains the macro prefix and one message fragment. It does not pin the complete diagnostic, spans, severity, formatting, or additional emitted errors.

Location: crates/cargo-gamma-attrs/tests/diagnostics.rs:5-14

Reproducible reasoning: The module documentation at lines 5-14 claims exactness and then explains that validation is performed through substrings. The helper returns the complete stderr, but the assertions at lines 131-192 use only contains. Therefore many changes to the actual diagnostic output can pass while the documentation tells maintainers that the exact output is protected.

Consequence: Maintainers can rely on a stronger test contract than exists and overlook regressions in diagnostic shape because the suite remains green despite the documented exact-output guarantee.

Recommended action: Rename and document these as checks for diagnostic identity and the essential reason. If byte-for-byte diagnostic stability is genuinely required, use a normalized golden comparison instead, recognizing that exact compiler stderr is more brittle across supported Rust versions.

Impacted locations:

• crates/cargo-gamma-attrs/tests/diagnostics.rs:5-14
• crates/cargo-gamma-attrs/tests/diagnostics.rs:131-192

Reviewed in: #113

---

Title: Make timeout-multiplier example reasons match the demonstrated code

[Copilot speaking]

The two newly added successful examples attach operational reasons that their functions do not demonstrate. digest accepts an arbitrary byte slice and computes a sum, but its reason says it hashes a megabyte. accumulate performs an ordinary conversion and u64 addition, but its reason says widening arithmetic is slow. Because these examples teach how to use auditable reason metadata, the reasons should remain verifiable from the scenario they describe.

Location: crates/cargo-gamma-attrs/src/lib.rs:564-573

Reproducible reasoning: Reading the example bodies reproduces the mismatch directly: digest has no input-size constraint and invokes no hash operation, while accumulate only folds bytes into a u64 total. The examples therefore demonstrate the attribute syntax with explanations unrelated to the shown workload. The same syntax can be taught without this shortcut by choosing scenario names, bodies, and reasons that describe one another accurately.

Consequence: Consumers copying the examples may treat reason as placeholder prose rather than durable justification for a timeout override, leaving future reviewers unable to determine why a multiplier is needed or whether it is still appropriate.

Recommended action: Revise these examples so each function and reason describe a coherent, realistic workload while preserving the positional-order and trailing-comma forms being demonstrated. For example, either show an operation and input contract that genuinely justify the multiplier, or use reasons that accurately describe the simple accumulation bodies.

Impacted locations:

• crates/cargo-gamma-attrs/src/lib.rs:564-573

Reviewed in: #113

---

Title: Select the proc-macro artifact without filesystem timestamps

[Copilot speaking]

gamma_artifact scans the target and host dependency directories for hashed dynamic libraries whose filenames have a gamma prefix, sorts the candidates by filesystem modification time, and silently passes the newest candidate to rustc. A Cargo target directory can retain matching artifacts from other source revisions, feature combinations, compiler versions, checkouts, or concurrent Cargo activity, so the newest file is not necessarily the artifact Cargo selected for this test run.

Location: crates/cargo-gamma-attrs/tests/diagnostics.rs:80-82

Reproducible reasoning: Cargo artifact hashes distinguish builds, but this lookup does not recover the hash of the dependency used by the current integration test. Cargo can reuse an already-valid current artifact without updating its modification time, while an unrelated artifact can have a later timestamp. A file's modification time records when it was written, not whether it belongs to the running test binary's build graph. Sorting by metadata.modified() and taking the last candidate therefore makes fixture compilation depend on filesystem time and target-directory history rather than the reviewed source, without documenting an invariant that makes multiple candidates safe.

Consequence: After switching branches or build configurations that share a target directory, the diagnostic tests can compile malformed fixtures against stale or unrelated proc-macro code. This can hide a regression in the current implementation or cause branch-history-dependent failures while leaving maintainers without explicit assumptions for recognizing or safely changing the selection behavior.

Recommended action: Arrange for Cargo to identify or load the exact proc-macro artifact for the current build, such as by compiling fixtures through a dedicated Cargo or trybuild-style crate with a path dependency, or by consuming Cargo's structured build output to obtain the artifact path. Cache that deterministic path for the test process instead of rescanning and ranking target contents by time. If the recency heuristic must remain, document the assumptions that make it valid and reject ambiguous candidate sets rather than silently selecting one.

References:

• crates/cargo-gamma-lib/tests/instrumented_compiles.rs (the attrs_crate rationale explains why selecting a Cargo proc-macro artifact from a direct rustc invocation is fragile)

Impacted locations:

• crates/cargo-gamma-attrs/tests/diagnostics.rs:34-82
• crates/cargo-gamma-attrs/tests/diagnostics.rs:34-80

Reviewed in: #113

---

Title: Exclude the new integration tests from the published crate

[Copilot speaking]

The change adds consumer.rs and diagnostics.rs as development-only integration tests, but cargo-gamma-attrs/Cargo.toml has no include or exclude package-file policy. Cargo therefore includes these tracked test sources in the crate archive even though downstream users do not compile or need them.

Location: crates/cargo-gamma-attrs/tests/consumer.rs

Reproducible reasoning: Both added files live under crates/cargo-gamma-attrs/tests/ and exist solely to validate this repository's proc-macro implementation. Cargo's default packaging file selection includes tracked test sources unless the manifest narrows the package contents. The package manifest currently declares neither an allowlist nor an exclusion for tests/**, so this diff directly expands the published archive with development-only source.

Consequence: Every release of cargo-gamma-attrs would distribute the external-consumer fixtures and the compiler-spawning diagnostic harness to users, increasing the package payload and exposing test infrastructure as published content without any runtime or documentation benefit.

Recommended action: Add an explicit package include list containing only the files required to compile and document the proc-macro crate, or at minimum exclude tests/**. An allowlist prevents future development artifacts from entering the archive, at the cost of updating it when a genuinely required package file is added.

References:

• crates/cargo-gamma-attrs/Cargo.toml

Impacted locations:

• crates/cargo-gamma-attrs/tests/consumer.rs:1-85
• crates/cargo-gamma-attrs/tests/diagnostics.rs:1-193

Reviewed in: #113

---

Title: Condense overlong Rust documentation summaries

[Copilot speaking]

New and rewritten Rust item and module documentation across the affected crates and tests uses opening paragraphs whose rendered visible text exceeds the 80-character summary limit. Wrapping the source across multiple documentation-comment lines does not create a paragraph break in rustdoc, so the affected comments still render as overlong summaries.

Location: crates/cargo-gamma-engine/src/cfg.rs:160-161

Reproducible reasoning: Every source finding identifies the same root cause: detailed or multi-clause prose remains in the uninterrupted first documentation paragraph, making its rendered summary longer than the required limit. The findings differ only in the affected items and files. A blank documentation line is required to separate a concise summary from the additional explanation.

Consequence: The generated documentation wraps these opening summaries instead of presenting the concise, consistently scannable first line required by the Rust documentation convention.

Recommended action: Rewrite each affected first paragraph to at most 80 visible rendered characters. Where the removed detail remains useful, retain it in a separate following paragraph after a blank documentation line, measuring rendered text rather than Markdown source width.

Impacted locations:

• crates/cargo-gamma-engine/src/cfg.rs:160-161
• crates/cargo-gamma-engine/src/model/identity.rs:145-152
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:279-280
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:298-299
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:306-307
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:327-328
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:432-433
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:98-99
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:117-118
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:125-126
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:145-146
• crates/cargo-gamma-engine/src/parse/comment.rs:33
• crates/cargo-gamma-engine/src/schema.rs:246
• crates/cargo-gamma-engine/src/text.rs:128-129
• crates/cargo-gamma-engine/src/text.rs:152
• crates/cargo-gamma-lib/src/ci/sarif.rs:24-25
• crates/cargo-gamma-lib/src/ci/sarif.rs:200
• crates/cargo-gamma-lib/src/discover/survey.rs:1030
• crates/cargo-gamma-lib/src/discover/survey.rs:1039
• crates/cargo-gamma-lib/src/exec/copy.rs:62
• crates/cargo-gamma-lib/src/exec/copy.rs:123
• crates/cargo-gamma-lib/src/exec/sweep.rs:827
• crates/cargo-gamma-lib/src/exec/workspace.rs:743
• crates/cargo-gamma-lib/src/parse.rs:9
• crates/cargo-gamma-lib/src/report/progress.rs:357
• crates/cargo-gamma-lib/tests/agreement.rs:6-7
• crates/cargo-gamma-lib/tests/agreement.rs:143-144
• crates/cargo-gamma-lib/tests/agreement.rs:182-183
• crates/cargo-gamma-lib/tests/agreement.rs:249-250
• crates/cargo-gamma-lib/tests/agreement.rs:293-294
• crates/cargo-gamma-lib/tests/agreement.rs:305-306
• crates/cargo-gamma-lib/tests/agreement.rs:316-319
• crates/cargo-gamma-lib/tests/docs.rs:269
• crates/cargo-gamma-lib/tests/docs.rs:582
• crates/cargo-gamma-lib/tests/docs.rs:596
• crates/cargo-gamma-lib/tests/docs.rs:720
• crates/cargo-gamma-lib/tests/sarif_contract.rs:36-37
• crates/cargo-gamma-lib/tests/sarif_contract.rs:79
• crates/cargo-gamma-process/src/process_tree.rs:38
• crates/cargo-gamma-process/src/process_tree.rs:223
• crates/cargo-gamma-process/src/process_tree.rs:279
• crates/cargo-gamma-process/src/process_tree.rs:289
• crates/cargo-gamma-rt/src/runtime.rs:585-613
• crates/cargo-gamma-rt/src/runtime.rs:670-718
• crates/cargo-gamma-unsafe/src/cgroup.rs:84
• crates/cargo-gamma-unsafe/src/cgroup.rs:109
• crates/cargo-gamma-unsafe/src/cgroup.rs:262
• crates/cargo-gamma-unsafe/src/cgroup.rs:276
• crates/cargo-gamma-unsafe/src/cgroup.rs:563
• crates/cargo-gamma-unsafe/src/cgroup.rs:592
• crates/cargo-gamma-unsafe/src/cgroup.rs:778
• crates/cargo-gamma-unsafe/src/cgroup.rs:899
• crates/cargo-gamma-unsafe/src/cgroup.rs:1982
• crates/cargo-gamma-unsafe/src/job.rs:152-164
• crates/cargo-gamma-unsafe/src/platform_error.rs:73-75
• crates/cargo-gamma/src/real_host.rs:47-53
• crates/cargo-gamma/tests/binary.rs:22
• crates/cargo-gamma/tests/binary.rs:30
• crates/cargo-gamma/tests/binary.rs:111
• crates/cargo-gamma-rt/src/runtime.rs:259-271
• crates/cargo-gamma-rt/src/runtime.rs:297-316
• crates/cargo-gamma-rt/src/runtime.rs:368-400

Reviewed in: #113

---

Title: Keep all Error implementation blocks together

[Copilot speaking]

The new Parts type is placed between Error's inherent implementation and its Display, StdError, and From<io::Error> implementations. This splits the blocks belonging to the primary error type with a different type definition.

Location: crates/cargo-gamma-engine/src/error.rs:95-110

Reproducible reasoning: The code-layout guidance requires different blocks associated with the same type to stay together. In this file, impl Error ends before Parts, but the subsequent impl Display for Error, impl StdError for Error, and impl From<io::Error> for Error remain below Parts. A reader following Error therefore has to cross the unrelated Parts definition before reaching the rest of Error's behavior.

Consequence: Future maintenance of the engine error can overlook one of its separated trait implementations, and readers cannot inspect the complete behavior of the primary type as one contiguous unit.

Recommended action: Move Parts below the Display, StdError, and From<io::Error> implementations so every block associated with Error is contiguous, while keeping the Parts definition and its own future implementations together.

Impacted locations:

• crates/cargo-gamma-engine/src/error.rs:95-136

Reviewed in: #113

---

Title: Define the discovery pre-pass terminology before using it

[Copilot speaking]

The new design boundary introduces several implementation-specific terms—“pre-pass”, “stated-value audit”, “numeric/import indexes”, and “fused entry point”—without defining them or identifying the fused API. The preceding Purpose and Boundaries text describes mutation discovery generally, but it does not give a reader enough context to distinguish these passes or understand how they relate.

Location: crates/cargo-gamma-engine/docs/DESIGN.md:23-30

Reproducible reasoning: A reader encounters this vocabulary for the first time in lines 23–30. The paragraph then contrasts the unnamed “fused entry point” with check_stated, but neither entry point is introduced and the role of the indexes is not explained. Understanding the boundary therefore requires finding and reverse-engineering implementation modules outside the design document, contrary to the requirement that specialized terms be defined before use.

Consequence: A reader can miss that the configuration-aware combined operation and the standalone stated-value checker intentionally inspect different regions, or may mistake internal pass names for previously established design concepts.

Recommended action: Introduce these concepts in plain language before this boundary, or rewrite the boundary to define each pass as it is named. Identify and link the combined API if “fused entry point” means check_stated_and_collect_with, and briefly state what evidence the numeric/import indexes provide.

Impacted locations:

• crates/cargo-gamma-engine/docs/DESIGN.md:23-30

Reviewed in: #113

---

Title: Describe MutantId compatibility at the actual serialization boundary

[Copilot speaking]

The new rustdoc attributes artifact compatibility to two different mechanisms more broadly than the implementation supports. The type documentation says Serde transparency means every plan, record, and report written by an earlier version still reads unchanged, although transparency only preserves this field's string-shaped wire representation and cannot guarantee compatibility of the surrounding artifact schema. The MutantId::new documentation then says accepting arbitrary text prevents future-version records from failing to read, but derived transparent deserialization constructs the wrapped CompactString directly and does not call new.

Location: crates/cargo-gamma-engine/src/model/identity.rs:31-38

Reproducible reasoning: The type derives Deserialize with #[serde(transparent)], so Serde deserializes the single CompactString field without routing the value through the inherent constructor. Consequently, validation added or omitted in MutantId::new has no effect on reading persisted records. Transparency also establishes only that MutantId itself has the same serialized representation as its inner string; successful deserialization of a complete plan, record, or report still depends on every other field and schema rule in that artifact. The current wording therefore promises artifact-wide backward compatibility and explains constructor behavior using a path the implementation does not take.

Consequence: A maintainer can rely on the comment as evidence that arbitrary older or future artifacts are supported, or preserve the constructor's permissive behavior to protect deserialization, even though neither conclusion follows from this code. That can turn later schema or validation work into an accidental compatibility promise and obscure where persisted-input compatibility is actually enforced.

Recommended action: Narrow the type documentation to the contractual fact that MutantId remains serialized as its underlying string. Document new only as constructing an opaque identity from already rendered text and state the actual reason it accepts arbitrary text, without tying that choice to Serde record reads. Put any whole-artifact backward- or forward-compatibility guarantee on the artifact formats that implement and test it.

Impacted locations:

• crates/cargo-gamma-engine/src/model/identity.rs:23-38

Reviewed in: #113

---

Title: Describe the identity contract without repeating its numeric version

[Copilot speaking]

The documentation for MUTANT_ID_VERSION names the newly assigned numeric version immediately above the constant that stores the same value. The useful part of the comment is the normalization change and its compatibility effect; repeating the numeric literal creates a second copy that must be kept synchronized.

Location: crates/cargo-gamma-engine/src/model/identity.rs:185-191

Reproducible reasoning: The constant declaration is the authoritative version value. Its new documentation says Version 5 while the declaration says MUTANT_ID_VERSION: u32 = 5. If a later identity change increments only the constant or only the prose, readers receive conflicting information. The surrounding explanation can remain independently useful by describing the current normalization contract and, if historical compatibility needs to be retained, referring to the preceding contract without restating the current literal.

Consequence: A future version increment can leave the comment claiming a different emitted identity version than the value serialized into campaign artifacts, making compatibility investigations harder.

Recommended action: Rewrite the newly added paragraph to explain the current self-type normalization and compatibility boundary without spelling out the constant's numeric value. Keep the declaration as the single source of truth for that value.

Impacted locations:

• crates/cargo-gamma-engine/src/model/identity.rs:185-191

Reviewed in: #113

---

Title: Document the trait implementation metadata on MutantDefinition

[Copilot speaking]

The new public trait_impl field on MutantDefinition has no field documentation, even though its representation is intentionally narrower than a full trait path and it exists for a specific policy boundary. The corresponding Candidate field explains that it stores the terminal trait name separately from item_path so policy can select implementations without parsing the identity, but that contract is absent where the metadata becomes part of the exported mutation definition.

Location: crates/cargo-gamma-engine/src/model/mutant_definition.rs:22

Reproducible reasoning: MutantDefinition is publicly re-exported from the engine model, and trait_impl: Option<Arc<str>> alone does not reveal whether the text is qualified, normalized, or intended for display, identity, or selection. The collector records only the last path segment, then the definition carries that value across the engine boundary. Without a local explanation, a maintainer or consumer must trace the collector and candidate types to reproduce the field's meaning and may reasonably infer the wrong representation.

Consequence: A consumer can treat trait_impl as a fully qualified trait path or fold it into identity logic, causing qualified traits with the same terminal name to be interpreted differently from the documented selection policy.

Recommended action: Add field documentation to MutantDefinition::trait_impl stating that it is the optional terminal name of the enclosing implemented trait and explaining that it is kept separately for trait-implementation selection policy rather than as a full path or identity component.

References:

• crates/cargo-gamma-engine/src/ops/collect/candidate.rs

Impacted locations:

• crates/cargo-gamma-engine/src/model/mutant_definition.rs:22
• crates/cargo-gamma-engine/src/ops/collect/candidate.rs:34-40

Reviewed in: #113

---

Title: Inline the new public trivial forwarders and accessors

[Copilot speaking]

New public APIs across the cargo-gamma crates add methods and trait implementations whose complete bodies only return a field, expose an underlying scalar, compare wrapped text, or forward to one existing operation. These include the supported MutantId and SiteIndex accessors and conversions, engine error and source-position accessors, control-encoding wrappers, the RunRecord iterator adapter, the Unix effective-user wrapper, and PlatformError accessors. They lack #[inline] even though downstream crates call them across crate boundaries.

Location: crates/cargo-gamma-engine/src/model/identity.rs:43-54

Reproducible reasoning: For a trivial public wrapper, #[inline] makes the body available to a downstream optimizer without requiring link-time optimization; it is an optimization opportunity rather than a guarantee that every call disappears. Each affected body has no independent algorithm or error path, so exposing it does not duplicate substantial code. This consolidation excludes test-only or representation-probing APIs that should instead be removed or narrowed rather than optimized.

Consequence: Ordinary non-LTO downstream builds may retain avoidable calls at small abstraction boundaries used while scanning source, comparing identities, rendering output, iterating records, or classifying platform failures.

Recommended action: Add #[inline] to the listed production-supported trivial public methods and trait implementations. Do not use #[inline(always)], and do not annotate an API that is being removed or narrowed for visibility reasons; the recommendation applies only to the supported cross-crate forwarders that remain.

Impacted locations:

• crates/cargo-gamma-engine/src/error.rs:68-70
• crates/cargo-gamma-engine/src/model/identity.rs:43-140
• crates/cargo-gamma-engine/src/model/identity.rs:164-179
• crates/cargo-gamma-engine/src/parse/source_file.rs:161-191
• crates/cargo-gamma-engine/src/schema.rs:205-215
• crates/cargo-gamma-engine/src/text.rs:44-46
• crates/cargo-gamma-engine/src/text.rs:56-58
• crates/cargo-gamma-lib/src/discover/record.rs:528-530
• crates/cargo-gamma-unsafe/src/identity.rs:16-20
• crates/cargo-gamma-unsafe/src/platform_error.rs:53-62

Reviewed in: #113

---

Title: Version mutation reports before changing the mutant-ID scheme

[Copilot speaking]

MUTANT_ID_VERSION advances to version 5 because implementation scopes now produce different IDs, but cargo-gamma reports do not record the identity scheme that produced their mutant IDs. The report reader and merger accept reports across cargo-gamma versions and union them by the opaque ID text alone. SARIF explicitly versions the fingerprint key, so that output recognizes the namespace change, while the report/merge path has no equivalent transition marker.

Location: crates/cargo-gamma-engine/src/model/identity.rs:187-194

Reproducible reasoning: For an implementation affected by the new scope normalization, the same logical mutant has one ID in a version-4 report and another in a version-5 report. merge keys verdicts by mutant.id and can withdraw an absent old ID only when an unsharded report supplies a complete current population. Its documented shard behavior deliberately keeps every ID admissible when only shards are available. Because neither Report nor RunInfo carries MUTANT_ID_VERSION, and the merge does not use framework.version to establish identity compatibility, a rotation containing pre-upgrade and post-upgrade shards cannot determine that the two IDs represent one mutant. Both survive as distinct entries even when they refer to the same source construct.

Consequence: A normal sharded CI rotation spanning the upgrade can count an affected implementation mutant twice, retain its stale version-4 verdict beside its version-5 result, and distort the merged score until a complete version-5 population happens to withdraw the legacy ID.

Recommended action: Record the mutant-ID scheme version in cargo-gamma report metadata and define an explicit merge policy for mixed schemes. For example, treat reports without the field as the legacy scheme and refuse or isolate mixed-version shard merges unless a complete current population can retire the legacy IDs. Document the upgrade effect so users know whether they need a complete run or must start a new report rotation.

References:

• crates/cargo-gamma/docs/DESIGN.md — Identity and knowledge across campaigns

Impacted locations:

• crates/cargo-gamma-engine/src/model/identity.rs:187-194
• crates/cargo-gamma-lib/src/elements/report.rs:44-81
• crates/cargo-gamma-lib/src/merge/union.rs:15-20
• crates/cargo-gamma-lib/src/merge/union.rs:72-81

Reviewed in: #113

---

Title: Call item_path an item path rather than a human-readable identity

[Copilot speaking]

The new trait_impl field documentation first names item_path and then calls the same value a "human-readable identity." In this module, identity already has the distinct, explicitly defined meaning represented by MutantId, while the surrounding APIs consistently call this value an item path.

Location: crates/cargo-gamma-engine/src/ops/collect/candidate.rs:38-39

Reproducible reasoning: The documentation says trait_impl avoids parsing a human-readable identity, but the value it is contrasted with is specifically item_path. A reader therefore has to decide whether "identity" means the item path, the hashed MutantId, or another representation. Reusing the established term "item path" removes that ambiguity and preserves the distinction between the descriptive scope text and the content-addressed mutant identity.

Consequence: A maintainer can incorrectly infer that trait-implementation selection would otherwise parse MutantId, or introduce more code and documentation using two terms for the same item-path concept.

Recommended action: Replace "human-readable identity" with "item path" (or refer directly to item_path) so the documentation uses the established term and leaves "identity" for MutantId.

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/candidate.rs:36-40

Reviewed in: #113

---

Title: Preserve token boundaries when normalizing implementation self types

[Copilot speaking]

impl_scope now passes an arbitrary implementation self type through compact_path, which removes every whitespace character and comment without preserving lexical token boundaries. That normalization is safe for ordinary paths separated by punctuation, but it is not injective for the full Rust Type syntax accepted as an implementation self type.

Location: crates/cargo-gamma-engine/src/ops/collect/collector.rs:466

Reproducible reasoning: For example, dyn Marker is a trait-object type while dynMarker can be a distinct nominal type identifier. Both normalize to the same text, dynMarker. Consequently, implementations such as impl Subject for dyn Marker and impl Subject for dynMarker receive the same scope string when their method names match. into_definitions then disambiguates identical site keys with a source-order occurrence index, so reordering those implementation blocks can transfer stable mutant identifiers between different self types. This contradicts the new identity design's purpose of representing the complete self type and eliminating source-order dependence.

Consequence: Cached verdicts, shard assignment, configured expectations, or report fingerprints can become associated with the wrong implementation after otherwise behavior-preserving implementation reordering.

Recommended action: Normalize the self type with a token-aware representation that removes trivia while retaining separators wherever removing trivia would merge adjacent Rust tokens. Preserve the current formatting-insensitivity, and add a regression that gives dyn Marker and a nominal dynMarker identical method bodies and verifies their identities remain distinct and stable when the implementation blocks are reordered.

References:

• crates/cargo-gamma-engine/docs/DESIGN.md
• crates/cargo-gamma-engine/src/model/identity.rs

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/collector.rs:466-467
• crates/cargo-gamma-engine/src/ops/collect/collector.rs:1194-1229

Reviewed in: #113

---

Title: Use the established “terminal identifier” term for trait matching

[Copilot speaking]

The new trait_impl field documentation calls the stored value the trait's “terminal name”, while the user-facing configuration contract consistently defines the same value as the implemented trait's “terminal identifier”. Both refer to the final written path segment, such as Debug in core::fmt::Debug, so the two terms do not identify different concepts.

Location: crates/cargo-gamma-engine/src/ops/collect/collector.rs:93-94

Reproducible reasoning: The configuration documentation introduces “terminal identifier” as the term for the value matched by trait-impl rules and explains its qualification and alias behavior. The new collector comment describes the value populated from path.segments.last() as a “terminal name”. Because the implementation stores exactly the documented terminal identifier and does not define a separate meaning for “name”, using a synonym here makes readers determine whether the wording signals a semantic distinction.

Consequence: Maintainers tracing trait-exclusion behavior from configuration into candidate collection can incorrectly suspect that trait_impl uses a different normalization rule from the documented terminal-identifier contract.

Recommended action: Change the field comment to call the value the “terminal identifier of the enclosing implemented trait”, matching the established configuration terminology.

References:

• crates/cargo-gamma/docs/CONFIG.md, [[exclude-mutants]] contract
• crates/cargo-gamma/README.md, trait implementation exclusions

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/collector.rs:93-94

Reviewed in: #113

---

Title: Describe the prefilter without duplicating its literal

[Copilot speaking]

The new prefilter comment repeats the exact leak string used by both contains calls. The rationale is useful, but spelling the implementation value again in prose creates two representations of the same decision that must remain synchronized.

Location: crates/cargo-gamma-engine/src/ops/collect/collector/noop.rs:57-62

Reproducible reasoning: Lines 57-61 explain that matching depends on a literal identifier and name that identifier explicitly; line 62 independently embeds the same spelling twice in executable code. A future change to the recognized method name or to a shared identifier can update the code while leaving the prose behind, so the comment no longer reliably communicates the decision it is intended to justify.

Consequence: A maintainer can read a stale method name in the rationale after changing the prefilter, obscuring which identifier the optimization is actually required to detect.

Recommended action: Keep the correctness and performance rationale, but refer to the required terminal method identifier checked by path_ends_with instead of repeating its spelling. Alternatively, introduce one suitably named constant and have the code refer to that single source of truth.

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/collector/noop.rs:57-62

Reviewed in: #113

---

Title: Borrow the configuration set in the fused pre-pass

[Copilot speaking]

The fused phase-one pass makes two owned copies of the build configuration for every source file: Walk::new(selection, cfg) clones it into Walk, and PhaseOne clones it again for the shared traversal gate. CfgSet owns three hash sets containing strings and string pairs, while both visitors only read it.

Location: crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:66-67

Reproducible reasoning: traversal::collect invokes phase_one::run once per parsed source file. At lines 66-67, constructing PhaseOne calls Walk::new and also explicitly clones cfg; Walk::new performs its clone at indexes.rs:479. Cloning a populated CfgSet allocates new hash-table storage and copies its owned names and key/value strings. This happens even when the selected mutators consult no numeric or import indexes, because Walk::new clones the set before the visitor can determine that those indexes are disabled. The configuration reference passed to run remains valid for the entire synchronous traversal, so neither owned copy is required.

Consequence: On a workspace with many source files, discovery performs repeated allocations and copies proportional to the number of active configuration names and feature pairs for every file. This can offset part of the allocation and traversal reduction that the fused pre-pass is intended to provide, including for selections that do not use the indexes at all.

Recommended action: Give Walk and PhaseOne a lifetime and store the existing &CfgSet in both visitors instead of cloning it. This adds lifetime parameters to these internal visitor types but preserves the current behavior while removing the per-file hash-set and string copies.

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:66-67
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:464-479

Reviewed in: #113

---

Title: Avoid re-interning the trait name for every candidate

[Copilot speaking]

Each candidate inside a trait implementation already carries an Arc<str> cloned from the collector's implementation-level trait_impl. into_definitions discards that sharing boundary and calls Interner::text again for every candidate. That call performs a hash-table lookup and another reference-counted clone for every mutant, and the first occurrence also allocates a replacement Arc<str> plus the interner's owned key.

Location: crates/cargo-gamma-engine/src/ops/collect/definitions.rs:83

Reproducible reasoning: The collector creates one trait-name Arc<str> when entering an impl, clones that same pointer into every emitted Candidate, and restores the previous value when leaving the impl. In into_definitions, line 83 borrows each candidate's already-shared pointer, probes Interner::texts through Interner::text, creates or clones a second pointer, and then drops the candidate's pointer. The repository's own interner documentation says a run can contain hundreds of thousands of mutations, so this converts implementation-level work into per-mutant hashing and atomic reference-count churn. Cross-implementation deduplication only needs work once per distinct incoming trait value or implementation scope, not once per candidate.

Consequence: A source file with large trait implementations pays an avoidable hash lookup and extra Arc clone/drop cycle for every generated mutant, increasing discovery CPU cost on the same high-cardinality path that the surrounding sharing logic is intended to optimize.

Recommended action: Preserve the existing candidate-owned Arc<str> when constructing MutantDefinition, or intern/cache trait names at implementation scope before candidates are emitted and then move the candidate pointer into the definition. If collapsing identical trait names across separate implementations is important, retain that deduplication while ensuring the hash lookup happens once per distinct implementation-level pointer rather than once per mutant.

References:

• crates/cargo-gamma-engine/src/model/interner.rs

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/definitions.rs:83

Reviewed in: #113

---

Title: Keep the Audit implementation blocks adjacent

[Copilot speaking]

The new inert_reason free function is placed between Audit's inherent implementation and its Visit implementation. This separates two blocks that together define the behavior of the same type, even though the layout guideline requires associated blocks for a type to remain together.

Location: crates/cargo-gamma-engine/src/ops/collect/stated.rs:221-229

Reproducible reasoning: impl Audit ends at line 212, the newly added inert_reason function occupies lines 214-229, and impl Visit for Audit begins at line 235. Reading or searching through Audit therefore reaches an intervening free function before the remainder of the type's implementation. Moving the helper before the Audit declaration or after the Visit implementation preserves the exact dependency and behavior while restoring one contiguous type-oriented block.

Consequence: A maintainer inspecting Audit can overlook its visitor behavior or must jump across unrelated item boundaries to understand the complete type, making this diagnostic traversal harder to navigate and maintain.

Recommended action: Move inert_reason outside the sequence of Audit implementation blocks, for example immediately after the Visit implementation, so the inherent and trait implementations remain adjacent.

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/stated.rs:148-235

Reviewed in: #113

[sandersaares]

[Copilot speaking]

Review findings (continued 2)

Title: Use the established discovery terminology in stated-value documentation

[Copilot speaking]

The new documentation repeatedly uses "collection" in sentence structures such as "a function collection never reads" and "a function collection reaches". In these passages it is unclear whether "function collection" is intended as a noun phrase, whether collection is the actor, or whether the text refers to the mutation-discovery traversal. The engine design document describes the same behavior as discovery not reading a stated value.

Location: crates/cargo-gamma-engine/src/ops/collect/stated.rs:85-87

Reproducible reasoning: The public error documentation at lines 85-87 intends to identify functions whose stated values are ignored, but the phrase "written on a function collection never reads a value from" lacks the connector needed to establish that relationship. The internal documentation at lines 151-152 and 214-215 repeats the same ambiguity. Elsewhere, crates/cargo-gamma-engine/docs/DESIGN.md uses the established term "discovery" for this operation: "A stated value is reported as an error where discovery would never read it." Reusing that term makes the subject and behavior explicit and keeps the implementation documentation aligned with the design vocabulary.

Consequence: Readers can parse "function collection" as a collection of functions rather than the discovery process, leaving the error contract and the purpose of inert_reason unnecessarily difficult to understand.

Recommended action: Rewrite the affected sentences around the established discovery term, for example "written on a function from which discovery never reads a value", "inert explains why discovery never reads a stated value from the function", and "Returns why discovery would never read a stated value from a function it reaches."

References:

• crates/cargo-gamma-engine/docs/DESIGN.md, stated-value discovery behavior

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/stated.rs:85-87
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:149-153
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:214-218

Reviewed in: #113

---

Title: Place CandidateKey before the helper functions that use it

[Copilot speaking]

The new CandidateKey type alias is declared after separately_and_fused, while candidate_key—the function that constructs this exact tuple—remains earlier in the file with the tuple type written out directly. The type and its associated helpers are therefore split, and the type declaration is buried below direct function members.

Location: crates/cargo-gamma-engine/src/ops/collect/tests.rs:3395-3397

Reproducible reasoning: Reading the module from top to bottom first exposes the tuple shape in candidate_key, later exposes CandidateKey in the signature of separately_and_fused, and only then defines what CandidateKey means. Moving the alias before both helpers and using it as candidate_key's return type presents the abstraction before its operations and keeps the related declarations together.

Consequence: A future change to the candidate comparison key can update the alias but overlook the duplicated tuple return type, allowing the helper and the alias to drift or forcing readers to compare two long tuple signatures manually.

Recommended action: Move CandidateKey above candidate_key (or into the module's type-declaration area near the top) and change candidate_key to return CandidateKey.

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3293-3302
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3381-3397

Reviewed in: #113

---

Title: Rewrite the CandidateKey comment as a complete definition

[Copilot speaking]

The new type-level comment says, "The tuple candidate_key reduces a candidate to, named so..." This is not a complete grammatical sentence, and it describes candidate_key as though it were the tuple even though that name identifies the conversion function. The comment therefore does not clearly introduce what the CandidateKey alias represents.

Location: crates/cargo-gamma-engine/src/ops/collect/tests.rs:3395-3397

Reproducible reasoning: CandidateKey is a specialized tuple alias used by the new comparison helper, so its comment should define the alias before explaining why it exists. In the current wording, "named" has no clear subject and the placement of the linked function name makes the function appear to be the tuple. A reader must inspect the function signature and the helper's return type to reconstruct the intended meaning: the alias names the tuple returned by candidate_key so two collections can use that type without repeating it.

Consequence: Readers encountering CandidateKey cannot rely on its documentation to distinguish the alias from the candidate_key conversion function, making an already dense comparison helper unnecessarily difficult to understand.

Recommended action: Rewrite the comment as a direct definition, for example: "The identifying fields returned by candidate_key. The alias lets separately_and_fused return both candidate lists without repeating the tuple type."

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3395-3397

Reviewed in: #113

---

Title: Align the fused collection traversal claim with its implementation

[Copilot speaking]

The public documentation for check_stated_and_collect_with says that stated-value checking and candidate collection happen in one syntax-tree walk. The expanded explanation in the same documentation instead describes two passes, and the implementation performs them separately: phase_one::run visits the file once to combine the audit and indexes, then finish drives the candidate collector over the file again. The exact traversal count is an implementation choice rather than part of the function's behavioral contract, and the current API documentation promises a behavior the implementation does not provide.

Location: crates/cargo-gamma-engine/src/ops/collect/traversal.rs:51-66

Reproducible reasoning: The opening sentence at lines 51-52 claims one syntax-tree walk. Lines 64-66 then distinguish the combined pre-pass from the candidate-collecting pass. phase_one::run calls combined.visit_file, after which finish calls collector.visit_file; therefore one invocation performs two syntax-tree walks. Because the API's observable contract is which errors and candidates are returned, documenting the internal traversal count both creates a false statement and unnecessarily turns an optimization strategy into an API promise.

Consequence: A caller or maintainer evaluating the cost of discovery can rely on the documented single traversal even though the function traverses the syntax tree twice, and a later refactoring can appear to break a public promise despite preserving every observable result.

Recommended action: Make the API documentation and implementation agree without treating the traversal count as incidental in one place and contractual in another. If a single syntax-tree walk is a required performance property, restructure collection so auditing, index construction, and candidate collection share that traversal and add a test or benchmark that protects it. Otherwise remove the exact one-walk claim and document only the behavioral equivalence and cfg-aware audit boundary; retain the two-pass topology in implementation documentation if it remains useful.

References:

• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/traversal.rs:51-66
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:4-12
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:63-70

Reviewed in: #113

---

Title: Keep expected coordinate values in one place

[Copilot speaking]

The new Unicode guard-position test repeats each expected coordinate and its arithmetic in comments immediately above assertions that encode the same values. The comments state the site start and end columns and the replacement start and end columns, while the assertions separately contain those endpoints.

Location: crates/cargo-gamma-engine/src/schema.rs:648-654

Reproducible reasoning: The maintainability guidance requires comments to justify literal choices without duplicating the literal value, including recomputed or derived forms. Here the function-level documentation already provides the useful rationale: the expectation is independently hand-derived from the exact guard template so the test can detect byte-counting and one-based-coordinate regressions. The additional inline comments then duplicate the assertion values as arithmetic results. A future template or fixture adjustment can update the assertions without updating the prose, leaving an authoritative-looking derivation that contradicts the executable expectation.

Consequence: A later change to the guard template or fixture can leave stale coordinate calculations in the comments, making maintainers verify or debug the test against misleading documentation.

Recommended action: Retain the explanation that expectations are independently derived from the fixture and rendered template, but remove the comments' repeated numeric endpoints and arithmetic results. Keep each expected coordinate represented only by executable test data, or calculate it through clearly named independent test helpers if preserving the derivation in code is valuable.

Impacted locations:

• crates/cargo-gamma-engine/src/schema.rs:648-657

Reviewed in: #113

---

Title: Place the public schema types before private position-sweep helpers

[Copilot speaking]

The newly added private Slot and Slots implementation types appear at the top of schema.rs, before the public Position, Guard, Ordinal, and AssignedMutant types that define the schema API. This makes low-level bookkeeping the first type-level concept presented by a module whose primary role is instrumentation and guard metadata.

Location: crates/cargo-gamma-engine/src/schema.rs:55-72

Reproducible reasoning: The file-order guideline requires types to be sorted by importance in descending order. Position, Guard, Ordinal, and AssignedMutant are public types used by callers to construct instrumentation inputs and consume guard locations, while Slot and Slots exist only to support the private positions sweep. Lines 55-72 introduce those private helpers before every public type in the file. Rust does not require a type to be declared before functions that use it, so the helpers and positions can be placed after the public schema types without changing dependencies or behavior.

Consequence: A reader entering the module encounters transient position-resolution machinery before the durable API concepts, making the file harder to navigate and obscuring which types are central to the schema contract.

Recommended action: Move Position, Guard, Ordinal, and AssignedMutant, with each type's associated implementation block, ahead of Slot, Slots, and the private position-resolution implementation. Keep the private helper types next to positions so their limited role remains clear.

Impacted locations:

• crates/cargo-gamma-engine/src/schema.rs:55-72
• crates/cargo-gamma-engine/src/schema.rs:163-299

Reviewed in: #113

---

Title: Define the sub-family selector term

[Copilot speaking]

The newly added resolve error documentation says that a selector may be a "family or sub-family prefix," while the immediately preceding selector definition lists only a family prefix. Neither this registry module nor the user-facing mutator catalog defines a sub-family; the catalog defines a family as the first component of family.transform and likewise describes selectors as family prefixes. The implementation also accepts intermediate dotted prefixes such as combinator.iter, so the new wording appears to name a real additional concept without explaining it.

Location: crates/cargo-gamma-engine/src/ops/registry/lookup.rs:46-48

Reproducible reasoning: families() returns only the first component of each registered name, establishing the existing meaning of family. resolve() instead appends a dot to any selector and accepts every registered name beginning with that string, which also admits intermediate dotted prefixes. The added Errors section introduces "sub-family prefix" for that latter case, but the public selector summary and the referenced catalog do not distinguish it from a family prefix. Readers therefore cannot reproduce the terminology distinction from the documentation even though the implementation distinguishes the two forms.

Consequence: An API consumer may conclude that only top-level family names are supported, or may invent incompatible validation rules because the syntax and meaning of a sub-family are unspecified.

Recommended action: Use one defined term consistently, such as "mutator-name prefix," or define a sub-family as an intermediate dotted prefix and update the selector summary with an example such as combinator.iter.

References:

• crates/cargo-gamma/docs/MUTATORS.md

Impacted locations:

• crates/cargo-gamma-engine/src/ops/registry/lookup.rs:40-48

Reviewed in: #113

---

Title: Replace the undefined blame terminology with diagnostic attribution

[Copilot speaking]

The new public documentation for Position refers to "the blame that decides which mutant a build error belongs to," and a new test comment later calls this the "blame pass." The engine's source and design documentation do not define blame as a component or concept; within this module, the surrounding Guard and Position documentation otherwise describes attribution of compiler diagnostics to mutants. In ordinary professional English, blame also suggests responsibility rather than a diagnostic-mapping algorithm.

Location: crates/cargo-gamma-engine/src/schema.rs:162-165

Reproducible reasoning: A search of the engine source and design documentation finds blame only in these two newly added comments. The public Position documentation therefore uses a specialized term before any definition, without a link to an API or module that would establish its meaning. Replacing it with the operation already described by the surrounding documentation makes the invariant reproducible: positions are ordered and nonzero so diagnostic attribution can determine which guard contains a compiler location.

Consequence: Readers of the public API may search for a nonexistent blame facility or misunderstand which consumer depends on the coordinate invariant, making the purpose of Position less clear.

Recommended action: Describe the consumer as the diagnostic-attribution logic or pass in both comments, or link and define a concrete component if blame is intended to be a formal project term.

Impacted locations:

• crates/cargo-gamma-engine/src/schema.rs:162-165
• crates/cargo-gamma-engine/src/schema.rs:691-692

Reviewed in: #113

---

Title: Align the terminal relay policy with the SGR sequences it preserves

[Copilot speaking]

The engine and Progress::relay documentation describe a color-only relay, but color_sequence_end preserves every syntactically shaped Select Graphic Rendition (SGR) sequence—digits, semicolons, or colons followed by m. SGR includes persistent presentation effects beyond color; for example ESC[8m enables concealment. The relay writes the accepted sequence directly and does not append a trusted reset at the end of each relayed line.

Location: crates/cargo-gamma-engine/src/text.rs:164-168

Reproducible reasoning: encode_preserving_color delegates to color_sequence_end, which validates only SGR syntax and never interprets parameter values. Progress::relay then sends the returned text to the terminal. Thus the engine contributes broad SGR acceptance, while the progress layer contributes direct rendering without resetting terminal state. A repository-controlled compiler wrapper can emit concealment or another persistent style that affects later cargo-gamma output, despite the documentation saying only color is preserved and other terminal effects are encoded.

Consequence: Repository-controlled build output can hide or alter the presentation of later diagnostics, verdicts, or progress text, and maintainers are given a narrower security boundary than the code enforces.

Recommended action: Choose and enforce one relay contract. If only color and a deliberately selected emphasis subset are trusted, parse SGR parameters, preserve only that allowlist, reject effects such as concealment, and end each relayed line with a trusted reset. If general SGR styling is intentionally accepted, document that broader boundary consistently and still contain style leakage with a reset or equivalent scoped rendering. Add tests for concealment and state leakage whichever policy is selected.

References:

• crates/cargo-gamma/docs/DESIGN.md, Reports and integrations
• XTerm Control Sequences

Impacted locations:

• crates/cargo-gamma-engine/src/text.rs:19-22
• crates/cargo-gamma-engine/src/text.rs:164-168
• crates/cargo-gamma-lib/src/report/progress.rs:34-39
• crates/cargo-gamma-lib/src/report/progress.rs:356-362
• crates/cargo-gamma-lib/src/report/progress.rs:1428-1430

Reviewed in: #113

---

Title: Remove or implement the reversible-encoding promise

[Copilot speaking]

The module documentation says control-character encoding is reversible by eye, but printable backslashes are left unchanged. A literal two-character \n in repository-controlled text is therefore rendered identically to an actual newline encoded as \n, and the same ambiguity exists for \e, \0, and \u{...} spellings.

Location: crates/cargo-gamma-engine/src/text.rs:13-15

Reproducible reasoning: encode returns ordinary printable bytes unchanged, while push_escape represents control characters with backslash-prefixed text. Consequently an input containing the printable characters a, backslash, n, and b renders the same as an input containing a, a newline, and b. No reader can reconstruct which input produced that output, so the implementation provides safe visible rendering but not the injective representation the new rustdoc promises.

Consequence: Diagnostics can make two distinct repository paths, identifiers, or source fragments look identical, while the API documentation tells maintainers that the original value can be recovered from the display.

Recommended action: Either weaken the documentation to promise only that controls are displayed rather than executed, matching the user-facing design, or escape literal backslashes with an unambiguous scheme if reversibility is an actual contract requirement. The documentation-only option preserves the current compact output; an injective encoding improves forensic precision but makes ordinary backslashes noisier.

References:

• crates/cargo-gamma/docs/DESIGN.md, Reports and integrations

Impacted locations:

• crates/cargo-gamma-engine/src/text.rs:13-15
• crates/cargo-gamma-engine/src/text.rs:134-147

Reviewed in: #113

---

Title: Link the CLI style constant to its documented convention

[Copilot speaking]

The new CLAP_STYLES constant selects green bold headings and usage plus cyan literals and placeholders, but the constant has no comment explaining why those particular colors and effects are the intended choices. The same decision is documented in crates/cargo-gamma/docs/DESIGN.md as the workspace Cargo-tool convention, so the implementation can point directly to that contract instead of leaving the literals unexplained.

Location: crates/cargo-gamma-lib/src/commands/cli.rs:15-19

Reproducible reasoning: Each AnsiColor and Effects value is a design choice for the user-visible help presentation; another maintainer could plausibly choose different colors or emphasis. The assigned maintainability guidance requires such literal decisions to carry either a reproducible inline rationale or a reference to the design document. The design document already names this exact style combination, but there is no reference beside CLAP_STYLES, so a maintainer working in this file must search elsewhere or infer that the values are intentional rather than incidental.

Consequence: A later help-style adjustment can silently diverge from the documented workspace convention, or a maintainer can preserve these literals without understanding the consistency requirement they implement.

Recommended action: Add a short documentation comment above CLAP_STYLES that identifies it as the workspace Cargo-tool help convention and references the command-help paragraph in crates/cargo-gamma/docs/DESIGN.md.

References:

• crates/cargo-gamma/docs/DESIGN.md, command-help convention

Impacted locations:

• crates/cargo-gamma-lib/src/commands/cli.rs:15-19

Reviewed in: #113

---

Title: Derive the C1 lead-byte constant without repeating its value

[Copilot speaking]

The documentation for C1_LEAD states that the C1 range encodes with 0xC2 as its UTF-8 lead byte immediately above the constant whose value is also 0xC2. Although the surrounding explanation usefully establishes why C1 controls must be recognized, repeating the literal in its comment creates a second copy of the value rather than documenting only the derivation.

Location: crates/cargo-gamma-engine/src/text.rs:28-36

Reproducible reasoning: The comment and constant can drift independently: changing either occurrence leaves plausible-looking documentation that disagrees with the implementation. The assigned comment guidance explicitly prohibits listing a value in any form in the comment on that value. The rationale remains independently reproducible without duplication by saying that this is the UTF-8 lead byte shared by U+0080..=U+009F; the Unicode range and UTF-8 encoding rule determine the literal in code.

Consequence: A future correction or representation change can update only one occurrence, leaving maintainers with contradictory sources for the byte used to detect C1 controls.

Recommended action: Remove the repeated hexadecimal byte from the comment while retaining the derivation from the C1 Unicode range and the explanation of why its single-byte terminal spellings must be encoded.

Impacted locations:

• crates/cargo-gamma-engine/src/text.rs:28-36

Reviewed in: #113

---

Title: Run internal-API integration tests in default test builds

[Copilot speaking]

The SARIF golden contract and the newly added or strengthened units and session assertions are placed in integration targets declared with required-features = ["internals"]. Because internals is not a default feature and the package has no self dev-dependency that activates it, an ordinary cargo test -p cargo-gamma-lib silently omits these targets instead of compiling and running the new coverage.

Location: crates/cargo-gamma-lib/Cargo.toml:110-112

Reproducible reasoning: Cargo selects a test target with required-features only when every named feature is active. The manifest's nearby feature comment describes activating internals through a self dev-dependency, but no such dependency exists. The same configuration gap therefore excludes the SARIF wire-format contract, the MutantId and SiteIndex assertions, and the cache-lock and nextest-report assertions from the default test build. Although an all-features CI job exercises them, the ordinary package test command remains a valid developer and downstream verification path and currently reports success without this coverage.

Consequence: Regressions in the SARIF wire format, newtype serialization or comparison, SiteIndex field distinctions, cache-lock handoff, or console-only nextest diagnostics can pass the ordinary package test command and be detected only when a separate feature-enabled test command runs.

Recommended action: Add a cargo-gamma-lib = { path = ".", features = ["internals"] } self dev-dependency as the manifest comment describes, then remove the required-features = ["internals"] gates from the affected integration targets so default test builds compile and run them without exposing internals in ordinary library builds. Tests that cannot use that setup should instead move into ungated unit-test locations with direct access to the implementation.

References:

• crates/cargo-gamma-lib/Cargo.toml

Impacted locations:

• crates/cargo-gamma-lib/Cargo.toml:110-112
• crates/cargo-gamma-lib/tests/sarif_contract.rs:22-24
• crates/cargo-gamma-lib/tests/units.rs:135-171
• crates/cargo-gamma-lib/tests/session.rs:389-435
• crates/cargo-gamma-lib/tests/session.rs:1023-1038

Reviewed in: #113

---

Title: Define terminal-control terminology before using its acronyms

[Copilot speaking]

The new text-safety documentation introduces OSC 8, SGR, CSI, and C1 in the module overview before defining them. The later function documentation expands SGR only indirectly as "select-graphic-rendition", while CSI and OSC remain acronyms and the C1 range is explained only after the overview. The CI annotation documentation separately uses ESC and OSC 8 without defining them.

Location: crates/cargo-gamma-engine/src/text.rs:10-22

Reproducible reasoning: These names come from terminal control standards rather than ordinary Rust or cargo-gamma vocabulary. A reader encounters them first at text.rs lines 10 and 21-22, but must already know that OSC means Operating System Command, CSI means Control Sequence Introducer, SGR means Select Graphic Rendition, and C1 identifies the U+0080 through U+009F control set to understand why one class is allowed and the others are encoded. Searching the cargo-gamma engine and library documentation finds no earlier package-level definition that these private/module docs can rely on. The annotation documentation repeats the same issue at lines 94-97. Defining each term on first use would let the documentation explain the security policy linearly and use the standard terminology consistently.

Consequence: Maintainers unfamiliar with terminal protocols can miss the distinction between permitted color rendition and terminal-addressing sequences, or interpret the phrase "operating-system command" as a generic shell command instead of the specific OSC control-string family.

Recommended action: At the first occurrence, write the established names followed by their acronyms, for example Operating System Command (OSC), Select Graphic Rendition (SGR), Control Sequence Introducer (CSI), and C1 control characters. Define ESC where the annotation documentation first uses it, or link that documentation directly to the module-level definitions, then use the acronyms consistently afterward.

References:

• XTerm control-sequence reference

Impacted locations:

• crates/cargo-gamma-engine/src/text.rs:10-22
• crates/cargo-gamma-lib/src/ci/annotations.rs:94-97

Reviewed in: #113

---

Title: Encode inconsistent-shard report paths before printing them

[Copilot speaking]

The existing review thread led the two merged-report Wrote messages to encode their paths, but report_merge still prints every path in Merged::inconsistent directly. Those strings come from the MergeArgs.inputs paths and directory entries collected by this command, so the inconsistent-shard warning remains a terminal-facing path output that bypasses the control-character encoding introduced by this pull request.

Location: crates/cargo-gamma-lib/src/commands/merge.rs

Reproducible reasoning: The merge collector stores each report's path as a plain String in the merge input. When reports disagree on shard count, the merge layer copies that string into Merged::inconsistent. The inconsistent-report branch interpolates it directly into host.error(), unlike the JSON and HTML output paths, which pass through encode_controls. The design states that path text must be control-character encoded before reaching a terminal or CI log, and this warning is emitted for an input-derived path.

Consequence: On a platform that permits control characters in filenames, a report with a mismatched shard count and a name containing a newline or terminal escape can split, erase, repaint, or forge the merge warning in an interactive terminal or CI log.

Recommended action: Pass input through encode_controls before formatting the inconsistent-shard warning, and add a regression test whose inconsistent report path contains hostile control characters so this remaining merge output path is covered by the same contract as the Wrote messages.

References:

• crates/cargo-gamma/docs/DESIGN.md, terminal and CI control-character encoding contract

Impacted locations:

• crates/cargo-gamma-lib/src/commands/merge.rs:289-294

Follows up on: #113 (comment)

Reviewed in: #113

---

Title: Rename the helper to describe a JSON document rather than JSON Lines

[Copilot speaking]

The new write_json_lines name uses established terminology for the JSON Lines/JSONL format, but the helper writes one pretty-printed JSON document followed by a trailing newline. Its current callers pass complete arrays, so the emitted format is ordinary JSON rather than one independently parseable JSON value per line.

Location: crates/cargo-gamma-lib/src/commands/list.rs:45

Reproducible reasoning: serde_json::to_writer_pretty serializes entries as a single value, and the following writeln! only terminates that document. JSON Lines instead requires each line to contain a complete JSON value and is not normally pretty-printed across multiple lines. The function documentation accurately calls the result "pretty JSON", so the function name and documented behavior use different terms for the same output.

Consequence: A maintainer can reasonably infer from the name that the CLI emits JSONL, then add callers or consumers that process one record per line; those assumptions would conflict with the existing pretty-printed array contract and could cause incompatible output or parsing failures.

Recommended action: Rename the helper to terminology such as write_pretty_json or write_json_document, preserving the current serialization behavior and trailing newline.

Impacted locations:

• crates/cargo-gamma-lib/src/commands/list.rs:37-48

Reviewed in: #113

---

Title: State the relayed-output boundary directly

[Copilot speaking]

The new justification for using relay ends with the colloquial statement that "everything else it could carry is not cargo's to say." That wording does not identify what "everything else" means or explain the actual boundary: Cargo's color controls are preserved while other terminal controls are encoded.

Location: crates/cargo-gamma-lib/src/commands/console_events.rs:84-86

Reproducible reasoning: The nearby Progress::relay contract distinguishes color sequences from all other controls because color only repaints text, whereas other controls can alter terminal state. The comment in build_output is meant to justify selecting that API, but its final clause replaces this distinction with an idiom whose subject and permitted behavior are ambiguous. A reader cannot reproduce the safety decision from the local comment without finding the callee's documentation.

Consequence: Future maintenance of build-output handling can preserve the informal wording while accidentally broadening or narrowing the allowed control sequences, because the comment does not state which content is intentionally retained and which is rejected.

Recommended action: Replace the final clause with direct professional wording that says color formatting is preserved because --show-build promises Cargo's colored output, while every other terminal control is encoded.

Impacted locations:

• crates/cargo-gamma-lib/src/commands/console_events.rs:83-87

Reviewed in: #113

---

Title: Document the cache-adoption pause boundary

[Copilot speaking]

The newly added internals hook pauses measured only when incremental state and an explicit cache directory are both present, immediately after record adoption and immediately before execution receives the cache locks. The call name identifies the boundary, but the code does not explain why this exact condition and placement are required.

Location: crates/cargo-gamma-lib/src/commands/run.rs:798-800

Reproducible reasoning: The integration test an_incremental_command_holds_its_cache_lock_from_adoption_through_preparation registers this pause, starts a competing command while the first command is stopped, and expects the competitor to fail because the cache lock is still held. That proof is valid only if the hook remains after cache adoption while the lock acquired above is still owned and before run_with_locks begins workspace preparation. A future maintainer reading run.rs cannot reproduce that constraint from the local code alone and could reasonably move the test-only hook nearer the adoption call, move it into execution, or simplify its guard without realizing that doing so changes what interval the concurrency test demonstrates.

Consequence: The concurrency test can continue passing while pausing outside the intended lock-lifetime boundary, masking a regression that releases the redirected-cache lock between adoption and workspace preparation.

Recommended action: Add an inline comment at the hook explaining that it exposes the post-adoption, pre-preparation boundary used to verify the redirected-cache lock remains held across both phases, and reference the corresponding session integration test.

References:

• crates/cargo-gamma-lib/tests/session.rs: an_incremental_command_holds_its_cache_lock_from_adoption_through_preparation

Impacted locations:

• crates/cargo-gamma-lib/src/commands/run.rs:798-800

Reviewed in: #113

---

Title: Acquire the default cache lock before incremental adoption

[Copilot speaking]

The new early-lock path is enabled only when --cache-dir is explicitly present. An incremental run using the default cache therefore still calls adopt before Workspace::prepare_with_locks acquires the workspace/default-cache lock. This leaves the default configuration outside the lock-lifetime guarantee that this change adds for redirected caches.

Location: crates/cargo-gamma-lib/src/commands/run.rs

Reproducible reasoning: incremental is populated for an ordinary default incremental run, but cache_locks remains None because args.measure.cache_dir.is_some() is false. The run then reads and applies last-gamma-run.json in adopt; only later does Workspace::prepare_with_locks handle None by calling claim_cache. Consequently, concurrent commands can both inspect or adopt the same default reusable state before either command establishes exclusive ownership. The design states that one cargo-gamma command may operate on a workspace at a time and that the default external cache contains the lock serializing those commands, so restricting the earlier claim to redirected caches does not implement that contract for the normal default-cache path.

Consequence: Two default incremental runs can both pass through cache adoption before contention is detected. One can therefore base its plan on reusable state that another command is concurrently replacing, rather than either failing immediately or observing state protected for the complete adoption-to-preparation interval.

Recommended action: Claim and pass CacheLocks for every non-dry incremental run, not only when cache_dir is set. Keep the redirected-cache branch inside claim_cache, which already decides whether a second cache-domain lock is necessary. Exercise the existing adoption/preparation concurrency scenario with the default cache as well as a redirected cache.

References:

• crates/cargo-gamma/docs/DESIGN.md, scratch workspace and cache locking contract

Impacted locations:

• crates/cargo-gamma-lib/src/commands/run.rs:750-753

Reviewed in: #113

---

Title: Enable the cache-adoption hook in test builds

[Copilot speaking]

The newly added cache-adoption pause is gated only by the internals Cargo feature, even though the testing module it calls is available under cfg(any(test, feature = "internals")). A normal unit-test compilation sets cfg(test) but does not automatically enable Cargo features, so this test-only orchestration path is removed from that build.

Location: crates/cargo-gamma-lib/src/commands/run.rs:798-801

Reproducible reasoning: The project convention for additive features is that feature-dependent code uses cfg(any(test, feature = "...")), ensuring test builds contain every feature path. At run.rs:786, the gate is cfg(feature = "internals"), while src/lib.rs exposes crate::testing under cfg(any(test, feature = "internals")) and the required dev dependencies are present. Therefore the hook can compile in both configurations, but the current narrower gate creates a test-build behavior that differs from the all-features behavior.

Consequence: A unit test compiled without explicitly enabling internals cannot exercise or detect regressions at the cache-adoption boundary, even though the shared pause infrastructure is compiled and available in that test build.

Recommended action: Change the gate to #[cfg(any(test, feature = "internals"))] so ordinary test builds include the same pause boundary while production builds continue to omit it.

References:

• crates/cargo-gamma-lib/src/lib.rs

Impacted locations:

• crates/cargo-gamma-lib/src/commands/run.rs:798-801
• crates/cargo-gamma-lib/src/lib.rs:303-308

Reviewed in: #113

---

Title: Describe the held-out configuration state without duplicating its Boolean literal

[Copilot speaking]

The new exhaustive configuration-test helper explains that test_workspace is "held false," and the companion test later describes the uncovered "true branch." Those comments restate the Boolean values used by the nearby Some(false) and Some(true) initializers instead of expressing only the semantic reason for the setup.

Location: crates/cargo-gamma-lib/src/config.rs:1022-1023

Reproducible reasoning: The assigned maintainability guidance requires comments on chosen values to justify the choice without repeating the value, because the prose can become stale independently of the code. Here the useful rationale is that test-workspace and test-packages cannot both apply, so the exhaustive helper must leave that setting in a non-conflicting state and a separate test must exercise the conflicting state. Naming the concrete Boolean states adds no explanatory information beyond the initializers and creates two descriptions that must be updated if the fixture is refactored.

Consequence: A future change can invert or restructure either fixture while leaving "held false" or "true branch" behind, causing the test documentation to describe a state the code no longer constructs and obscuring why the case is separated.

Recommended action: Rephrase the comments semantically, for example by saying the exhaustive helper leaves test_workspace in its non-conflicting state and the separate test exercises the conflicting/enabled case, without naming the literal Boolean values.

Impacted locations:

• crates/cargo-gamma-lib/src/config.rs:1018-1023
• crates/cargo-gamma-lib/src/config.rs:1278-1288

Reviewed in: #113

---

Title: Use one term for the trait exclusion identifier

[Copilot speaking]

The current trait-exclusion surface describes the same configuration value with three similar terms: Config::exclude_trait_impls documents "lexical terminal trait names", its validation error requires an "unqualified Rust identifier", and the associated test calls the values "terminal identifiers". The API documentation does not define whether these word choices describe different constraints.

Location: crates/cargo-gamma-lib/src/config.rs:56-57

Reproducible reasoning: Each value is validated as one syn::Ident and matching compares it with the final written trait-path segment, so the phrases refer to one concept rather than separate concepts. Alternating between name and identifier, and between lexical terminal and unqualified, makes readers infer equivalence that the public field does not state. Consistent terminology would make the syntactic and matching contract visible without relying on the validation error.

Consequence: A caller reading the public configuration type can remain unsure whether an entry accepts a qualified trait path, a general trait name, or specifically one Rust identifier, and must rely on a runtime configuration error to learn the actual restriction.

Recommended action: Choose one term and use it consistently in the public field documentation, validation message, and test name. Prefer wording such as "the unqualified Rust identifier forming the final written trait-path segment" and define any retained terminal-name shorthand at first use.

References:

• crates/cargo-gamma/docs/CONFIG.md

Impacted locations:

• crates/cargo-gamma-lib/src/config.rs:56-57
• crates/cargo-gamma-lib/src/config.rs:270-276
• crates/cargo-gamma-lib/src/config.rs:748-754

Reviewed in: #113

---

Title: State the read_from error conditions in terms of its arguments

[Copilot speaking]

The new # Errors text says that "- names a stream that fails part way through or is not UTF-8". A - path selects the supplied input; it does not name a stream whose defining characteristic is failure, and the sentence does not directly connect the possible read or UTF-8 failure to the input argument.

Location: crates/cargo-gamma-lib/src/discover/diff.rs:44-45

Reproducible reasoning: The implementation branches on path == "-" and then calls input.read_to_string; otherwise it calls fs::read_to_string(path). Describing those two branches directly produces a reproducible and unambiguous API contract. The current grammatical construction instead makes the failure predicates modify "stream", which can be read as describing what - denotes rather than when the method returns an error.

Consequence: API readers can misunderstand which reader is used for - and whether any supplied reader is accepted, particularly because read_from deliberately differs from read by accepting an arbitrary input.

Recommended action: Rewrite the error documentation around the parameters, for example: return an error when path is - and reading input fails or its bytes are not UTF-8, or when another path cannot be opened, read, or decoded as UTF-8.

Impacted locations:

• crates/cargo-gamma-lib/src/discover/diff.rs:44-45

Reviewed in: #113

---

Title: Describe the iterator without undefined or consumption-oriented terms

[Copilot speaking]

The new iterator documentation says values are returned "in file order and then in record order," but "record order" is not defined and does not identify the ordering within each file. The field documentation also calls a borrowed slice iterator "the file currently being drained," although the field is an iterator over entries and advancing it removes nothing from the record.

Location: crates/cargo-gamma-lib/src/discover/record.rs:495-504

Reproducible reasoning: Entries::next advances self.mutants, which is a core::slice::Iter<Entry>, until that iterator is exhausted, then obtains the next RecordedFile from self.files. Therefore the observable order is the stored order of files followed by the stored order of entries within each file. In Rust, "drain" conventionally describes consuming or removing elements, as with Vec::drain; it is inaccurate for immutable slice iteration and also misidentifies the field as a file rather than an iterator over that file's entries.

Consequence: Callers cannot derive the iterator's precise ordering guarantee from the public description, and maintainers may incorrectly infer that iterating mutates or consumes record contents.

Recommended action: Describe the order as files in stored order and entries within each file in stored order. Describe mutants as the iterator over entries in the current file, using terms such as "iterated" or "exhausted" rather than "drained."

Impacted locations:

• crates/cargo-gamma-lib/src/discover/record.rs:495-504

Reviewed in: #113

---

Title: Align trait-implementation exclusions with the documented visible-outcome contract

[Copilot speaking]

The current exclude-trait-impls path removes every matching mutant from Scanned::mutants before the plan and reports are produced. This conflicts with the user documentation: the README says an ignored verdict includes mutants withdrawn by a config rule, and the design says excluded mutants remain visible.

Location: crates/cargo-gamma-lib/src/discover/survey.rs:596-608

Reproducible reasoning: The scan receives collected mutants, including source-suppressed mutants marked Outcome::Ignored. Lines 593-608 identify configured trait-name matches and retain only nonmatching entries, while subtracting overlaps from the suppression count. Later planning and reporting receive only the retained vector, so no report can contain an ignored verdict for a mutant removed by this configuration rule. That contradicts the current README verdict table and design statement.

Consequence: A user following the documented reporting model may expect to audit which mutation sites a broad trait rule excluded, but those sites are absent from the report. This also makes the documented meaning of the ignored verdict unreliable for configuration-based policy.

Recommended action: Choose and document one contract consistently. Either retain matching mutants with an excluded outcome so configuration exclusions remain visible, or update the README and design to state that exclude-trait-impls filters matching sites out before the campaign population is published.

References:

• crates/cargo-gamma/README.md, Verdicts and Choosing a channel
• crates/cargo-gamma/docs/DESIGN.md, Verdicts and scoring

Impacted locations:

• crates/cargo-gamma-lib/src/discover/survey.rs:593-608

Reviewed in: #113

---

Title: Explain the exclusion and suppression overlap accounting

[Copilot speaking]

The current trait-implementation exclusion block removes every matching mutant, but first counts matching mutants whose outcome is Ignored and subtracts that overlap from suppressed with saturating arithmetic. This ordering and adjustment encode an accounting invariant that the block and Survey::scan documentation do not explain.

Location: crates/cargo-gamma-lib/src/discover/survey.rs:594-608

Reproducible reasoning: Scanned::suppressed counts directive-suppressed mutants that remain in mutants as ignored entries. A configured trait exclusion is applied after the ordinary scan and removes matching entries entirely. Therefore an ignored mutant matching both mechanisms must be removed from the suppression count as well as from the vector, while a live matching mutant changes only the vector. The filter's position before later selection stages and the choice of saturating subtraction are not evident from local documentation.

Consequence: A later refactor could move or simplify the filter while leaving it apparently equivalent, causing the reported suppressed count to include mutants no longer present or masking an unexpected accounting mismatch behind saturation.

Recommended action: Add an inline comment above the exclusion block explaining why trait exclusions are applied at this point, why ignored matches are subtracted from suppressed, and why saturating subtraction is the intended response if the counts disagree.

References:

• crates/cargo-gamma/docs/CONFIG.md

Impacted locations:

• crates/cargo-gamma-lib/src/discover/survey.rs:593-608

Reviewed in: #113

---

Title: Document the deliberate nesting depth in the declaration-only regression

[Copilot speaking]

The new declaration-only regression chooses a fixed nesting depth to force SourceFile::read down its skippable nesting-limit path, but the literal has no adjacent rationale. The immediately preceding selected-file regression uses the same depth and explicitly explains that the value is deliberately independent of the implementation limit.

Location: crates/cargo-gamma-lib/src/discover/survey.rs:3271

Reproducible reasoning: The exact depth is not inherent to the scenario: another value could be chosen, and a maintainer could plausibly replace it with the parser limit or with a smaller fixture. The neighboring test documents why that would change the property being tested: this regression should prove behavior for a file the tool refuses, rather than mirror the current refusal threshold. Because the new test repeats the literal without sharing or repeating that rationale, its design decision cannot be reproduced from the test itself.

Consequence: A future cleanup could couple this test to the current nesting-limit constant or change only one of the two fixtures, weakening the intended guarantee that selected and declaration-only discovery exercise the same independently over-limit input.

Recommended action: Put both tests behind a shared fixture helper or justified test constant, or add the same rationale beside this literal. Keep the comment about why the value is chosen rather than restating the numeric value.

Impacted locations:

• crates/cargo-gamma-lib/src/discover/survey.rs:3270-3271

Reviewed in: #113

---

Title: Use distinct terms for filesystem paths and processing flows

[Copilot speaking]

The added comments and API documentation use “path” for two different concepts in the same discussion. “Path order” and “keyed by path” refer to filesystem paths, while “selected-file path”, “mutating path”, “this one”, and “declaration-only path” refer to alternative processing flows. The wording makes statements such as “which of the two paths read a file” and “move such a file from the mutating path to this one” require interpretation before the reader can identify whether a path value or a control-flow branch is meant.

Location: crates/cargo-gamma-lib/src/discover/survey.rs:1049-1054

Reproducible reasoning: The documentation first establishes the filesystem meaning by saying results are returned in path order. It then immediately switches to using “path” for the selected-file and declaration-only processing branches, without defining a second meaning or using a distinctive term. The same overload appears in the skip-merging comment and the new test documentation. Because these passages specifically explain ordering and movement of files between processing branches, both concepts occur together and the shared term obscures the explanation.

Consequence: A maintainer investigating skip ordering or selection behavior can misread a processing-path statement as a filesystem-path statement and spend additional time reconstructing which scan branch the documentation describes.

Recommended action: Reserve “path” for filesystem paths in these passages and rename the processing concepts consistently, for example to “selected-file scan” and “declaration-only scan” or “processing flow”. Update the nearby comment and test documentation to use the same terms.

Impacted locations:

• crates/cargo-gamma-lib/src/discover/survey.rs:1000-1004
• crates/cargo-gamma-lib/src/discover/survey.rs:1045-1060
• crates/cargo-gamma-lib/src/discover/survey.rs:3258-3265

Reviewed in: #113

---

Title: Include source parsing failures in build's error contract

[Copilot speaking]

The newly added # Errors section says build fails when a mutated file cannot be read back from disk, but the function also propagates errors from SourceFile::parse. A readable file can therefore still make report construction fail if its contents no longer parse as Rust or exceed the parser's nesting limit.

Location: crates/cargo-gamma-lib/src/elements/report.rs:449-451

Reproducible reasoning: The documented failure path is the fs::read_to_string(...)? call at lines 468-469. Immediately afterward, line 471 calls SourceFile::parse(...)?, which independently returns an error for invalid Rust text and for source nested beyond the engine's supported limit. Because the source is read after the mutation run, the original workspace can be edited between discovery and report generation: the file may remain readable while becoming syntactically invalid or too deeply nested. That execution reaches an error case the new API documentation does not disclose.

Consequence: A caller relying on the documented contract may handle file-access failures but be surprised when report generation also aborts for a readable source file, making the API documentation an incomplete account of when the operation fails.

Recommended action: Extend the # Errors section to state that build also fails when a mutated file's current contents cannot be parsed for source-location rendering.

Impacted locations:

• crates/cargo-gamma-lib/src/elements/report.rs:449-451
• crates/cargo-gamma-lib/src/elements/report.rs:468-471

Reviewed in: #113

---

Title: Reject non-finite durations during typed report validation

[Copilot speaking]

The typed validation path treats MutantResult::duration: Option<f64> as sufficient proof that a present duration satisfies the report schema's JSON-number requirement, but f64 also admits NaN and positive or negative infinity. serde_json serializes those non-finite values as JSON null, while validate_file_result does not inspect duration and the vendored Mutation Testing Elements schema requires it to be a number. As a result, to_json and write_json can validate a constructed or transformed typed report successfully and then emit schema-invalid output.

Location: crates/cargo-gamma-lib/src/elements/report.rs:993-1024

Reproducible reasoning: The previous path converted the report to serde_json::Value before calling validate_schema; non-finite durations became Value::Null, which the schema validator rejected because duration must be a number. The replacement validate_report delegates file contents to validate_file_result, which checks mutant identity uniqueness, status vocabulary, and source positions but never checks that a present duration is finite. A caller can therefore use Some(f64::NAN) or an infinity, pass typed validation, and receive JSON containing "duration": null; that output violates the schema and cannot round-trip into the typed MutantResult representation.

Consequence: A caller can receive Ok from cargo-gamma's serialization API while the resulting report is rejected by schema consumers or fails to round-trip through cargo-gamma's merge/read path, undermining the API's validation guarantee.

Recommended action: Extend validate_file_result to reject every present duration for which is_finite() is false, and add regression coverage for NaN and infinity showing that both to_json and write_json fail before returning or publishing output. Alternatively, retain generic-value validation for numeric fields whose Rust types admit values JSON numbers cannot represent.

References:

• Mutation Testing Elements vendored schema: duration must be a number
• Cargo.lock (serde_json 1.0.151)
• serde_json::Number documentation: NaN and infinite values are not JSON numbers

Impacted locations:

• crates/cargo-gamma-lib/src/elements/report.rs:292-295
• crates/cargo-gamma-lib/src/elements/report.rs:960-989
• crates/cargo-gamma-lib/src/elements/report.rs:290-295
• crates/cargo-gamma-lib/src/elements/report.rs:971-1024

Reviewed in: #113

[sandersaares]

[Copilot speaking]

Review findings (continued 3)

Title: Use precise terms for typed and untyped report representations

[Copilot speaking]

The new serialization documentation introduces serde_json::Value and immediately renames it "the tree" without defining that term. The adjacent typed-validation documentation also says that a schema field which must be a number is an f64, although this report model represents numeric schema fields with u32, u64, usize, and f64. These descriptions blur the distinction between the typed Report, the intermediate JSON value, JSON numbers, and the Rust numeric types used to represent them.

Location: crates/cargo-gamma-lib/src/elements/report.rs:942-948

Reproducible reasoning: The write_json documentation first identifies the former intermediate representation precisely as serde_json::Value, then refers to "The tree" and "the types themselves" without establishing which object each phrase denotes. A reader must infer that "tree" means the intermediate JSON value rather than the typed Report. The validate_report documentation then states that a field required to be a number "is an f64", but Thresholds uses u32, RunInfo::started_at uses u64, Position uses usize, and only values such as duration use f64. Reading the relevant type definitions therefore disproves the blanket numeric example. Consistent names and an accurate generalization would let a reader reproduce the allocation and validation argument without translating synonyms or correcting the stated type model.

Consequence: A maintainer extending the schema could misunderstand which representation caused the removed full-document allocation or assume that all numeric schema constraints are represented through floating-point fields, leading to incorrect documentation or validation reasoning for new integer fields.

Recommended action: Refer to the former allocation consistently as the intermediate serde_json::Value or intermediate JSON value. Describe the typed guarantee generally—for example, that schema number fields use Rust numeric types—and name integer or floating-point types only where that distinction is relevant.

Impacted locations:

• crates/cargo-gamma-lib/src/elements/report.rs:942-948
• crates/cargo-gamma-lib/src/elements/report.rs:960-966

Reviewed in: #113

---

Title: Clarify what a zero diagnostic position represents

[Copilot speaking]

The new position_range documentation says that "a zero is a producer that meant no position at all." A numeric value cannot itself be a producer, so the sentence does not clearly identify whether Cargo, rustc, or another JSON producer emitted the sentinel value.

Location: crates/cargo-gamma-lib/src/exec/build/blame.rs:320-321

Reproducible reasoning: The preceding clause establishes that Cargo positions are one-based. The next clause needs to explain the meaning of zero, but its subject changes from the zero value to an unspecified producer through the grammatically invalid phrase "a zero is a producer." Rephrasing it as "zero means the producer supplied no position" preserves the intended distinction from the first character and makes the explanation reproducible without guessing at the missing relationship.

Consequence: Readers may have to reverse engineer the sentinel semantics from the implementation instead of relying on the documentation, and may misread zero as identifying a particular producer rather than representing an absent position.

Recommended action: Rephrase the sentence so the value and producer have distinct roles, for example: "Cargo counts both from one, so zero means the producer supplied no position rather than the first character."

Impacted locations:

• crates/cargo-gamma-lib/src/exec/build/blame.rs:320-321

Reviewed in: #113

---

Title: Correct the sparse-file explanation and test claims

[Copilot speaking]

The new read_bounded documentation and sparse-file test use an inaccurate model of sparse files: metadata reports the file's real logical length, holes are returned as zero-filled logical bytes, and the implementation reads a file rather than a pipe. The test's second rejection also does not distinguish bounded streaming from a metadata-length check, because the sparse file already has an enormous logical length before a sealed record is appended. The implementation is correctly bounded by Read::take, but the explanation and claimed proof do not establish that guarantee accurately.

Location: crates/cargo-gamma-lib/src/exec/census.rs:998-999

Reproducible reasoning: set_len(1 << 34) creates a file with a large logical endpoint while allowing its physical allocation to remain small. metadata().len() reports that logical length, and ordinary reads through holes return zero bytes. Wrapping the file in take(MAX_CENSUS_BYTES + 1) limits the logical bytes that read_to_end can consume and therefore bounds allocation independently of metadata-based classification. However, both sparse-file assertions would also reject under an implementation that checked metadata length first, because appending a record does not reduce the existing logical length. Consequently, terms such as misstate, claimed length, and pipe, together with the assertion that the second read proves streaming enforcement, misdescribe the filesystem behavior and the test's evidentiary value.

Consequence: Maintainers can infer that sparse holes are absent from the byte stream or that sparse-file metadata reports a false size, and may rely on a test that does not actually distinguish the security property it claims to verify. That misunderstanding can lead to incorrect reasoning in later allocation and input-boundary changes.

Recommended action: Rewrite the comments to say that sparse files may have a large logical length while consuming little physical storage, and that Read::take independently bounds bytes consumed and allocation even if the file changes or cannot be safely classified from metadata. Replace pipe with file or reader and use logical-length terminology consistently. Rename or simplify the sparse-file test so it claims only bounded rejection, or add a test mechanism that genuinely observes whether the reader attempts to consume more than the permitted byte count.

References:

• Linux lseek(2): reads from sparse-file holes return null bytes
• Microsoft Sparse File Operations: reads from sparse zero ranges return zero-filled buffers

Impacted locations:

• crates/cargo-gamma-lib/src/exec/census.rs:998-999
• crates/cargo-gamma-lib/src/exec/census.rs:1516-1543

Reviewed in: #113

---

Title: Express census bounds without restating their literal values in comments

[Copilot speaking]

The new bound-related comments translate the adjacent literals back into prose: saturating_add(1) is described as "One byte past the cap", saturating_add(4) as "one record past", and 1 << 34 as "tens of gigabytes". The MAX_CENSUS_BYTES documentation similarly enumerates the two marker records represented by + 2. The review standard requires comments to explain why a value is appropriate without repeating or recomputing that value in prose.

Location: crates/cargo-gamma-lib/src/exec/census.rs:1006-1010

Reproducible reasoning: Each of these comments is attached to a newly introduced literal and restates its quantity in another form. That couples the explanation to the current spelling of the expression: changing the sentinel width, record width, marker set, or sparse-file magnitude can leave a plausible-looking but stale comment. The sparse-file comment also explains only that the value is very large; it does not provide a reproducible reason for choosing this threshold rather than another safely unallocatable size. Semantic derivation and purpose-focused rationale would preserve the useful boundary and trust-boundary explanation without duplicating the value.

Consequence: A maintainer can adjust one side of the code/comment pair and leave the other inconsistent, making it unclear whether the read still probes past the cap and whether the sparse fixture still detects allocation based on claimed length.

Recommended action: Derive record widths and protocol overhead from semantic constants or types where practical, give the sparse probe a named value with a reproducible selection rationale, and rewrite the comments to describe the sentinel-read, protocol-bound, and allocation-detection purposes without restating the encoded quantities.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/census.rs:985-990
• crates/cargo-gamma-lib/src/exec/census.rs:1006-1010
• crates/cargo-gamma-lib/src/exec/census.rs:1498-1509
• crates/cargo-gamma-lib/src/exec/census.rs:1530-1532

Reviewed in: #113

---

Title: Filter relevant census sites before sorting and deduplicating

[Copilot speaking]

The new duplicate defense sorts and deduplicates the complete site vector returned by an untrusted census file before discarding sites that are not selected targets for this binary. The bounded file format still permits more than one million records, while the loop immediately below only consumes records present in wanted.

Location: crates/cargo-gamma-lib/src/exec/census.rs:837-839

Reproducible reasoning: read_bounded permits every protocol-sized record, and decode converts all of them into sites. Lines 837-839 then perform an in-place sort over that entire vector. Only afterward does the iterator filter against wanted. Consequently, an honest census pays an unnecessary sort even though the runtime already serializes its bitmap in ascending ordinal order, and a malformed census can force worst-case sorting work over nearly MAX_CENSUS_SITES records that will mostly be discarded. This coordinator-side work occurs after the sampled process has exited and after sample captures its elapsed duration, so it is neither contained as child work nor represented in the census timing model. The duplicate-saturation invariant only requires deduplicating relevant sites, not sorting irrelevant records.

Consequence: A repository-controlled test can return a protocol-sized, randomly ordered census and make every active census worker sort roughly one million ordinals before the next deadline check, consuming substantial coordinator CPU and allowing the optimization phase to overrun its economic budget.

Recommended action: Retain only ordinals present in wanted before sorting and deduplicating. Alternatively, use the runtime's ascending-record contract to deduplicate in a linear pass and conservatively reject out-of-order malformed input. Either approach preserves the saturation-count defense without sorting records that cannot affect the result.

References:

• crates/cargo-gamma/docs/DESIGN.md, Guard census
• crates/cargo-gamma-rt/src/runtime.rs, write_reached

Impacted locations:

• crates/cargo-gamma-lib/src/exec/census.rs:837-839

Reviewed in: #113

---

Title: Represent bounded census bytes with a distinct type

[Copilot speaking]

The new bounded reader establishes a security-relevant state—its result is small enough for the decoder's length-based preallocation—but returns an ordinary Vec<u8>. The decoder consequently still accepts any byte slice and relies on a documentation-only requirement that callers first apply read_bounded.

Location: crates/cargo-gamma-lib/src/exec/census.rs:1000

Reproducible reasoning: read_bounded reads at most the protocol-derived limit plus one byte and rejects the over-limit case, but its successful return type does not retain that validation. decode takes &[u8] and allocates bytes.len() / 4 entries before examining the records. The newly added documentation explicitly says callers are expected to bound the bytes first, which identifies two distinct states (unchecked bytes and size-validated census bytes) while leaving them interchangeable in the type system. A future production caller can therefore invoke decode directly on unbounded input without a compile-time indication that it bypassed the allocation bound.

Consequence: A later refactor or additional decoder call site can accidentally reintroduce attacker-controlled allocation proportional to an oversized census buffer, despite the coordinator appearing to enforce a protocol-derived limit.

Recommended action: Return a private bounded-census wrapper from read_bounded and make decode accept that wrapper, with a checked constructor available to the small decoder fixtures, or combine bounded reading and decoding behind one function so unchecked byte slices cannot reach the preallocating decoder.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/census.rs:1029-1033

Reviewed in: #113

---

Title: Fully qualify the Read::take intra-doc link

[Copilot speaking]

The new read_bounded documentation links to [Read::take], but Read is not a name in the item's module scope. The only import is use std::io::Read as _; inside the function body, which enables method lookup without binding the name Read where rustdoc resolves the function's documentation.

Location: crates/cargo-gamma-lib/src/exec/census.rs:998

Reproducible reasoning: Intra-doc links are resolved from the documented item's Rust scope. This module imports std::io at line 52, but it does not import std::io::Read by name. The anonymous as _ import begins inside read_bounded after the doc attribute and deliberately introduces no usable identifier. Therefore rustdoc cannot resolve the newly added [Read::take] target; the workspace's rustdoc.broken_intra_doc_links lint reports this as a broken link.

Consequence: API documentation generation emits a broken-intra-doc-link warning, and configurations that promote rustdoc warnings to errors cannot generate the crate documentation. Readers also do not receive the intended link to the standard-library method.

Recommended action: Use an explicit destination such as [Read::take][std::io::Read::take] (and make the same qualification in the duplicated test documentation if it is retained), or bind std::io::Read by name in the module scope so rustdoc can resolve the link.

References:

• Cargo.toml rustdoc lint configuration

Impacted locations:

• crates/cargo-gamma-lib/src/exec/census.rs:998
• crates/cargo-gamma-lib/src/exec/census.rs:1519

Reviewed in: #113

---

Title: Test the census size boundary independently of the production constant

[Copilot speaking]

The new over-limit regression builds its fixture from MAX_CENSUS_BYTES, the same production constant that read_bounded uses for both the Read::take limit and the rejection comparison. The sparse-file regression is only a far-above-limit case. Neither test independently verifies that the documented protocol maximum is still accepted.

Location: crates/cargo-gamma-lib/src/exec/census.rs:1508-1509

Reproducible reasoning: The bound formula is newly introduced as (MAX_CENSUS_SITES + marker records) times the record width. If mutation testing changes that arithmetic to a smaller value, read_bounded becomes too restrictive, but read_bounded_refuses_a_file_past_the_protocol_maximum also shrinks its fixture because it computes the length as MAX_CENSUS_BYTES plus one record. It therefore still observes an error and passes. The fixed large sparse file also remains above the mutated limit and passes. An acceptance assertion using an independently derived contractual size is needed to make a too-small bound observable.

Consequence: A surviving mutation or later arithmetic regression can cause valid high-density census files to be discarded, silently disabling test narrowing for affected binaries and making mutation campaigns run the entire binary for those sites.

Recommended action: Derive the expected protocol boundary independently in the test from gamma_rt::MAX_CENSUS_SITES and the wire-record shape, without referencing MAX_CENSUS_BYTES. Assert that read_bounded accepts a file exactly at that boundary and rejects one byte or record beyond it; keep the sparse-file case as the separate allocation-defense regression.

References:

• crates/cargo-gamma/docs/DESIGN.md, census file size contract (lines 490-495)

Impacted locations:

• crates/cargo-gamma-lib/src/exec/census.rs:990-1014
• crates/cargo-gamma-lib/src/exec/census.rs:1504-1514

Reviewed in: #113

---

Title: Keep reflink capability state scoped to one synchronization operation

[Copilot speaking]

The new CAPABILITIES registry permanently stores one latch for every destination path, although production needs to share a latch only inside one sync_or_copy operation. Workspace::prepare_with_locks selects one cache base with gamma_base, derives one workspace root, and calls sync_or_copy once for that root. sync_or_copy already creates one Reflinks value before choosing delta synchronization or a fresh-copy fallback and passes it through both paths.

Location: crates/cargo-gamma-lib/src/exec/copy.rs:87-89

Reproducible reasoning: The useful behavior is operation-local: after one clone attempt fails, later files and the fallback copy for that same destination should stop attempting reflinks. A fresh latch created at sync_or_copy preserves that behavior because every relevant branch already shares it. The process-global map adds cross-operation state that the CLI does not need, a destination/key pairing precondition in copy_tree_with, mutex-poison policy, permanent path and Arc retention, and stale capability after a path is rebound to another filesystem. The comments’ premise that one run writes both default and redirected roots is contradicted by gamma_base, which chooses one root.

Consequence: Long-lived test or library processes retain every destination key indefinitely, can keep an obsolete unsupported result after filesystem changes, and carry synchronization and API complexity that does not contribute to the production copy operation.

Recommended action: Create one Reflinks latch at the start of sync_or_copy and pass it through the delta and fresh-copy branches, then remove the global registry, destination lookup, poison recovery, destination/capability pairing precondition, and registry-isolation test support. Update the type and test documentation to describe the per-operation guarantee. If reuse across separate operations is retained instead, demonstrate that requirement and bind the destination identity and invalidation policy into one context type rather than storing unbounded path keys forever.

References:

• crates/cargo-gamma-lib/src/exec/sync.rs
• crates/cargo-gamma-lib/src/exec/workspace.rs
• crates/cargo-gamma/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-lib/src/exec/copy.rs:70-100
• crates/cargo-gamma-lib/src/exec/copy.rs:81-84
• crates/cargo-gamma-lib/src/exec/copy.rs:94-102
• crates/cargo-gamma-lib/src/exec/copy.rs:164-166
• crates/cargo-gamma-lib/src/exec/copy.rs:94
• crates/cargo-gamma-lib/src/exec/copy.rs:68-89
• crates/cargo-gamma-lib/src/exec/copy.rs:104-110
• crates/cargo-gamma-lib/src/exec/copy.rs:1082-1125

Reviewed in: #113

---

Title: Rewrite the new comments in idiomatic English

[Copilot speaking]

The newly added containment comment says the fallback is "a process group any descendant leaves with one unprivileged call," which omits the relationship and ability being described. The memory-support comment then says the code will "quote its reason at the user" and refers to "taking prose." These phrases obscure the intended design: a descendant can leave the process group, while the structured platform error is converted to display text so the policy decision can be tested independently of the host.

Location: crates/cargo-gamma-lib/src/exec/measure.rs:488-492

Reproducible reasoning: Reading the first sentence literally does not establish whether the process group leaves, the descendant leaves, or the call itself causes the transition; adding "that" and "can" is necessary to express the intended subject and capability. In the second comment, English normally shows or reports a reason to a user rather than quoting it at a user, and "taking prose" does not name the actual String conversion visible in the code. Because these comments justify platform-containment and error-boundary decisions, readers should be able to recover those decisions without reverse engineering the implementation.

Consequence: A maintainer changing containment or memory-support propagation can misunderstand whether the fallback is escapable and why the error is converted to text at this boundary.

Recommended action: State the mechanics directly, for example that containment falls back to a process group that any descendant can leave with an unprivileged call, and that the platform error is converted to display text here because downstream logic only reports it and accepting text keeps the decision tests host-independent.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/measure.rs:488-492
• crates/cargo-gamma-lib/src/exec/measure.rs:1040-1043

Reviewed in: #113

---

Title: Describe the local enumeration diagnostic as truncated and best-effort

[Copilot speaking]

The new documentation says nextest's raw enumeration output is raised through crate::notes and printed locally, while the implementation forwards only tail(output, 20) and the shared notes queue retains at most 64 diagnostics. The related design text likewise says that output is raised once locally. This promises more diagnostic availability than the implementation provides.

Location: crates/cargo-gamma-lib/src/exec/sweep.rs:110-112

Reproducible reasoning: enumeration_note passes tail(output, 20) to crate::notes::note, so earlier lines are deliberately discarded before the diagnostic is queued. notes.rs then caps the complete command's queue at 64 entries and replaces every later note with only a count. Therefore a long enumeration failure never prints its complete raw output, and a campaign with enough earlier diagnostics may print none of a particular enumeration failure's output. The added wording that the raw text is raised and that the tool prints it does not describe these observable limits.

Consequence: An operator following the documented behavior may expect the local console to contain the enumeration error that explains a mutant, but the relevant line can be outside the retained tail or the entire diagnostic can be dropped after the shared cap is reached.

Recommended action: Update this doc comment and the corresponding report-boundary text in crates/cargo-gamma/docs/DESIGN.md to describe a best-effort, tail-truncated local diagnostic subject to the command's note cap. If showing each complete output once is the intended contract, route it through a dedicated uncapped path instead, while preserving the rule that child output never enters published reports.

References:

• crates/cargo-gamma-lib/src/notes.rs
• crates/cargo-gamma/docs/DESIGN.md, Reports and integrations

Impacted locations:

• crates/cargo-gamma-lib/src/exec/sweep.rs:114-122
• crates/cargo-gamma-lib/src/notes.rs:21-22
• crates/cargo-gamma-lib/src/notes.rs:105-109
• crates/cargo-gamma/docs/DESIGN.md:747-754

Reviewed in: #113

---

Title: Document or centralize the enumeration-output tail limit

[Copilot speaking]

The new transient enumeration diagnostic keeps only the last 20 lines of nextest output, but neither the function rationale nor a shared named constant explains why that boundary is appropriate. The surrounding documentation thoroughly explains why the output is local rather than durable, but it does not make the amount retained independently reproducible.

Location: crates/cargo-gamma-lib/src/exec/sweep.rs:116

Reproducible reasoning: The refreshed enumeration path introduces tail(output, 20) at line 116. The focus-area rule requires a chosen literal to be justified whenever another maintainer could plausibly choose a different value. A diagnostic could reasonably retain fewer lines to reduce noise or more lines to preserve context, and the same numeric limit also appears in another verdict path without any expressed relationship. Therefore the choice and whether the two limits are intentionally synchronized are not evident from context.

Consequence: A later maintainer can change one diagnostic limit without realizing the outputs are intended to follow the same policy, or preserve an unsuitable value because there is no recorded trade-off between useful nextest context and bounded console output.

Recommended action: Replace the literal with a shared, descriptively named line-limit constant whose comment records how the limit was selected, or add an adjacent comment explaining why this diagnostic retains that amount and whether it deliberately matches the existing verdict-output tail.

References:

• crates/cargo-gamma/docs/DESIGN.md, published-reason and local enumeration-output policy (lines 747-754)

Impacted locations:

• crates/cargo-gamma-lib/src/exec/sweep.rs:116

Reviewed in: #113

---

Title: Keep stale hinted results from exhausting file-local learning

[Copilot speaking]

The newly unconditional observed.publish(&judged) in the per-mutant-hint branch lets a result with no killer transition the file's shared learning state from Untried or InProgress to Exhausted. A stale persisted hint failing for one mutant does not establish that another mutant in the same file cannot reveal a useful file-local killer.

Location: crates/cargo-gamma-lib/src/exec/sweep.rs:748

Reproducible reasoning: Start with a file whose FileLearning state is Untried. A worker for a mutant with a persisted hint takes the hint.is_some() branch. If that hint is stale and the ensuing ordinary run produces no killer, line 748 now calls publish; the None arm at line 855 writes Exhausted. Every later unhinted sibling then takes the Learning::Exhausted path and runs without attempting file-local learning. The same call can also replace InProgress with Exhausted and wake waiting siblings while the actual learner is still running. Before this change, the hinted branch published only a discovered killer and otherwise left the learning state alone, while only the worker that claimed Untried completed a no-killer learning attempt as Exhausted.

Consequence: One stale per-mutant cache entry can disable killer learning for the rest of a source file, causing every remaining mutant to launch the full cold test order instead of probing a killer learned from a sibling. On files with many mutants or expensive test binaries, this can materially lengthen the sweep.

Recommended action: Distinguish publishing a discovered killer from completing the designated learning attempt. Allow any worker to publish Some(killer), but allow None to produce Exhausted only for the worker that changed Untried to InProgress; a no-killer result from the persisted-hint branch should leave Untried/InProgress unchanged. Add a regression covering a stale hinted mutant followed by an unhinted sibling that discovers a killer, and preferably the concurrent InProgress case.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/sweep.rs:748
• crates/cargo-gamma-lib/src/exec/sweep.rs:853-856

Reviewed in: #113

---

Title: Select the diagnostic tail without collecting every output line

[Copilot speaking]

The new enumeration-failure diagnostic calls tail(output, 20). That helper first collects every line in the complete output into a Vec<&str> and then allocates another String containing the selected suffix, even though this call retains only a bounded tail and immediately formats it into a second owned string.

Location: crates/cargo-gamma-lib/src/exec/sweep.rs:116

Reproducible reasoning: The refreshed call at line 116 receives the complete captured nextest output. exec/verdict.rs::tail executes text.lines().collect() before calculating the start of the last requested lines, so its temporary allocation grows with the number of lines in the full output rather than with the twenty lines retained. join then allocates the tail string, and the surrounding format! copies that tail into the diagnostic string. A reverse newline scan can locate the start of the retained suffix without a line vector, after which formatting can copy directly from a borrowed slice.

Consequence: A verbose or adversarial enumeration failure can create a large temporary line-index allocation while the full captured output is already resident, increasing peak memory and latency on an error path intended to bound retained diagnostic output.

Recommended action: Use a borrowing tail operation that locates the suffix boundary without collecting all lines, or write the selected suffix directly into the diagnostic string. Preserve the existing last-lines behavior while making temporary storage independent of the total output line count.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/sweep.rs:114-122
• crates/cargo-gamma-lib/src/exec/verdict.rs:1367-1372

Reviewed in: #113

---

Title: Use one term for the persisted report reason

[Copilot speaking]

The new enumeration-output comments call the string returned by enumeration_note a "score-bearing note", report.rs's "serialized reason", and a "durable note". These phrases are not defined as separate concepts, and in this path they describe the same value: the optional mutant note that is projected into a report status reason. The transient crate::notes message is the genuinely separate channel.

Location: crates/cargo-gamma-lib/src/exec/sweep.rs:105-112

Reproducible reasoning: enumeration_note returns one String. judge_ordered places that string in Judgement::Reached's optional note, the sweep stores it as the mutant note, and report projection turns that note into the serialized status reason. Because the comments use several near-synonyms for this single persisted value without defining distinctions, a reader cannot tell whether they name separate retention boundaries or merely different stages of one value. The terminology guideline requires specialized terms to be defined and similar concepts to use clear, consistent names.

Consequence: A maintainer changing this security-sensitive path could mistake the durable note and serialized reason for different outputs, then filter or test only one name while raw test output still reaches a published report through the other stage.

Recommended action: Define the returned value once using the report model's terminology, such as the mutant note that becomes the report statusReason, and use that term consistently in the function documentation, regression-test documentation, and assertion message. Reserve local diagnostic for the separate crate::notes output.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/sweep.rs:105-112
• crates/cargo-gamma-lib/src/exec/sweep.rs:1524-1537

Reviewed in: #113

---

Title: Preserve the authoritative-stream contract through final verdict parsing

[Copilot speaking]

The new reader documentation says only the harness's authoritative stream may settle a verdict, and the new test is named as though diagnostic output cannot forge a libtest failure. That guarantee currently applies only to the live Progress watcher. After the child exits, collected concatenates stdout and stderr without retaining which stream produced each chunk, and settle searches that combined text for the first libtest or nextest failure line.

Location: crates/cargo-gamma-lib/src/exec/verdict.rs:1226-1227

Reproducible reasoning: readers marks direct-run stdout and nextest stderr as authoritative and routes the other stream through heard_diagnostic, which prevents diagnostic lines from creating a live failure. However, each reader sends only (Vec<u8>, bool); collected appends both chunks into one buffer; and settle passes that buffer to first_failure or nextest::first_failure. The existing collected documentation even states that a failure announcement may be on either stream. Therefore the newly documented authoritative-stream property is not enforced by the final classification path, and the unit test at lines 1504-1513 verifies only the streaming watcher rather than the end-to-end verdict.

Consequence: A directly run test binary that exits non-zero can print a libtest-shaped line on stderr and have that diagnostic text reported as the killing test; the equivalent can happen from nextest's non-authoritative stream. The campaign can then publish and learn an incorrect killer name even though the added documentation says that stream is not verdict evidence.

Recommended action: Keep stream identity in the reader result and parse failure names only from the authoritative stream during settle, while continuing to scan both streams for the runtime environment marker and activity. Add an end-to-end assertion covering a non-zero run whose diagnostic stream contains a forged failure line. If combining both streams is intentional, narrow the new comment and test name to say they protect only live early-termination state rather than claiming an end-to-end verdict boundary.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/verdict.rs:589-607
• crates/cargo-gamma-lib/src/exec/verdict.rs:919-932
• crates/cargo-gamma-lib/src/exec/verdict.rs:1504-1513

Reviewed in: #113

---

Title: Use direct technical language in the new rationale comments

[Copilot speaking]

The new comments use conversational phrasing where concise technical statements would be clearer. Only::names describes an “allocation with no reason behind it” and a value that “has nowhere” to be borrowed from, while confirm_enumeration begins “Whatever the exoneration ran into, it ran into it.”

Location: crates/cargo-gamma-lib/src/exec/verdict.rs:71-78

Reproducible reasoning: The professional-English guidance calls for professional wording and avoidance of colloquialisms. These phrases personify values and repeat idiomatic language instead of stating the relevant facts: borrowing These avoids an unnecessary vector allocation, One must own a one-element collection because the enum stores only the string reference, and an exonerating run has no active mutant. Those direct statements preserve the rationale without requiring readers to interpret conversational expressions.

Consequence: The allocation and verdict-classification rationale is harder to scan and translate into the actual ownership and evidence rules that future changes must preserve.

Recommended action: Rewrite these comments as direct technical statements, for example by calling the copied vector unnecessary, explaining the lifetime constraint on One, and stating that timeout or memory-limit evidence from a run with no active mutant cannot be attributed to the mutant.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/verdict.rs:71-78
• crates/cargo-gamma-lib/src/exec/verdict.rs:496-500

Reviewed in: #113

---

Title: Reference the cache identity width instead of restating it

[Copilot speaking]

The workspace_identity documentation says that the digest is truncated to 64 bits, while the implementation independently expresses the same width through an eight-byte array and slice. The repository's cache design document already owns the exact external cache-identity contract.

Location: crates/cargo-gamma-lib/src/exec/workspace.rs:732-740

Reproducible reasoning: The comment is attached directly to the truncation logic, so its numeric statement is a second representation of the implementation value. If the implementation width changes, Rust cannot verify that the prose changes with it. The maintainability rule for literal values requires the comment to justify the choice without repeating the value, and the existing design document provides a stable place to state the contractual width.

Consequence: A later cache-identity change can leave the source comment describing a different width from the code, misleading maintainers about cache-name compatibility and the collision assumptions behind the ownership marker.

Recommended action: Replace the numeric restatement with a reference to the cache identity section of crates/cargo-gamma/docs/DESIGN.md and keep the rationale that the shortened digest is readable and collision-checked, without repeating its width in this code comment.

References:

• crates/cargo-gamma/docs/DESIGN.md, scratch workspace and cache identity

Impacted locations:

• crates/cargo-gamma-lib/src/exec/workspace.rs:732-740

Reviewed in: #113

---

Title: Document why both Unix owner-marker open flags are required

[Copilot speaking]

The Unix open_cache_owner path adds O_NOFOLLOW | O_NONBLOCK without explaining the distinct invariant each flag protects. The surrounding documentation explains same-handle inspection, but it does not tell a maintainer why nonblocking mode is necessary before the regular-file check.

Location: crates/cargo-gamma-lib/src/exec/workspace.rs:1279-1281

Reproducible reasoning: O_NOFOLLOW prevents the final marker component from being followed when it is opened. O_NONBLOCK serves a different purpose: an attacker-controlled special file such as a FIFO must not block the open before metadata().is_file() can reject it. Neither role is obvious from the combined expression, and the higher-level design only states that the marker is read through the inspected handle. A maintainer cannot independently reproduce the need for both flags from the current comments.

Consequence: A future cleanup can remove O_NONBLOCK as apparently redundant with O_NOFOLLOW, allowing validation of a planted FIFO to wait indefinitely before the code reaches its regular-file refusal.

Recommended action: Add a short comment beside custom_flags explaining that O_NOFOLLOW rejects a symlink at open time and O_NONBLOCK ensures a non-regular marker cannot block before handle metadata rejects it.

References:

• crates/cargo-gamma/docs/DESIGN.md, redirected cache ownership-marker handling

Impacted locations:

• crates/cargo-gamma-lib/src/exec/workspace.rs:1275-1282

Reviewed in: #113

---

Title: Bound owner-marker reads before allocating

[Copilot speaking]

Default-cache ownership validation now runs for ordinary workspace claims and reads the entire .cargo-gamma-owner file into a String. The marker is filesystem state rather than a trusted in-memory value, but its size is not checked before allocation; on a mismatch, encode_controls can then allocate another representation of the same arbitrarily large contents.

Location: crates/cargo-gamma-lib/src/exec/workspace.rs:1231-1235

Reproducible reasoning: The pull request adds validate_cache_owner(root, &base, CacheKind::Default) to claim_workspace, so this path now executes for every command that claims the normal per-workspace cache, not only for an explicitly redirected cache. The opened marker's metadata is already available, but only its file type is checked. The code then calls read_to_string without a take limit or a comparison between metadata.len() and the expected physical workspace path length. Therefore allocation is controlled by the marker file size rather than by the small ownership record the format requires.

Consequence: A corrupted or planted owner marker can make an otherwise routine cargo gamma command allocate memory proportional to the file size and potentially terminate from memory exhaustion before reporting that the cache does not belong to the workspace.

Recommended action: Derive the expected physical workspace path before reading, reject marker lengths that cannot equal that path, and compare through a bounded buffer or bounded reader. Keep mismatch diagnostics bounded as well—for example, report an invalid ownership marker when its length is impossible, and only encode/display contents after they have been limited to a legitimate path-sized value.

References:

• crates/cargo-gamma/docs/DESIGN.md, cache identity and ownership marker

Impacted locations:

• crates/cargo-gamma-lib/src/exec/workspace.rs:1390

Reviewed in: #113

---

Title: Use one term for the cache owner marker

[Copilot speaking]

The .cargo-gamma-owner file is called the "owner marker" or "cache owner marker" throughout this module, but claim_workspace calls the same file the "identity marker". The nearby workspace_identity function names the digest used for the cache directory, so "identity marker" can also appear to refer to that separate value.

Location: crates/cargo-gamma-lib/src/exec/workspace.rs:1378

Reproducible reasoning: CACHE_OWNER identifies the single marker file. Its documentation and diagnostics consistently describe it as the owner marker, while the claim_workspace documentation switches to "identity marker" without introducing a distinct concept. workspace_identity already names the directory-name digest, so using identity for both concepts makes the ownership flow harder to follow.

Consequence: A maintainer tracing cache collisions can reasonably infer that the directory identity digest and the owner marker are two forms of one identity mechanism, or that a second identity marker exists, and misread which value is verified under the lock.

Recommended action: Replace "identity marker" at claim_workspace with "cache owner marker" (or "owner marker") and retain that term consistently for .cargo-gamma-owner.

Impacted locations:

• crates/cargo-gamma-lib/src/exec/workspace.rs:733
• crates/cargo-gamma-lib/src/exec/workspace.rs:1378-1381

Reviewed in: #113

---

Title: Document why the vendored viewer version excludes SemVer suffixes

[Copilot speaking]

The new provenance test accepts only ASCII digits and periods in VIEWER_VERSION. The surrounding documentation explains why a version marker exists and why it is read from the vendored file, but it does not explain the separate decision to reject other version syntax such as pre-release or build suffixes.

Location: crates/cargo-gamma-lib/src/html.rs:228-233

Reproducible reasoning: At lines 228-233 the predicate is is_ascii_digit() || character == '.', so the literal period and the omitted alternatives define a format policy rather than merely checking that the marker is present. Another maintainer could plausibly accept - or +, or parse a complete semantic version, and the current comments provide no source or policy from which to reproduce the digits-and-periods-only choice. The test name and failure message describe the selected shape as a bare version number, but describe what is required rather than why that narrower shape is correct for the vendoring process.

Consequence: When the viewer is refreshed from a pre-release or build-tagged upstream version, the test will fail without telling the maintainer whether such inputs are intentionally forbidden, whether the marker should discard the suffix, or whether the validation should be broadened.

Recommended action: Add a comment that identifies the vendoring policy or source format that guarantees a stable release-core version, or validate against the documented version syntax actually accepted by the vendoring workflow. A short policy comment is sufficient if suffixes are deliberately unsupported; broader parsing is only warranted if the workflow is intended to accept them.

References:

• crates/cargo-gamma-lib/src/vendor/VERSION

Impacted locations:

• crates/cargo-gamma-lib/src/html.rs:228-233

Reviewed in: #113

---

Title: Deserialize incoming files directly into the ordered map

[Copilot speaking]

The incoming report still decodes files into the crate's FxHashMap, then converts that map into Report.files with into_iter().collect(). Every merge input therefore allocates and populates two map containers even though only the ordered map survives.

Location: crates/cargo-gamma-lib/src/merge/incoming.rs

Reproducible reasoning: Deserializing Incoming allocates the hash table and inserts every file entry. The conversion at line 89 then allocates B-tree nodes and inserts every entry again; moving the keys and values does not release the hash table's bucket allocation until collection finishes. The merge reader explicitly permits reports up to 256 MiB and documents peak-memory control as a design concern, so the extra container and second indexing pass are material for large reports. Declaring Incoming.files as BTreeMap<String, FileResult> lets Serde populate the final representation directly while preserving the deterministic ordering required by Report.

Consequence: Merging a report with many mutated files consumes avoidable peak memory and CPU, and a directory merge can run out of memory or become substantially slower while retained reports are already occupying the aggregate budget.

Recommended action: Change Incoming.files to BTreeMap<String, FileResult> and move it directly into Report (files: incoming.files), removing the intermediate FxHashMap allocation and conversion.

References:

• crates/cargo-gamma-lib/src/elements/report.rs: ordered Report.files contract
• crates/cargo-gamma-lib/src/merge/read.rs: report-size and peak-memory bounds

Impacted locations:

• crates/cargo-gamma-lib/src/merge/incoming.rs:49
• crates/cargo-gamma-lib/src/merge/read.rs:18-26

Reviewed in: #113

---

Title: Keep implementation modules private behind one internals facade

[Copilot speaking]

The internals feature changes each implementation module itself from private to pub and then re-exports that module under internals. Enabling the feature therefore exposes duplicate paths such as cargo_gamma_lib::commands and cargo_gamma_lib::internals::commands, despite the documentation describing a single integration-test facade. A repository-wide test banning every public glob re-export is the constraint that led to the duplicated module declarations and root visibility.

Location: crates/cargo-gamma-lib/src/lib.rs:125-139

Reproducible reasoning: #[doc(hidden)] affects rustdoc navigation, not Rust visibility; any dependent crate can enable a Cargo feature and name a root pub mod. pub use crate::$name cannot re-export a private module object, so the current facade widens the root modules first. An inline facade wrapper can instead keep mod commands; private and expose its public test items under pub mod internals::commands { pub use crate::commands::*; }. In that explicitly unsupported test facade, automatic mirroring is the intended behavior, so the blanket no-glob policy creates more API exposure and synchronization work than it prevents.

Consequence: Integration tests and external feature users can couple to undocumented root-module paths, and every module addition or cfg change must keep duplicate declarations, the facade list, and the policy test synchronized.

Recommended action: Narrow the no-glob check to supported public facades, then restore one private declaration for each implementation module and expose test-facing items only through inline wrappers under internals. A glob inside that hidden, explicitly unsupported facade is the simplest way to mirror the module; explicit item lists are an alternative only if their maintenance cost is justified. Keep file-bearing mod declarations outside macros so source-discovery tools can still resolve them.

References:

• crates/cargo-gamma-lib/docs/design/README.md, Boundaries

Impacted locations:

• crates/cargo-gamma-lib/src/lib.rs:125-300
• crates/cargo-gamma-lib/tests/docs.rs:683-714
• crates/cargo-gamma-lib/src/lib.rs:137-246
• crates/cargo-gamma-lib/src/lib.rs:125-294
• crates/cargo-gamma-lib/src/ops/registry.rs:4-11

Reviewed in: #113

---

Title: Include cfg(test) in the internals visibility gates

[Copilot speaking]

The new paired module declarations use cfg(feature = "internals") and cfg(not(feature = "internals")), and the generated internals facade remains feature-only. The test-build feature rule requires feature-dependent code to use cfg(any(test, feature = "internals")), with the inverse branch using the exact negation, so a unit-test build exercises the feature-specific surface.

Location: crates/cargo-gamma-lib/src/lib.rs:135-138

Reproducible reasoning: In a normal library unit-test build, cfg(test) is true but the internals Cargo feature may be false. Under the added gates at lines 135-247, rustc therefore selects only the private module declarations. The facade emitted by expose_internals! is also removed by the feature-only gate at line 285. Consequently that test build does not type-check the newly introduced public module visibility or the facade re-exports. The crate already uses the required pattern for testing at line 306, and its optional tempfile dependency is also present as a dev-dependency, so extending the same test-inclusive condition is supported by the manifest.

Consequence: A visibility or re-export regression in this new internals API surface can pass an ordinary unit-test build because the affected declarations are compiled only when an explicit all-features/internals build happens.

Recommended action: Gate the public module declarations and the internals facade with cfg(any(test, feature = "internals")), and gate the private declarations with cfg(not(any(test, feature = "internals"))). Keep the integration-test feature requirement for separate test crates, where cfg(test) is not set on the library dependency.

References:

• crates/cargo-gamma-lib/Cargo.toml
• Rust Reference: conditional compilation

Impacted locations:

• crates/cargo-gamma-lib/src/lib.rs:135-247
• crates/cargo-gamma-lib/src/lib.rs:285

Reviewed in: #113

---

Title: Keep trait-exclusion metadata out of the long-lived mutant population

[Copilot speaking]

Mutant now carries trait_impl: Option<Arc<str>> even though the field is documented as discovery-only. The exclusion pass consumes this metadata immediately after scanning, but every surviving mutant retains the field through diff filtering, sharding, execution, diagnostics, and reporting. Interner::share even performs another lookup and reassignment for the field after the exclusion pass has already made its only production use of it.

Location: crates/cargo-gamma-lib/src/model/mutant.rs:82-88

Reproducible reasoning: The campaign stores mutants in one contiguous population and the surrounding model explicitly expects hundreds of thousands of entries. Adding an Option<Arc<str>> therefore adds pointer metadata to every entry, including mutants outside trait implementations and every run with no configured trait exclusions. For mutants inside trait implementations, the value also incurs Arc cloning/refcount work while candidates become definitions and definitions become coordinator mutants. Survey::scan now reads Mutant::trait_impl directly in the retention closure that applies configured exclusions before diff and shard filtering, while the added Interner::share branch runs later. The metadata can consequently remain in the short-lived candidate/definition layer, be used to apply exclusions there, and be discarded before constructing the long-lived campaign model.

Consequence: Large workspaces pay a permanent resident-memory and cache-footprint cost for discovery metadata that is no longer actionable, and the post-selection interning branch adds hash-table and reference-count work without benefiting any later operation.

Recommended action: Apply configured trait exclusions while the candidate or definition metadata is still available, then construct Mutant without a trait_impl field. Remove the corresponding post-selection interning branch. If restructuring the filter boundary is deferred, at minimum avoid interning the field after its last read, but removing it from the long-lived type is what eliminates the per-mutant footprint.

Impacted locations:

• crates/cargo-gamma-lib/src/model/mutant.rs:82-88
• crates/cargo-gamma-lib/src/model/interner.rs:33-35

Reviewed in: #113

---

Title: Document summary failures from both host streams

[Copilot speaking]

The new summarize error documentation says the function fails only when the host's output stream rejects a write and characterizes that failure through conversational closed-pipe wording, but summarize also propagates failures while writing skipped-file diagnostics to host.error().

Location: crates/cargo-gamma-lib/src/report/summary.rs:126-129

Reproducible reasoning: summarize evaluates skipped(host, plan, styler)? before acquiring the results stream. When skipped files exist, skipped uses fallible writes to host.error() and propagates their errors. The subsequent summary writes likewise propagate errors from host.output(). The contractual condition is therefore a rejected write to either stream; a downstream reader closing a pipe is only one possible cause and should not characterize every failure.

Consequence: Callers using distinct output and diagnostic sinks may incorrectly assume that summarize cannot fail during skipped-file reporting or that every returned error means the results pipe was closed, leaving the public error contract misleading.

Recommended action: Rewrite the # Errors section in formal API language to state that summarize returns an error when writing either skipped-file diagnostics or summary results fails. If retained, present a downstream reader closing a pipe only as an example rather than as the general failure condition.

Impacted locations:

• crates/cargo-gamma-lib/src/report/summary.rs:102-118
• crates/cargo-gamma-lib/src/report/summary.rs:130-131

Reviewed in: #113

---

Title: Document the single-multiplier rule in the primary directive guide

[Copilot speaking]

The new state_multiplier path changes directive parsing from silently retaining one value to returning a usage error whenever an argument list states a second timeout multiplier. The attribute macro documentation describes this rule, but the primary cargo-gamma README sections that define directive arguments and the comment form only say that test_timeout_multiplier overrides the default and that comments behave like attributes; they do not state that positional and named spellings collectively permit exactly one multiplier.

Location: crates/cargo-gamma-lib/src/suppress/directive.rs:252-258

Reproducible reasoning: Users can write comment directives without relying on compiler validation, so cargo gamma is the first component that diagnoses this newly rejected syntax. The README is the tool's user-facing directive contract and shows the accepted generic, namespaced, and comment forms. Because this change turns previously accepted duplicate lists into hard usage errors, documenting the uniqueness rule only in the separate attribute crate's rustdoc leaves the comment-channel contract incomplete at the place users are directed to learn it.

Consequence: A user may reasonably read #[gamma::test_timeout_multiplier(2.0, factor = 3.0)] or the equivalent comment as an ordered override, then have the campaign fail with a rule the tool's primary directive documentation never disclosed.

Recommended action: Update the cargo-gamma directive-arguments documentation to state that a positional multiplier may appear anywhere in the list, but exactly one multiplier may be stated across positional and named aliases; a second multiplier is a usage error. Include the comment form in that contract rather than relying on the separate attribute-macro documentation.

References:

• crates/cargo-gamma/README.md, Directive arguments and The comment form
• crates/cargo-gamma-attrs/src/lib.rs, test_timeout_multiplier
• crates/cargo-gamma-lib/docs/design/README.md, Public contract

Impacted locations:

• crates/cargo-gamma/README.md:884-903
• crates/cargo-gamma/README.md:923-944

Reviewed in: #113

---

Title: Use the established named-argument terminology for multipliers

[Copilot speaking]

The new state_multiplier documentation calls assignment-style multiplier arguments “keyed”, although this module consistently calls the same name = value syntax “named values”, a “named argument”, and the “named form”. No distinct keyed concept is defined.

Location: crates/cargo-gamma-lib/src/suppress/directive.rs:246-247

Reproducible reasoning: The terminology guideline asks reviewers to avoid introducing new terms when existing terms are available and to document differences when similar terms distinguish concepts. Here, lines 123, 141, and 216 establish “named” for this syntax, while line 246 introduces “keyed” only once and uses it as a synonym rather than a distinct concept. Replacing “keyed” with “named” preserves the existing vocabulary and makes the positional-versus-named contrast match the surrounding implementation and tests.

Consequence: A reader can reasonably wonder whether a keyed multiplier differs from a named multiplier, or must search the parser to infer that the terms describe the same syntax.

Recommended action: Replace “keyed” with “named” in this documentation so the module uses one term for name = value arguments.

Impacted locations:

• crates/cargo-gamma-lib/src/suppress/directive.rs:246-247

Reviewed in: #113

[sandersaares]

[Copilot speaking]

Review findings (continued 4)

Title: Document the pause-registry alias

[Copilot speaking]

The new Pauses alias compresses a nested OnceLock<Mutex<HashMap<...>>> into an otherwise unexplained name. Its declaration does not state that each instance is a lazily initialized registry keyed by workspace root and stores the two channels used for a one-shot command-boundary handshake.

Location: crates/cargo-gamma-lib/src/testing.rs

Reproducible reasoning: The alias is introduced so both cache-adoption and workspace-preparation pause registries share one complex synchronization shape. Reading Pauses alone does not reveal its ownership scope, key meaning, or why the channel pair is stored behind OnceLock and Mutex; a maintainer must inspect the statics, hold_at, and pause_at together to reconstruct that role. The maintainability guidance requires a type-level comment for nontrivial types, and this alias represents concurrency state rather than a self-evident scalar.

Consequence: A future maintainer extending the pause mechanism can misread Pauses as a collection of active pause handles rather than the per-boundary registration registry, making synchronization changes harder to review and increasing the chance of combining registries that must remain independent.

Recommended action: Add a type-level documentation comment above Pauses explaining that it is a lazily initialized, mutex-protected map from workspace roots to the reached/release channel pair for one command boundary, and that separate instances isolate independently controlled boundaries.

Impacted locations:

• crates/cargo-gamma-lib/src/testing.rs:40

Reviewed in: #113

---

Title: Use production-shaped mutant IDs in the SARIF golden

[Copilot speaking]

The new SARIF golden labels each fingerprint as gammaMutantId/v5, but the values are absolute path/line/mutator strings such as /w/src/a.rs:7:relational.gt_to_ge. Cargo Gamma documents version 5 mutant IDs as content-addressed twelve-character hexadecimal digests and uses those IDs so SARIF findings survive reformatting. The committed contract fixture therefore shows a different fingerprint value from the production contract it is intended to pin.

Location: crates/cargo-gamma-lib/tests/fixtures/sarif.golden.json:61-62

Reproducible reasoning: The golden stores /w/src/a.rs:7:relational.gt_to_ge under gammaMutantId/v5. sarif copies mutant.id directly into partialFingerprints, while the related test fixture constructs that ID from file, line, and mutator. Production discovery instead computes MutantId from normalized semantic context and renders a twelve-character digest, explicitly excluding line and column. Consequently, the golden comparison verifies only that an arbitrary fixture string is passed through; it does not verify the documented version-5 fingerprint representation and gives the versioned key semantics that its value does not satisfy.

Consequence: A regression that feeds line-based coordinates into SARIF can retain the same golden shape even though GitHub dismissals would be detached by formatting or inserted lines, and the checked-in example itself suggests an unstable fingerprint format to maintainers.

Recommended action: Construct the SARIF contract fixture with representative version-5 content-addressed mutant IDs, preferably by using the production identity builder, and update all three golden fingerprint values. Add a focused assertion that the emitted fingerprint values correspond to production-shaped IDs while retaining the existing whole-document comparison.

References:

• crates/cargo-gamma/docs/DESIGN.md, "Identity and knowledge across campaigns"
• crates/cargo-gamma-engine/src/model/identity.rs
• crates/cargo-gamma-lib/tests/sarif_contract.rs

Impacted locations:

• crates/cargo-gamma-lib/tests/fixtures/sarif.golden.json:61-62
• crates/cargo-gamma-lib/tests/fixtures/sarif.golden.json:84-85
• crates/cargo-gamma-lib/tests/fixtures/sarif.golden.json:107-108

Reviewed in: #113

---

Title: Document the cache-owner fixture contract

[Copilot speaking]

The new mark_cache_owner fixture reproduces two private production details—the .cargo-gamma-owner file name and the canonical workspace path stored in it—but has no comment identifying those values as a synchronized cache-format contract. The helper name explains what it does, not why canonicalization and this exact marker name are required.

Location: crates/cargo-gamma-lib/tests/cli.rs:45-49

Reproducible reasoning: validate_cache_owner in src/exec/workspace.rs reads a marker named by CACHE_OWNER and compares its contents with physical(source). The test helper independently canonicalizes the temporary workspace and hard-codes the same marker name so clean and hint promotion accept the seeded cache. Both choices therefore reflect production design decisions rather than arbitrary fixture setup, and a maintainer needs a pointer to the production contract to reproduce or update them correctly.

Consequence: A later cache-format or path-normalization change can leave this fixture looking like incidental filesystem setup; a maintainer may update only the failing literal or remove canonicalization without understanding that both must continue to match production's ownership check.

Recommended action: Add a short comment or documentation reference above mark_cache_owner that names the production owner-marker contract and explains why the stored path is canonicalized. If the production API can expose a test-oriented helper or constant without widening the public contract, prefer using that instead of duplicating the marker name.

References:

• crates/cargo-gamma-lib/src/exec/workspace.rs

Impacted locations:

• crates/cargo-gamma-lib/tests/cli.rs:45-50

Reviewed in: #113

---

Title: Document the grammar assumptions behind the source scanners

[Copilot speaking]

The new policy tests classify functions, module declarations, and re-exports with line-oriented string parsing, but their comments describe only the desired result. They do not explain which Rust formatting and declaration forms the parsers intentionally support, why that subset is sufficient for this repository, or how those assumptions should be kept valid.

Location: crates/cargo-gamma-lib/tests/docs.rs:564-581

Reproducible reasoning: function_name recognizes a fixed sequence of visibility and function qualifiers and leaves the previous enclosing function in effect when a declaration is not recognized. declared_modules recognizes a fixed set of one-line module prefixes, and reexported_root recognizes only lines beginning with pub use . These are deliberate lightweight-parser decisions that determine whether environment-write exemptions and rustdoc policy violations are detected. Unlike the existing fields helper in the same file, which explicitly calls its parsing crude and explains why it is acceptable, the new scanners provide no rationale or stated boundary from which a maintainer can verify the approach.

Consequence: A future maintainer can introduce or refactor a valid declaration into an unrecognized form and receive a silently incomplete policy check, while the comments still imply that every relevant function or re-export is covered.

Recommended action: Document the accepted source-shape assumptions and the repository convention or validation that makes line parsing sufficient. If those assumptions cannot be bounded reliably, parse the Rust syntax structurally instead so the comments can state a complete detection contract.

Impacted locations:

• crates/cargo-gamma-lib/tests/docs.rs:564-581
• crates/cargo-gamma-lib/tests/docs.rs:640-667
• crates/cargo-gamma-lib/tests/docs.rs:672-681

Reviewed in: #113

---

Title: Move the corpus submodule below the file-level test members

[Copilot speaking]

The newly added corpus submodule is placed immediately after the first test, while the file then resumes with file-level helper functions, a constant, and the remaining tests. The assigned layout guideline requires submodules and namespaces to appear below direct members.

Location: crates/cargo-gamma-lib/tests/agreement.rs:76

Reproducible reasoning: the_proc_macro_limits_match_the_library is a direct file-level member ending before line 76. Lines 76-140 define the nested corpus namespace, but lines 156 onward return to direct file-level helpers and tests through the end of the file. Therefore the submodule interrupts, rather than follows, the direct members. Rust permits referring to a module declared later, so moving this block does not require changing the test logic.

Consequence: Readers must cross a large nested namespace before reaching the file's primary helpers and tests, and future direct members are likely to continue accumulating on both sides of the module instead of following one predictable layout.

Recommended action: Move the corpus module, including its documentation, to the end of agreement.rs, after all direct file-level constants, helpers, and tests.

Impacted locations:

• crates/cargo-gamma-lib/tests/agreement.rs:76-140
• crates/cargo-gamma-lib/tests/agreement.rs:156-416

Reviewed in: #113

---

Title: Use precise wording for scanner outcomes

[Copilot speaking]

The new postfix-contract explanation says the engine "waves through" text and that "half one fires" when an ordering condition is violated.

Location: crates/cargo-gamma-lib/tests/agreement.rs:190-197

Reproducible reasoning: The assigned English guideline requires professional language and avoidance of colloquialisms. "Waves through" is a metaphor for accepting input, while "half one fires" is an informal and indirect reference to the first assertion. The surrounding text already uses the precise terms "accepts", "rejects", and "asserts", so those established terms can express the same logic unambiguously.

Consequence: The metaphorical wording makes a subtle directional invariant harder to understand, particularly for readers who do not recognize the idioms.

Recommended action: Replace the phrases with direct wording such as "text the engine accepts" and "the first assertion fails".

Impacted locations:

• crates/cargo-gamma-lib/tests/agreement.rs:190-197

Reviewed in: #113

---

Title: Describe only the SARIF fields that require serde renames

[Copilot speaking]

The new module-level rationale says every field name in the SARIF document is produced by a hand-written serde rename from a different Rust identifier. Most fields, including version, runs, tool, driver, name, rules, results, level, message, locations, text, uri, and region, serialize directly from identically named Rust fields; only camel-cased or otherwise special wire names use rename attributes.

Location: crates/cargo-gamma-lib/tests/sarif_contract.rs:8-10

Reproducible reasoning: Reading the new comment at lines 8-10 establishes the blanket claim. Comparing it with src/ci/sarif.rs shows direct fields without #[serde(rename = ...)] throughout Log, Run, Tool, Driver, Rule, Finding, Physical, Artifact, and related types, while only names such as $schema, informationUri, ruleId, and partialFingerprints have explicit renames. The golden comparison still usefully guards the complete emitted shape, but the documented rename-specific justification applies only to the subset whose Rust and wire names differ.

Consequence: A maintainer following this rationale may incorrectly assume every SARIF field depends on a rename annotation and misunderstand which edits can actually cause the cited snake_case-versus-camelCase compatibility failure.

Recommended action: Narrow the sentence to say that every SARIF field whose wire name differs from its Rust identifier uses a hand-written serde rename, and retain the broader explanation that the golden protects the entire document shape.

Impacted locations:

• crates/cargo-gamma-lib/tests/sarif_contract.rs:8-10

Reviewed in: #113

---

Title: Define the expected-output fixture and use neutral consumer terminology

[Copilot speaking]

The new module documentation introduces the specialized term “golden” in its opening sentence without defining it, then describes the compatibility boundary using conversational phrases such as “other people's tools” and “somebody deliberately re-blesses the file.” The failure message repeats the same terminology with “somebody else's tool.”

Location: crates/cargo-gamma-lib/tests/sarif_contract.rs:6-19

Reproducible reasoning: A reader encounters “golden” on line 6 before the later text explains that the test compares emitted SARIF with a committed, manually reviewed expected document. Even that later explanation does not explicitly connect the mechanism to the term. The surrounding phrases refer to external consumers and intentional fixture updates, but express those concepts through personal and colloquial wording instead of the established technical concepts. Reading the block linearly therefore requires the reader to infer both the meaning of “golden” and the intended update workflow.

Consequence: A maintainer unfamiliar with golden-file testing may not understand that the fixture is the reviewed expected output or what action “re-bless” requires, while the conversational wording obscures that SARIF field names are compatibility contracts with external consumers.

Recommended action: Introduce the first occurrence as a committed expected-output fixture, optionally naming “golden file” as the parenthetical testing term, and consistently call it the expected fixture or golden file thereafter. Replace the personal phrasing with neutral terms such as “external tools” or “external consumers,” and describe re-blessing as intentionally updating the fixture after validating consumer compatibility.

Impacted locations:

• crates/cargo-gamma-lib/tests/sarif_contract.rs:6-19
• crates/cargo-gamma-lib/tests/sarif_contract.rs:73-75

Reviewed in: #113

---

Title: Describe SiteIndex as grouping the counts rather than preventing swaps

[Copilot speaking]

The new test documentation says SiteIndex keeps the occurrence and replacement index apart at the call site instead of leaving two adjacent integers for the reader to order correctly. Its constructor is still SiteIndex::new(occurrence: u32, replacement_index: u32), and the test's own calls contain the same adjacent u32 values. The type groups and names the values after construction, but it does not prevent or detect their accidental interchange.

Location: crates/cargo-gamma-lib/tests/units.rs:155-156

Reproducible reasoning: The claim can be checked directly from the documented test and the type's constructor. Lines 161-166 construct SiteIndex from two positional integers. The constructor in cargo-gamma-engine/src/model/identity.rs accepts two parameters of the same type and performs no validation that could distinguish a swapped call. The assertion then demonstrates that reversing the values is accepted and produces another valid identity; it does not demonstrate that the values cannot be interchanged. Therefore the documentation promises a safety property stronger than the API provides.

Consequence: A maintainer can reasonably conclude from this documentation that accidental count swaps have been designed out, even though such a swap still compiles and silently assigns a different mutant identity, detaching the corresponding cache, shard, or report knowledge.

Recommended action: Rewrite the comment and test name to state the actual benefit: SiteIndex groups the two identity coordinates into one named value and exposes named accessors, while its constructor still relies on argument order. If compile-time swap prevention is required, use distinct coordinate types or another construction API that names each field; that stronger remedy has broader call-site and revalidation cost, whereas the documentation-only correction preserves the current API.

References:

• crates/cargo-gamma/docs/DESIGN.md, Identity and knowledge across campaigns

Impacted locations:

• crates/cargo-gamma-lib/tests/units.rs:155-166
• crates/cargo-gamma-engine/src/model/identity.rs:147-168

Reviewed in: #113

---

Title: Rename the runaway-mutant test to match its remaining assertions

[Copilot speaking]

The change removes the elapsed-time measurement and the assertion that the run finishes well before its configured budget, but the test remains named a_runaway_mutant_is_cut_off_long_before_its_budget. The remaining assertions verify the reported verdict and stalled classification, not the quantitative timing claim in the name.

Location: crates/cargo-gamma-lib/tests/session.rs:1277-1279

Reproducible reasoning: Professional test names should state the behavior the test demonstrates. With the removed assertion, the test checks that the run succeeds, reports no survivor, emits TIMEOUT and stalled, and counts one timed-out mutant. Supplying a two-minute budget does not by itself establish how long execution took, so “long before” is no longer supported by the test body. The new comment accurately focuses on silence-based classification, making the older name inconsistent with the present test.

Consequence: Maintainers can reasonably but incorrectly treat this test as coverage for a prompt-cutoff guarantee; a substantial latency regression could leave every remaining assertion passing while the test name continues to claim the timing behavior is protected.

Recommended action: Rename the test to describe the verified classification, such as a_runaway_mutant_is_reported_as_stalled, and keep the documentation focused on the observable stalled verdict. Do not retain the quantitative “long before” wording unless a deterministic assertion actually verifies it.

Impacted locations:

• crates/cargo-gamma-lib/tests/session.rs:1274-1288

Reviewed in: #113

---

Title: Verify cache-lock continuity across the adoption-to-preparation handoff

[Copilot speaking]

The expanded integration test proves that another command is rejected while the first command is paused after cache adoption, then releases that pause, waits until workspace preparation has acquired or received locks, and proves rejection again. It does not observe the interval between those pauses. Consequently, it cannot distinguish continuous ownership of the originally claimed redirected-cache lock from an implementation that drops that lock after adoption and claims a new lock before reaching the preparation pause.

Location: crates/cargo-gamma-lib/tests/session.rs:413-415

Reproducible reasoning: The contract under test is continuous ownership: the redirected cache must remain unavailable from adoption through preparation. The first synchronous competing session finishes before adopted.release(), so it observes only the adoption endpoint. The second competing session starts only after preparing.wait() returns, so it observes only the preparation endpoint. No contender or identity assertion spans the handoff. A regression that releases the CacheLocks passed from commands::run, then lets Workspace::prepare_with_locks reacquire them, satisfies both endpoint assertions while violating the interval that the test name claims to cover.

Consequence: A future refactor could reopen the redirected cache between adopting incremental state and preparing the workspace, allowing another cargo-gamma command to claim and modify the same reusable state concurrently even though this regression test remains green.

Recommended action: Make the test prove that the same claimed lock remains owned across the handoff without relying on a timing race. For example, add a test-only observation that identifies the lock pair received by workspace preparation and assert it is the pair claimed before adoption, or otherwise arrange a deterministic contender that cannot acquire the cache until the final preparation pause is released.

References:

• crates/cargo-gamma/docs/DESIGN.md, cache locking contract (lines 344-367)

Impacted locations:

• crates/cargo-gamma-lib/tests/session.rs:406-424

Reviewed in: #113

---

Title: Align the Linux containment contract with the unsupported-host fallback

[Copilot speaking]

The updated design and crate-level API documentation state that every Linux launch enters a cgroup and that a launch which cannot receive one is refused. The documented prepare implementation instead permits an unmetered launch to continue with process-group-only containment when cgroup creation reports Situation::Unsupported; containment reports that host-wide limitation and PreparedCommand::sealed remains false.

Location: crates/cargo-gamma-process/docs/DESIGN.md:24-27

Reproducible reasoning: In process_tree.rs, prepare handles Cgroup::create returning Situation::Unsupported with !request.wanted() by storing no cgroup and returning a prepared command successfully. Therefore a Linux launch can be accepted without a cgroup when the host lacks cgroup support. This contradicts the absolute statements added to DESIGN.md and lib.rs, while the implementation's own API documentation describes the fallback explicitly.

Consequence: Callers and maintainers can incorrectly assume that sealed() == false is impossible on Linux or that auxiliary unmetered launches never run with best-effort containment, leading to incorrect policy, diagnostics, or future changes.

Recommended action: Choose one Linux containment contract and align the design, crate overview, prepare, containment, sealed, and tests. If unmetered launches may fall back to a process group when cgroups are unsupported, qualify the absolute cgroup/refusal statements and define the resulting unsealed state. If every Linux launch must be sealed, change the unsupported branch to refuse those launches. Preserve the existing distinction between host-wide unsupported capability and per-launch refusal whichever direction is selected.

References:

• crates/cargo-gamma-process/src/process_tree.rs (prepare fallback and error contract)

Impacted locations:

• crates/cargo-gamma-process/docs/DESIGN.md:24-27
• crates/cargo-gamma-process/src/lib.rs:39-44

Reviewed in: #113

---

Title: Clean up a spawned command abandoned before adoption

[Copilot speaking]

SpawnedCommand owns a live Child and its containment guard without cleanup-on-drop, so safe callers can abandon a successful child before ProcessTree::adopt consumes it.

Location: crates/cargo-gamma-process/src/process_tree.rs:309-319

Reproducible reasoning: SpawnedCommand contains the child and guard but provides only id; unlike ProcessTree, it has no Drop implementation that kills and waits for its child. Dropping the adoption-pending value therefore closes its handles without establishing that the child was terminated and reaped.

Consequence: Safe code can leave a successfully spawned process untracked and potentially unreaped. Such a process can survive cancellation or cleanup on Unix, or remain suspended outside the managed job on Windows, defeating the process-tree lifecycle that the API is intended to enforce.

Recommended action: Ensure abandoning a SpawnedCommand before adoption terminates and reaps its child.

References:

• crates/cargo-gamma-process/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-process/src/process_tree.rs:309-327

Reviewed in: #113

---

Title: Place the public re-exports before the submodule declarations

[Copilot speaking]

The change adds PlatformError, Situation, PreparedCommand, and containment to the crate's direct public surface in the pub use block after all private and public submodule declarations. The assigned layout guideline requires submodules and namespaces to appear below direct members.

Location: crates/cargo-gamma-process/src/lib.rs:84

Reproducible reasoning: The crate declares its private and public submodules before the cohesive direct-member re-export block. A reader therefore encounters implementation namespaces before the crate's public entry points. Moving the re-exports above the module declarations satisfies the importance-first order without changing name resolution or ownership semantics.

Consequence: The crate root presents internal organization before its public entry points, making the exported process-tree API harder to identify and encouraging future direct members to continue accumulating below the namespace block.

Recommended action: Move the complete pub use block above the module declarations, keeping each re-export and its #[doc(inline)] attribute together; leave the submodule declarations as the final item block.

Impacted locations:

• crates/cargo-gamma-process/src/lib.rs:84

Reviewed in: #113

---

Title: Define sealed containment before exposing it as a state

[Copilot speaking]

The design introduces a "sealed boundary," and the module overview says that a host may "seal" a launch and links to ProcessTree::sealed, but neither high-level document defines the term. The lower-level API documentation uses the specific meaning "a boundary its descendants cannot leave," which is the distinction these overviews need to establish before using sealed as a named state.

Location: crates/cargo-gamma-process/docs/DESIGN.md:51-54

Reproducible reasoning: The assigned terminology guideline requires specialized terms to be defined before use, with key API terms defined in a package or module overview. Here, sealed is central to the containment contract and to a public Boolean method, but readers have to infer its meaning from surrounding platform examples or follow the link into lower-level API documentation. A successful launch, an installed process group, and a descendant-proof boundary are different conditions; only the last one is intended by sealed.

Consequence: A reader can interpret sealed as merely meaning that containment setup succeeded, and therefore misunderstand why a best-effort process-group launch reports false even though it still has some containment.

Recommended action: Define the term before its first use in both high-level documents, for example by stating that sealed containment uses a boundary that descendants cannot leave, and then use that defined term consistently when describing containment and ProcessTree::sealed.

Impacted locations:

• crates/cargo-gamma-process/docs/DESIGN.md:51-54
• crates/cargo-gamma-process/src/lib.rs:39-44

Reviewed in: #113

---

Title: Name the reliable containment capability explicitly

[Copilot speaking]

The design says that cleanup sweeps "the boundary that names a directory or a handle, because only the latter" can still reach the run's descendants. The nearest grammatical pair is "a directory or a handle," so "the latter" reads as the handle alone. The intended contrast appears to be between reusable numeric identities and the nonnumeric containment boundary, not between the two platform-specific forms of that boundary.

Location: crates/cargo-gamma-process/docs/DESIGN.md:55-59

Reproducible reasoning: The sentence introduces several specialized concepts at once: numeric capabilities, a retained child handle, a process-group identifier, and a boundary represented by either a directory or a handle. It then uses a relative phrase whose referent is ambiguous. The Linux implementation relies on the cgroup represented by a directory before revoking the process-group identity, so reading "the latter" as only the handle contradicts the Linux case. A reader should not need implementation knowledge to resolve which capability remains trustworthy.

Consequence: Maintainers can conclude that the directory-backed Linux boundary is no longer reliable after an external reap, obscuring the reason that the cgroup must be swept before numeric process identities are discarded.

Recommended action: Rewrite the bullet to name the two capability categories and the surviving one directly. If the external-reap behavior is platform-specific, describe that scope explicitly instead of grouping directory- and handle-backed boundaries behind "the latter."

Impacted locations:

• crates/cargo-gamma-process/docs/DESIGN.md:55-59

Reviewed in: #113

---

Title: Use one term for retryable spawn failures

[Copilot speaking]

The refreshed process-launch documentation describes the same retry scenario as a failed spawn, a transient shortage, and a transient spawn refusal. These differences are not defined, even though SpawnFailure can also carry permanent operating-system errors that should not be retried.

Location: crates/cargo-gamma-process/docs/DESIGN.md:37-39

Reproducible reasoning: The module overview says a failed spawn returns SpawnFailure for retry and backoff, the design says a transient shortage can retry, and PreparedCommand::backoff says transient spawn refusal. Failure, shortage, and refusal have different breadth, so readers cannot tell which io::Error values are eligible without finding the coordinator's separate transient-error classifier.

Consequence: API readers may infer that every SpawnFailure should be passed through backoff, including permanent errors for which waiting cannot help.

Recommended action: Choose one precise term, such as a transient resource-related spawn failure, and use it in the module overview, design, and backoff documentation. State that callers must classify the underlying io::Error before retrying.

Impacted locations:

• crates/cargo-gamma-process/docs/DESIGN.md:32-39
• crates/cargo-gamma-process/src/lib.rs:52-58
• crates/cargo-gamma-process/src/process_tree.rs:289-299

Reviewed in: #113

---

Title: Propagate ordinary launch failures in the usage example

[Copilot speaking]

The new prepare example handles prepare, spawn, and ProcessTree::adopt with generic expect calls. Each operation documents ordinary failures that callers can encounter, including unsupported or refused containment, interruption, and operating-system spawn errors. As a copyable API example, it should demonstrate propagation or deliberate handling of those failures rather than turning them into panics.

Location: crates/cargo-gamma-process/src/process_tree.rs:482-484

Reproducible reasoning: The # Errors section immediately above the example makes these outcomes part of normal operation rather than impossible states. The surrounding production call sites preserve that information by propagating or matching the errors, while the example discards it with messages that only name the operation. A reader following the example therefore learns a less robust error-handling pattern than the API and its structured errors are designed to support.

Consequence: A caller copying the example can panic on a host without delegated containment, during shutdown, or after a routine spawn refusal instead of reporting the actionable platform or I/O reason through its normal error path.

Recommended action: Wrap the example in a fallible entry point and use ?, hiding only wrapper boilerplate if needed, or show an explicit match when the example should teach how to classify containment failures. Keep the compile-fail example focused on ownership, but avoid presenting generic panics as the normal successful-usage pattern.

References:

• crates/cargo-gamma-process/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-process/src/process_tree.rs:482-484

Reviewed in: #113

---

Title: Document when to advance the cached helper name

[Copilot speaking]

The helper is now cached under gamma-process-helper-3, and build_or_reuse_helper returns an existing binary without checking whether it was compiled from the current HELPER_SOURCE. The changed suffix evidently invalidates the previous cached helper after adding the flee directive, but no comment records that maintenance contract.

Location: crates/cargo-gamma-process/src/testing.rs:114

Reproducible reasoning: At line 114, helper_binary_path supplies a fixed name to build_or_reuse_helper. That function returns immediately when a file with that name already exists, so changes to the embedded helper source are invisible unless the name also changes. This pull request changes both HELPER_SOURCE and the suffix, which demonstrates that the numeric literal is a manual cache generation rather than an arbitrary test name. Without documenting why this literal exists and when it must change, a future maintainer cannot independently reproduce the invalidation decision from the code.

Consequence: A later helper-source change can accidentally reuse a stale binary from target/test-work, causing tests to exercise old behavior or fail because a newly added directive is unavailable.

Recommended action: Add a comment explaining that the suffix is the helper cache generation and must be advanced whenever HELPER_SOURCE changes, or derive the cache key from the source so invalidation is automatic. The comment should explain the invariant without repeating the current numeric value.

Impacted locations:

• crates/cargo-gamma-process/src/testing.rs:114

Reviewed in: #113

---

Title: Narrow the deterministic startup-capture claim for fallback Unix targets

[Copilot speaking]

The pull request summary still says runtime mutant selection and census capture are deterministic across concurrent workloads. The refreshed runtime and design documentation now state that non-Linux Unix capture uses getenv under a process-wide precondition that no native environment mutation runs concurrently, placing a foreign native constructor that violates the precondition outside the runtime abstraction.

Location: crates/cargo-gamma-rt/src/lib.rs:128-134

Reproducible reasoning: The refreshed documentation correctly qualifies the fallback: POSIX getenv capture assumes no concurrent native environment mutation, and the second read is only integrity detection for a visibly inconsistent result. The pull request description still makes an unqualified cross-platform concurrency claim, so it promises more than the documented fallback contract when a foreign native constructor mutates the environment during startup.

Consequence: A reviewer or maintainer can approve the change believing concurrent startup selection is fully deterministic on every supported Unix platform, then rely on that guarantee in an environment that does not satisfy the fallback capture precondition.

Recommended action: Qualify the pull request summary so its deterministic concurrent-workload claim excludes fallback Unix startup with concurrent native-constructor environment mutation, or scope the claim to platforms with an immutable startup environment source.

References:

• Pull request #113
• crates/cargo-gamma-rt/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-rt/docs/DESIGN.md:47-53

Reviewed in: #113

---

Title: Describe the process-group escape directly

[Copilot speaking]

The new helper comment describes process_group(0) as "The escape a process group has no answer to" and says cgroup leaves and job objects are not "renounceable". Those metaphors obscure the concrete behavior being tested: the child starts a new process group, so signals sent to the parent group no longer reach it, while cgroup and job-object membership cannot be left through the same call.

Location: crates/cargo-gamma-process/src/testing.rs:95-96

Reproducible reasoning: The assigned writing guidance requires professional English and clear, consistent terminology. "Has no answer to" personifies a process group, and "renounceable" is an uncommon label for whether a child can opt out of a containment mechanism. A direct description of group membership and signal delivery lets a maintainer reproduce why the flee directive exists without decoding the metaphor.

Consequence: A maintainer changing this adversarial test helper can misunderstand which containment boundary the child escapes or why the comparison with cgroups and job objects matters.

Recommended action: Replace the comment with a direct explanation, for example: starting a new process group removes the child from the parent group, so later group-directed signals do not reach it; a child cannot opt out of a cgroup leaf or Windows job object through this mechanism.

Impacted locations:

• crates/cargo-gamma-process/src/testing.rs:95-96

Reviewed in: #113

---

Title: Use path consistently for the census destination

[Copilot speaking]

The added design paragraph first calls the captured value a "census path" and then calls the same value "a name that fills the buffer". Here the value is the complete destination path stored in GAMMA_CENSUS; "name" can instead suggest the environment-variable name or only the final file name.

Location: crates/cargo-gamma-rt/docs/DESIGN.md:55-59

Reproducible reasoning: The terminology guidance requires distinct concepts to use distinctive terms and discourages introducing synonyms for an existing term. These adjacent sentences discuss one value and one size limit, so switching from "path" to "name" introduces ambiguity without distinguishing a new concept.

Consequence: A reader can incorrectly conclude that the limit applies only to a file name rather than to the complete census path value.

Recommended action: Replace "A name that fills the buffer" with "A census path that fills the buffer" or "A path value that fills the buffer", and retain path terminology throughout this paragraph.

Impacted locations:

• crates/cargo-gamma-rt/docs/DESIGN.md:55-59

Reviewed in: #113

---

Title: Define native constructors before referring to them

[Copilot speaking]

The added design text says signal retries avoid spinning "inside a constructor", and the crate overview later discusses "native constructors", but neither assigned document explains this specialized startup mechanism before using the term.

Location: crates/cargo-gamma-rt/docs/DESIGN.md:38-40

Reproducible reasoning: The terminology guidance requires specialized terms to be defined before usage in documents and package-level API documentation. A Rust reader may interpret "constructor" as object construction, while this text means an operating-system loader or C-runtime startup hook that executes before main; that distinction is central to the described failure window.

Consequence: Readers unfamiliar with native startup hooks cannot tell when the capture runs or why ordinary error reporting and synchronization options are unavailable.

Recommended action: At the first mention, define a native constructor as a loader/CRT startup hook that runs before main, then use that term consistently in the design and crate overview.

Impacted locations:

• crates/cargo-gamma-rt/src/lib.rs:123-134

Reviewed in: #113

---

Title: Align the environment-capture retry budget across open and read operations

[Copilot speaking]

The Linux startup-capture documentation says interrupted attempts consume one budget across the whole capture, but copy_environment and scan_environ each create an independent counter. After the open loop succeeds, the read loop receives a fresh allowance even if opening /proc/self/environ already consumed the documented budget.

Location: crates/cargo-gamma-rt/src/runtime.rs:767-784

Reproducible reasoning: copy_environment initializes interruptions before retrying open_fd and allows up to INTERRUPTED_ATTEMPTS interrupted opens. It then calls scan_environ, which initializes another interruptions counter and independently allows the same number of interrupted reads. A sequence of 64 interrupted opens followed by a successful open and 64 interrupted reads can therefore continue, although the constant documentation and docs/DESIGN.md state that the budget is spent across the whole capture rather than replenished per operation. The existing scan-level regression proves only that successful reads do not reset the scan counter; it cannot detect the reset at the open-to-read boundary.

Consequence: Startup can remain in the loader constructor for up to roughly twice the documented interruption allowance, and the implementation no longer matches the design contract used to justify the bounded retry behavior.

Recommended action: Make the implementation and documentation describe the same retry budget. If the budget is meant to cover the whole startup capture, pass one remaining allowance from the /proc/self/environ open loop into scan_environ and test a sequence containing both interrupted opens and interrupted reads. If separate per-operation budgets are intended, document the resulting combined maximum and justify that boundary instead of calling it one whole-capture budget.

References:

• crates/cargo-gamma-rt/docs/DESIGN.md, Selection and census protocol

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:685-691
• crates/cargo-gamma-rt/src/runtime.rs:720-731
• crates/cargo-gamma-rt/src/runtime.rs:767-784
• crates/cargo-gamma-rt/docs/DESIGN.md:34-40

Reviewed in: #113

---

Title: Keep the exported census limit documentation contractual

[Copilot speaking]

MAX_CENSUS_SITES is newly exported as the coordinator/runtime protocol limit, but its public documentation also describes the current bitmap representation, exact address-space cost, and paging behavior. Those details were appropriate on the former private SITES constant; moving the same paragraph onto a public constant makes implementation choices appear to be API promises.

Location: crates/cargo-gamma-rt/src/runtime.rs:108-113

Reproducible reasoning: The contractual part of this export is the maximum supported site ordinal and the overflow behavior that the coordinator uses to bound census files. Whether the runtime stores one bit per site, reserves a particular amount of virtual address space, or causes pages to be committed lazily is internal to REACHED and can change without changing the protocol. The documentation-design guideline requires API documentation to describe contractual promises rather than implementation details, so the public rustdoc should not bind future implementations to the current bitmap/storage model.

Consequence: A future change to sparse, compressed, or differently paged census storage can leave the public API documentation false or unnecessarily constrain an implementation change that preserves the wire contract.

Recommended action: Limit MAX_CENSUS_SITES rustdoc to the protocol maximum, overflow semantics, and its role as the shared coordinator/runtime bound; move the bitmap size and paging explanation to the private SITES or REACHED documentation.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:99-119

Reviewed in: #113

---

Title: Remove the tautological active documentation example

[Copilot speaking]

The changed documentation now marks assert_eq!(gamma_rt::active(), gamma_rt::active()) as an illustrative text block and explicitly acknowledges that the comparison proves nothing. It nevertheless remains an inline API example, and changing the fence from rust to text also removes rustdoc's ability to compile and check the snippet.

Location: crates/cargo-gamma-rt/src/runtime.rs:1862-1870

Reproducible reasoning: API examples are teaching surfaces that consumers may copy as recommended usage. This assertion makes two immediately adjacent calls and compares their results, so it demonstrates neither how callers should use the returned ordinal nor the documented startup-capture guarantee; the newly added comment says exactly that. Keeping acknowledged non-demonstrative code as an untested text block provides less guidance than the surrounding prose and the referenced process regression, while also allowing the snippet to drift from the Rust API without a doctest failure.

Consequence: A reader may copy a tautological assertion into production diagnostics or tests and believe it verifies environment immutability, even though it cannot exercise the startup transition that the contract describes. The uncompiled text block can also become stale without being detected.

Recommended action: Remove this code block and leave the contract plus the process-test reference in prose, or replace it with an executable Rust example that demonstrates a real consumer action, such as reading the ordinal once and branching on NONE. Removing it has the smallest maintenance cost; a replacement is worthwhile only if it can remain deterministic without depending on a specially launched process.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:1862-1870

Reviewed in: #113

---

Title: Document why the environment scan uses a 512-byte chunk

[Copilot speaking]

The new Linux environment scanner hardcodes a 512-byte chunk in both the reader callback type and the local stack buffer, and the test helper repeats the same size. The surrounding documentation explains why the environment is scanned incrementally, but not why this particular chunk size is the appropriate trade-off.

Location: crates/cargo-gamma-rt/src/runtime.rs:720-721

Reproducible reasoning: Any nonzero chunk size preserves the scanner's framing logic, while the selected size changes loader-constructor stack usage and the number of read syscalls needed to traverse /proc/self/environ. Because neither the function comments nor the runtime design document gives a platform constraint, measurement, or deliberately arbitrary policy for this value, a maintainer cannot independently reproduce the choice or know whether changing it is safe from a startup-cost perspective.

Consequence: A future maintainer may tune one occurrence or choose a materially different size without understanding the intended syscall-versus-stack trade-off, and the production and test callback types can drift apart.

Recommended action: Introduce one named chunk-size constant used by the production scanner and its scripted test helper, and justify its value with the relevant platform constraint or measurement; if it is intentionally arbitrary, state that and the trade-off it is meant to balance.

References:

• crates/cargo-gamma-rt/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:720-721
• crates/cargo-gamma-rt/src/runtime.rs:2445-2448

Reviewed in: #113

---

Title: Remove the duplicated EINTR number from its comment

[Copilot speaking]

The new EINTR constant's documentation spells out the same numeric value that the declaration contains. The comment usefully explains why the crate defines the constant itself, but repeating the literal creates a second representation of the value.

Location: crates/cargo-gamma-rt/src/runtime.rs:693-697

Reproducible reasoning: The assigned comment guidance explicitly says comments on values must not list the value, including recomputed or derived forms, because the comment can diverge from the declaration. Here the numeral appears immediately in both the documentation and the constant, so a later correction or portability adjustment has two independently editable copies.

Consequence: The prose can claim one Linux ABI value while the code uses another, leaving maintainers unsure which copy is authoritative.

Recommended action: Keep the dependency-free and cross-architecture rationale, but describe this as the Linux ABI-defined EINTR value without repeating the numeral; add a source reference that lets a maintainer verify the declaration.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:693-697

Reviewed in: #113

---

Title: Document how the interruption retry budget was selected

[Copilot speaking]

INTERRUPTED_ATTEMPTS introduces a fixed retry budget of 64. Its comment explains why retries must be bounded and argues qualitatively that the budget is generous, but it does not identify how this exact threshold, or an equivalent acceptable threshold, was derived.

Location: crates/cargo-gamma-rt/src/runtime.rs:683-691

Reproducible reasoning: Many positive thresholds satisfy the stated requirement to avoid an infinite constructor loop. Lower values increase the chance of refusing a valid startup during a signal burst, while higher values delay reporting a genuine signal storm. The comment's statement that reads take microseconds has no referenced measurement or explicit safety-factor calculation, so another maintainer cannot reproduce the balance represented by the chosen limit as required by the assigned literal-value guidance.

Consequence: The retry limit may be changed based on intuition, creating either avoidable startup failures or a substantially longer pre-main stall without reviewers being able to compare the new trade-off to the intended one.

Recommended action: Extend the constant's documentation with the source or measurement used to select the budget and the tolerated signal-burst assumption, or state that the value is deliberately arbitrary and define what range would be considered equivalent.

References:

• crates/cargo-gamma-rt/docs/DESIGN.md

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:683-691

Reviewed in: #113

---

Title: Document or remove the forced no-inline boundary

[Copilot speaking]

The newly added uninstalled_guard function is marked #[inline(never)], but its surrounding documentation only explains when the failure occurs and why it panics. It does not explain why the compiler must be forbidden from inlining this function or why the existing #[cold] annotation is insufficient.

Location: crates/cargo-gamma-rt/src/runtime.rs:1920

Reproducible reasoning: The performance guideline permits #[inline(never)] only with documented justification that specifically explains why inlining is forbidden. This attribute is added on a private cold failure helper at line 1920. The function-level documentation establishes semantic correctness but contains no performance, code-size, or compilation argument for forcing a call boundary, so the explicit requirement is not met.

Consequence: The runtime permanently overrides the compiler's inlining decision without leaving maintainers enough information to determine whether that restriction is necessary or remains beneficial as the guard implementation changes.

Recommended action: Either remove #[inline(never)] and rely on #[cold], or add a nearby justification explaining the concrete harm from inlining this failure path and why #[cold] alone does not provide the required behavior.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:1920

Reviewed in: #113

---

Title: Track or complete the outstanding census precheck measurement

[Copilot speaking]

The newly added bitmap precheck is explicitly described as lacking empirical validation: the comment says first-hit, repeated-hit, and unique-site throughput measurement remains outstanding. That deferred work is not linked to a follow-up work item, and the comment does not justify why leaving the validation incomplete is acceptable.

Location: crates/cargo-gamma-rt/src/runtime.rs:1212-1214

Reproducible reasoning: The precheck adds a relaxed atomic load before every census guard and skips the recorder lease only for sites already observed by that thread. It should help repeated hits, but it also adds work to first hits and unique-site-heavy censuses. Lines 1212-1214 acknowledge that the relevant comparison has not been measured, so the change itself identifies unresolved technical debt. The review guideline requires added technical debt to be justified and documented with a reference to the work item that will eliminate it; no such reference appears in this file or the pull-request context.

Consequence: The unvalidated optimization can remain indefinitely without an owner or completion criterion, and future maintainers cannot tell whether its extra atomic load materially improves representative censuses or merely shifts cost between workloads.

Recommended action: Either perform the stated low/high workload comparison now and document the conclusion, or explain why deferral is acceptable and add a reference to the follow-up work item that owns the measurement. If no follow-up is intended because structural reasoning is considered sufficient, remove the claim that measurement remains outstanding and document that decision instead.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:1212-1214

Reviewed in: #113

---

Title: Represent guaranteed non-zero lengths with NonZeroUsize

[Copilot speaking]

The newly introduced state enums store lengths as plain usize even though their variants guarantee that zero is impossible. CensusRequest::Path(usize) is produced only after Found(0) has been mapped to Absent, and EnvironmentReadOutcome::Found(usize) is produced only after a zero return has been mapped to Absent.

Location: crates/cargo-gamma-rt/src/runtime.rs:310

Reproducible reasoning: The focus guideline says that a non-zero integer parameter or field should prefer NonZero<usize> instead of relying on manual checks. In terminate_census_path, the zero case is removed before constructing CensusRequest::Path, so the payload's invariant is strictly positive. In environment_read_outcome, length == 0 returns Absent before Found(length) is constructed, establishing the same invariant. Keeping both payloads as usize means their types do not preserve the state distinction that the surrounding enums were added to express.

Consequence: Later code can construct CensusRequest::Path(0) or EnvironmentReadOutcome::Found(0) without a compiler error. In the census case, that contradictory state selects census mode while publishing a zero path length, so the file cannot be opened and the census is discarded instead of the invalid state being impossible.

Recommended action: Use core::num::NonZeroUsize for the Path and Found payloads, construct it at the existing zero checks, and call .get() only where the numeric length is required.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:309-310
• crates/cargo-gamma-rt/src/runtime.rs:383-384

Reviewed in: #113

---

Title: Conditionally document links to hosted-only runtime items

[Copilot speaking]

The new documentation on the unconditional selected function links to UNINSTALLED and uninstalled_guard, but those items only exist for hosted Unix/Windows targets outside Miri. The new fallback ACTIVE documentation also links to install while that static is compiled both for Miri and for targets with neither unix nor windows; install does not exist in the latter branch.

Location: crates/cargo-gamma-rt/src/runtime.rs:1884-1885

Reproducible reasoning: selected has no cfg, so its documentation is processed on every target. UNINSTALLED is gated by cfg(any(unix, windows)), while uninstalled_guard is gated more narrowly by cfg(all(any(unix, windows), not(miri))). Therefore the links in the refreshed selected documentation cannot resolve on a target with neither platform cfg, and the uninstalled_guard link also cannot resolve under Miri. Separately, the fallback ACTIVE static is enabled by cfg(any(not(any(unix, windows)), miri)); its install link resolves in the Miri-on-hosted branch because the function exists there, but not in the non-hosted branch. Documentation links must use conditions matching the linked item's availability and provide equivalent plain-text fallback wording when the item is absent.

Consequence: Generating API documentation for a supported non-hosted target, or documenting the runtime under Miri, emits broken intra-doc-link diagnostics; any documentation build that promotes warnings to errors fails instead of producing the target's documentation.

Recommended action: Split these sentences into cfg_attr(..., doc = "...") branches: retain clickable links exactly where install, UNINSTALLED, and uninstalled_guard exist, and emit the same wording with plain code formatting on branches where each linked item is absent.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:189-190
• crates/cargo-gamma-rt/src/runtime.rs:1881-1886

Reviewed in: #113

[sandersaares]

[Copilot speaking]

Review findings (continued 5)

Title: Terminate instead of panicking for a pre-install guard

[Copilot speaking]

uninstalled_guard uses a panic to enforce the stated rule that execution must not continue after a guard observes the transient UNINSTALLED state. A Rust panic does not guarantee process termination: arbitrary instrumented constructor code can enclose the guard call in catch_unwind, absorb this panic, and return normally. The later install constructor then overwrites UNINSTALLED with an ordinary selection, erasing the evidence that startup executed an untrustworthy guard.

Location: crates/cargo-gamma-rt/src/runtime.rs:1922-1926

Reproducible reasoning: This path exists specifically because another native constructor has already entered arbitrary instrumented Rust code before this runtime's constructor. Such Rust code may legitimately use std::panic::catch_unwind around the operation containing the injected guard. uninstalled_guard is an ordinary Rust function, so its panic unwinds through ordinary Rust frames and is catchable before reaching any non-unwinding constructor ABI boundary. After the catch returns, constructor processing continues and this crate's install function stores a normal value into ACTIVE. Therefore the implementation does not establish the documented invariant that observing UNINSTALLED makes the process fail and prevents its result from being trusted.

Consequence: For example, an earlier constructor can invoke a callback under catch_unwind; if a mutation guard in that callback observes UNINSTALLED, the callback catches the panic, startup continues, and the process can later finish successfully or emit a sealed census. Cargo Gamma may then classify a result from the exact startup ordering that this check declares untrustworthy.

Recommended action: Replace the panic with an allocation-free diagnostic followed by process termination that cannot unwind or be caught, on every hosted target supported by this constructor mechanism. Reuse the existing immediate-exit style where possible, add the corresponding Windows termination path, and add a child-process regression that places the guard under catch_unwind and verifies execution cannot continue past it.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:1922-1926

Reviewed in: #113

---

Title: Replace informal failure metaphors with precise outcomes

[Copilot speaking]

The added comments use informal expressions such as "fail loudly", a startup with "nothing wrong with it", and a test action that would "ruin" a process or cause "damage". These phrases refer to different concrete outcomes but do not name them.

Location: crates/cargo-gamma-rt/src/runtime.rs:181-184

Reproducible reasoning: Professional technical English should describe observable results precisely. Here the code already supplies precise vocabulary: startup failures emit the environment-error marker and exit, interruptions are retried, forcing UNINSTALLED would invalidate sibling tests in the shared process, and affected guards would panic. Using those outcomes lets a reader distinguish mechanisms without interpreting metaphors.

Consequence: A maintainer can misread whether "loudly" means a panic, a diagnostic marker, or a non-zero status, and whether "ruin" or "damage" refers to memory safety, process state, or test-result contamination.

Recommended action: Replace the remaining informal phrases with their exact outcomes, such as "emit the startup marker and exit", "transient interruption", and "invalidate sibling test results by making their guards panic". This comments-only cleanup leaves validated behavior unchanged.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:181-184
• crates/cargo-gamma-rt/src/runtime.rs:685-687
• crates/cargo-gamma-rt/src/runtime.rs:754-757
• crates/cargo-gamma-rt/src/runtime.rs:820-826
• crates/cargo-gamma-rt/src/runtime.rs:2867-2868
• crates/cargo-gamma-rt/src/runtime.rs:2961-2965

Reviewed in: #113

---

Title: State cgroup error requirements without failure consequences

[Copilot speaking]

The new Cgroup::create and Cgroup::arm error documentation combines their contractual Situation classifications with narratives about what happens if the caller proceeds without containment. The caller requirements are useful, but the consequences of violating them are implementation rationale rather than API contract.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs:706-709

Reproducible reasoning: For create, the contract is that host-wide capability failures are Unsupported, per-launch setup failures are Refused, and a refused launch must not proceed. For arm, the contract is that failure to open cgroup.procs is Refused and the command must not be spawned. Phrases such as running inside a boundary the launch does not have or running outside the boundary entirely explain why those requirements exist. The documentation-design rule requires API documentation to state requirements without documenting the consequences of violating them; that rationale can remain in implementation comments or the implementation guide without becoming part of the public promise.

Consequence: The public error contract becomes harder to scan and future containment changes must preserve explanatory failure scenarios that callers do not need in order to handle PlatformError correctly.

Recommended action: Keep the Situation::Unsupported/Situation::Refused conditions and the requirement not to launch after refusal in the public rustdoc. Move or remove the sentences describing the uncontained execution that would result from ignoring those requirements.

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:618-628
• crates/cargo-gamma-unsafe/src/cgroup.rs:706-709

Reviewed in: #113

---

Title: Document the meaning of the test process-group identifiers

[Copilot speaking]

The new interrupt-registration tests use three conspicuous hexadecimal process-group identifiers without explaining whether their values encode a boundary, protocol marker, or merely arbitrary valid identities. The second test additionally relies on the replacement identifier being distinct from the original one.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs:939

Reproducible reasoning: The interrupt registry accepts process-group identifiers greater than its empty sentinel, while the test logic only needs valid non-empty identifiers and, for the replacement scenario, two distinct identities. Nothing near the declarations at lines 939, 976, and 983 records those constraints or says that the hexadecimal patterns have no additional meaning. A maintainer therefore cannot independently reproduce equivalent values from the test's rationale and must reverse engineer the registry implementation to decide which edits are safe.

Consequence: A future maintainer may preserve accidental digit patterns as though they were significant, or replace the identifiers with equal or reserved values and make the test exercise a different path than intended.

Recommended action: Add a comment or documented local helper/constant stating that these are arbitrary valid process-group identifiers and that the original and replacement identifiers must be distinct. Keep the explanation constraint-based rather than repeating the literal values.

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:939
• crates/cargo-gamma-unsafe/src/cgroup.rs:976-983

Reviewed in: #113

---

Title: Avoid recapturing a backtrace for every unsupported cgroup attempt

[Copilot speaking]

Cgroup::create now converts the settled, host-wide root() failure into a fresh PlatformError on every call. Every Linux process launch calls this API, including unmetered launches that deliberately discard Situation::Unsupported and fall back to a process group. Because PlatformError::new eagerly calls Backtrace::capture, a run started with backtraces enabled repeatedly captures and then discards the same unsupported-capability stack.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs:629-630

Reproducible reasoning: root() caches one Result<PathBuf, String> for the process, so an unsupported host returns the same standing reason after the first probe. Cgroup::create nevertheless constructs a new structured error at line 630. The launch path explicitly invokes Cgroup::create for every listing, census, build, and test process, and treats an unsupported result as an expected fallback when no memory accounting was requested. PlatformError::new captures a backtrace at construction when backtraces are enabled. Therefore a campaign on a common undelegated Linux host can perform one stack capture per child launch even though most of those errors are immediately discarded.

Consequence: For example, running a large mutation campaign with RUST_BACKTRACE=1 on a CI runner without delegated cgroups can add thousands of stack walks and associated allocations before spawning children, increasing coordinator CPU time and memory traffic without producing additional diagnostic information.

Recommended action: Represent the cached unsupported capability with a cheap value and defer PlatformError construction until the caller will actually return or report the failure. For example, let the unmetered fallback inspect the cached situation directly, and construct one backtrace-bearing error only for a metered request that cannot proceed. Preserve backtraces for per-launch Refused failures, where each stack can identify a distinct failed operation.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md, structured platform errors
• crates/cargo-gamma-process/src/process_tree.rs, per-launch cgroup preparation
• crates/cargo-gamma-unsafe/src/platform_error.rs, eager backtrace capture

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:629-630
• crates/cargo-gamma-process/src/process_tree.rs:554-557
• crates/cargo-gamma-unsafe/src/platform_error.rs:33-38

Reviewed in: #113

---

Title: Represent the watched process-group identifier as non-zero

[Copilot speaking]

The new CgroupWatch state stores its process-group identifier as a plain i32, although a watch can only be created after Registry::claim has rejected identifiers less than or equal to zero. The field therefore has a non-zero (and in fact positive) invariant that its type does not express.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs:598

Reproducible reasoning: Spawning::watch_cgroup first calls watch, and Registry::claim returns None unless group > 0; only after that validation does watched_at create CgroupWatch. Every valid CgroupWatch::group is consequently non-zero. The assigned type-design guideline prefers NonZero<T> for fields with this invariant so invalid zero values cannot be represented accidentally by future constructors or refactors.

Consequence: A later crate-internal call or refactor can construct watch state with group zero, causing Drop to attempt a release that cannot identify the actual registry entry and leaving the cgroup descriptor registered past its owner's lifetime.

Recommended action: Store the identifier as NonZeroI32, converting it at the successful claim boundary, or use a small positive-process-group newtype backed by NonZeroI32 if positivity also needs to be encoded. Pass its value to the registry only when releasing the watch.

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:783-784

Reviewed in: #113

---

Title: Make the Loom selector participate in ordinary test builds

[Copilot speaking]

The newly added cgroup concurrency models are compiled only under the raw loom configuration flag. In the crate manifest, the Cargo feature named loom merely selects the loom integration-test target, while the target and the Loom dependency are themselves guarded by cfg(loom). Cargo features do not set a same-named configuration flag, so cargo test --all-features selects an integration test whose entire source is compiled away and does not compile or run these new models.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs:1895-1896

Reproducible reasoning: The assigned focus requires feature-dependent code to be available in test builds and says tests should not need feature gates. Enabling the manifest's additive loom feature changes only Cargo's feature set; it does not make cfg(loom) true. The new module at this location therefore remains absent unless the caller also supplies the out-of-band RUSTFLAGS="--cfg loom". The integration target has the same raw cfg at its crate root, so an ordinary all-features test build succeeds with no Loom test instead of revealing that the feature-associated coverage was skipped.

Consequence: A normal cargo test --all-features validation can report success even if the added cgroup models no longer compile or their integration entry point is broken, leaving the concurrency regression checks dependent on a separate undocumented-at-the-call-site flag combination.

Recommended action: Remove the empty Cargo feature used only as a target selector, or restructure the model support so ordinary test builds compile the models with Loom supplied as a non-optional dev-dependency. If the custom cfg(loom) build must remain for swapping production atomics, keep that as a separate specialized run while ensuring the Cargo feature and all-features test path cannot silently select an empty test.

References:

• crates/cargo-gamma-unsafe/Cargo.toml
• crates/cargo-gamma-unsafe/tests/loom.rs

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:31-45
• crates/cargo-gamma-unsafe/Cargo.toml:30-36
• crates/cargo-gamma-unsafe/tests/loom.rs:4-9

Reviewed in: #113

---

Title: Place LeafOwner after the primary cgroup types

[Copilot speaking]

The new private LeafOwner parsing helper is defined hundreds of lines before the public Cgroup capability. The assigned layout guidance requires types to be ordered by descending importance.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs

Reproducible reasoning: Cgroup is the file's public accounting-boundary capability and is identified by the design materials as the high-importance type for this module. LeafOwner only represents fields parsed from an internal leaf-directory name, so placing the new helper at line 152 before Cgroup at line 574 adds an ordering inversion: a local implementation detail precedes the module's primary type.

Consequence: A reader encounters internal stale-leaf bookkeeping before the module's central abstraction, making the file's type hierarchy harder to scan from primary capabilities to supporting details.

Recommended action: Move the newly added LeafOwner definition below the module's primary and externally used supporting types. Rust permits leaf_owner and related functions to refer to a type declared later in the module, so this does not require changing their behavior.

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:152-155

Reviewed in: #113

---

Title: Name the process-group identifier explicitly in the CgroupWatch documentation

[Copilot speaking]

The new CgroupWatch documentation calls the stored process-group identifier simply "the group" while discussing a cgroup, then uses pronouns with unclear antecedents in "the group it was claimed for is still in it" and the grammatically incomplete "slots that frees". The design document distinguishes Unix process groups from Linux cgroups, but this type-level explanation collapses those terms at the point where their relationship matters.

Location: crates/cargo-gamma-unsafe/src/cgroup.rs:592-596

Reproducible reasoning: CgroupWatch stores both a registry slot and an i32 process-group identifier so Drop can clear the slot only if that same process group still owns it. The surrounding type is itself a Cgroup, so the bare term "group" can reasonably be read as either the process group or the cgroup. Because the sentence does not name the signal handler as the subject of "frees", it also does not clearly explain that a sweep can clear the slot and a later spawn can reuse it before the cgroup is dropped. Using the distinct terms already established in docs/DESIGN.md makes the ownership invariant reproducible without reverse engineering interrupt.

Consequence: A maintainer can misunderstand slot ownership as following the cgroup's lifetime and change the release logic to clear a slot unconditionally, which could remove a later process group's live interrupt registration.

Recommended action: Rewrite the type comment to name the stored value as the process-group identifier and state the reuse sequence with explicit actors. For example, explain that a signal-handler sweep clears a killed process group's slot, a later spawn may reuse that slot before this Cgroup is dropped, and CgroupWatch retains the original identifier so Drop clears only the registration it owns.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md, Boundaries

Impacted locations:

• crates/cargo-gamma-unsafe/src/cgroup.rs:592-596

Reviewed in: #113

---

Title: Describe the ECHILD predicate without claiming a particular cause

[Copilot speaking]

The new reaped_elsewhere helper is documented as proving that another waiter already reaped the child, but it returns true for every ECHILD. The accompanying test obtains ECHILD from PID 1, which was never this process's child, and still expects the helper to return true.

Location: crates/cargo-gamma-unsafe/src/group.rs:47-50

Reproducible reasoning: POSIX uses ECHILD when the requested process is not waitable as a child of the caller. That can result from a known child having already been reaped, but the error alone does not establish that history. The implementation checks only cause.raw_os_error() == Some(libc::ECHILD) and receives neither the PID nor evidence that the process was previously the caller's child. The test demonstrates the broader classification while the helper's name and documentation describe the narrower causal conclusion.

Consequence: A caller can treat this public predicate as proof that a competing waiter consumed a known child and make lifecycle, capability-revocation, diagnostic, or recovery decisions based on a guarantee the API does not provide.

Recommended action: Rename and document the predicate to describe the observable ECHILD or no-waitable-child condition. Keep the stronger "reaped elsewhere" interpretation at call sites that can establish the required prior-child premise.

Impacted locations:

• crates/cargo-gamma-unsafe/src/group.rs:227-236

Reviewed in: #113

---

Title: Keep Loom mechanics out of the module API contract

[Copilot speaking]

The module-level rustdoc now explains that Loom models sequentially consistent accesses as acquire-release operations and that Loom builds therefore add explicit fences. This describes the verification backend and the current atomic encoding, rather than behavior a caller can rely on.

Location: crates/cargo-gamma-unsafe/src/interrupt.rs:24-27

Reproducible reasoning: Module //! documentation is part of the API documentation. The caller-facing contract here is the handshake's observable guarantee and the obligation to hold Spawning across the unwatched interval. Whether that guarantee is checked with Loom, and whether a particular Loom version needs extra fences, is an internal implementation choice. The same rationale is already recorded next to each #[cfg(loom)] fence, where it remains available to maintainers without presenting it as part of the module contract.

Consequence: Readers are encouraged to treat a test-framework-specific memory-model workaround as part of the API, and a future change to the verification tool or atomic implementation requires editing caller-facing documentation despite leaving all observable behavior unchanged.

Recommended action: Remove the Loom-specific sentence from the module rustdoc. Retain the explanation in ordinary implementation comments beside the Loom-only fences, or place broader verification design in an implementation guide if it needs a central description.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md

Impacted locations:

• crates/cargo-gamma-unsafe/src/interrupt.rs:24-27

Reviewed in: #113

---

Title: Centralize and justify the Loom process-group fixture value

[Copilot speaking]

Each newly added Loom model declares the same process-group literal 41, but neither declaration explains whether that value has protocol significance or is merely an ordinary valid group identifier.

Location: crates/cargo-gamma-unsafe/src/interrupt.rs:1304

Reproducible reasoning: The model only needs a positive process-group identifier distinct from the zero/negative sentinel space. A maintainer can plausibly choose many values and cannot tell from the local declarations whether changing 41 would alter coverage. Repeating the literal also makes the identical test premise look independently significant in each model rather than a shared fixture constraint.

Consequence: Future changes may preserve or alter the number for the wrong reason, or introduce inconsistent fixture values while trying to exercise what appears to be an undocumented boundary case.

Recommended action: Define one Loom-module fixture constant and document the reproducible constraint it satisfies, such as being an arbitrary positive group identifier distinct from the registry's empty marker, then reuse it in the models.

Impacted locations:

• crates/cargo-gamma-unsafe/src/interrupt.rs:1364

Reviewed in: #113

---

Title: Use the protocol's established technical terms instead of colloquial metaphors

[Copilot speaking]

New documentation describes protocol states as a child that 'no one was told about', a run that 'swallowed a Ctrl-C', and either side having 'performed the death'; the new identity module similarly refers to a permission decision 'nobody makes'. The surrounding modules already provide precise terms for these concepts: unwatched children, registered groups, deferred signal re-raising, and effective-identity access checks.

Location: crates/cargo-gamma-unsafe/src/interrupt.rs:1299-1307

Reproducible reasoning: The Loom model counts which side returns a signal for later re-raising; it does not directly count process deaths, so deaths, 'performed the death', and 'swallowed' blur distinct protocol events. Likewise, 'no one was told about' describes the already-defined unwatched or unregistered state indirectly. These phrases are colloquial and personify software components instead of using the module's established terminology, making the same concepts harder to recognize across the API documentation and tests.

Consequence: Readers can misread the model as observing actual process termination rather than assignment of the re-raise responsibility, and must translate several informal phrases back into the registry states used by the implementation.

Recommended action: Rewrite the changed prose and identifiers using the existing technical vocabulary. For example, name the model around an 'unwatched child', describe the interrupt as not being re-raised, rename the deaths recorder after re-raise or fatal-action responsibility, and state that the real user identity does not correspond to the identity used for access checks.

Impacted locations:

• crates/cargo-gamma-unsafe/src/interrupt.rs:1356-1361
• crates/cargo-gamma-unsafe/src/interrupt.rs:1408-1411
• crates/cargo-gamma-unsafe/src/identity.rs:11-14

Reviewed in: #113

---

Title: Keep Situation documentation limited to classification semantics

[Copilot speaking]

The new Situation API documentation defines run-level responses in addition to classifying platform failures: Unsupported says the run should refuse or degrade once, Refused says the caller should reject one launch, and support() repeats that the caller should not retry. Those decisions belong to the coordinating caller, while this crate's own module documentation says policy does not live here.

Location: crates/cargo-gamma-unsafe/src/situation.rs:14-25

Reproducible reasoning: Situation is the value exported by the unsafe platform layer so callers can distinguish a standing capability limitation, one failed operation, and interruption. Describing those distinctions is part of the type's contract. The additional statements about refusing a run, degrading, and retry policy describe what higher layers currently do with the value, not a requirement enforced by this crate. This is also inconsistent with lib.rs, which explicitly assigns policy to the caller, and with the design document's boundary that policy and limit calculation remain in cargo-gamma-lib.

Consequence: A higher layer that legitimately adds retry/backoff for a transient refusal or chooses a different user-facing response would contradict the published enum documentation even though the classification itself remains unchanged. The duplicated policy text can also drift from the coordinator's actual behavior.

Recommended action: Document only the semantic distinction each variant represents and what condition support() reports. Move the refuse/degrade/retry decisions to the cargo-gamma-process or cargo-gamma-lib design/API documentation where that policy is implemented.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md, Boundaries

Impacted locations:

• crates/cargo-gamma-unsafe/src/support.rs:18-23
• crates/cargo-gamma-unsafe/src/lib.rs:22-25

Reviewed in: #113

---

Title: Describe PlatformError without promising capture implementation costs

[Copilot speaking]

The public type documentation specifies that PlatformError uses Backtrace::capture and claims the precise allocation cost when backtraces are disabled. The API contract only needs to state that construction records a backtrace whose availability depends on process configuration; the chosen standard-library function and hot-path cost are implementation details.

Location: crates/cargo-gamma-unsafe/src/platform_error.rs:20-22

Reproducible reasoning: Callers can observe the backtrace through backtrace() and can therefore rely on its presence and enabled/disabled status. They do not need to know which constructor produced it or how many allocations construction currently performs. Naming Backtrace::capture and promising 'an allocation for its message and nothing else' exposes a design choice and performance characteristic that the requirements do not demand, so a storage or capture refactor would require changing API documentation despite preserving behavior.

Consequence: The rustdoc can become false when the capture implementation changes or when standard-library internals impose different work, and callers may make performance decisions based on a cost guarantee the crate does not need to maintain.

Recommended action: Keep the rustdoc at the contractual level: construction captures a backtrace, and it may be disabled unless backtrace capture is enabled for the process. Move the exact capture mechanism and cost rationale to an implementation comment or implementation guide if it remains useful.

Impacted locations:

• crates/cargo-gamma-unsafe/src/platform_error.rs:20-22

Reviewed in: #113

---

Title: Document the calibration of the handle-leak test values

[Copilot speaking]

The new handle-leak checks use fixed repetition counts, warm-up counts, and an allowed handle-count drift, but the nearby comments only say that there are "enough repetitions" and that the "first few" attempts may populate caches. They do not explain how the selected values were derived or what constraints an equivalent replacement must satisfy.

Location: crates/cargo-gamma-unsafe/src/job.rs:882-883

Reproducible reasoning: Both tests depend on the same three choices: repeat each injected failure enough times to amplify a one-handle leak, warm the process before taking the baseline, and tolerate some residual process-wide handle drift. The code selects 200 repetitions, eight warm-ups, and an allowance of eight handles. A maintainer can plausibly choose different values for every one of these parameters, but the comments provide no measurement, platform constraint, or statement that the values are intentionally arbitrary. The one-gibibyte limit used to enter the limited-job paths is likewise unexplained even though these tests do not exercise actual allocation.

Consequence: Future maintainers cannot tell whether changing these values weakens leak detection, increases false failures, or merely changes arbitrary fixtures, so maintenance of the tests requires rediscovering their calibration experimentally.

Recommended action: Document the reproducible rationale or calibration process for the repetition, warm-up, drift, and limit values. If the exact numbers are arbitrary, state the constraints they satisfy without duplicating the values, and centralize the shared values so both leak checks retain the same premise.

Impacted locations:

• crates/cargo-gamma-unsafe/src/job.rs:858
• crates/cargo-gamma-unsafe/src/job.rs:891-909
• crates/cargo-gamma-unsafe/src/job.rs:969-997

Reviewed in: #113

---

Title: Explain why the situation classification is non-exhaustive

[Copilot speaking]

The new public Situation enum is marked #[non_exhaustive], but its type documentation presents unsupported hosts, refused operations, and interrupted runs as the classification without explaining why callers must also handle unknown future variants.

Location: crates/cargo-gamma-unsafe/src/situation.rs:12-13

Reproducible reasoning: The attribute is an API design decision: it prevents downstream callers from exhaustively matching the three documented categories and preserves the option to add more categories without a source-breaking change. The crate design document also lists these same three categories as the structured failure model. Nothing in either location says whether additional categories are anticipated, whether the attribute is defensive policy for published implementation crates, or whether callers are expected to treat unknown categories in a particular way.

Consequence: A future maintainer cannot determine whether removing the restriction would violate an intended compatibility policy, while callers must add wildcard handling without understanding what behavior that fallback is meant to preserve.

Recommended action: Add a type-level explanation of the extensibility or compatibility reason for #[non_exhaustive] and, if relevant, the intended treatment of unknown classifications. If the three categories are deliberately closed for this internal API, remove the attribute instead.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md, Boundaries

Impacted locations:

• crates/cargo-gamma-unsafe/src/situation.rs:12-13

Reviewed in: #113

---

Title: State the constraints behind the native-error test values

[Copilot speaking]

The new source-retention test constructs native error code 13 and accepts any rendered source longer than one character, but neither literal is explained.

Location: crates/cargo-gamma-unsafe/src/platform_error.rs:112

Reproducible reasoning: The test's stated purpose is to prove that an operating-system cause is retained as the error source. That purpose does not reveal why this particular platform error number is representative or why a one-character rendering would be insufficient while a two-character rendering is sufficient. Both choices could be replaced by many alternatives, so a maintainer cannot independently reproduce the intended fixture and threshold from the test rationale.

Consequence: The test can be changed or ported based on a mistaken assumption that the numeric code or string-length boundary has semantic significance, or those values may be preserved indefinitely even though they do not directly express the retained-source contract.

Recommended action: Document the constraints these values satisfy without repeating them, or replace them with a self-describing deterministic cause and an assertion that directly expresses the retained property, such as checking the source kind or exact controlled message.

Impacted locations:

• crates/cargo-gamma-unsafe/src/platform_error.rs:119

Reviewed in: #113

---

Title: Avoid allocating static platform-error messages

[Copilot speaking]

PlatformError::new and PlatformError::because convert every message through Into<String>, so even the many static string literals used for unsupported and refused platform operations require a heap allocation. The type documentation explicitly identifies these errors as potentially occurring on a hot path.

Location: crates/cargo-gamma-unsafe/src/platform_error.rs:20-25

Reproducible reasoning: Both constructors store message: String and unconditionally call message.into(). Rust converts an &'static str into String by allocating and copying it. Related process-containment call sites commonly pass string literals, including per-launch refusal paths, so these errors pay an allocation even though the message already has static storage. A Cow<'static, str> field (or a separate static-message constructor) can borrow literals while still owning dynamically formatted paths.

Consequence: If a platform operation is repeatedly refused across mutant launches, each failure allocates and copies an otherwise static diagnostic, adding allocator traffic to an already failure-heavy campaign.

Recommended action: Store the message as Cow<'static, str> and accept impl Into<Cow<'static, str>>, or add a constructor dedicated to static messages while retaining the current owned-message constructor. This adds a borrowed/owned discriminator to the error representation, but avoids heap work for the common literal-message case without changing dynamic diagnostics.

Impacted locations:

• crates/cargo-gamma-unsafe/src/platform_error.rs:23-49

Reviewed in: #113

---

Title: Complete or narrow the promised Windows native-call fault coverage

[Copilot speaking]

The new unsafe-layer design states that Windows containment routes each native call through a private wrapper so tests can fail it in isolation, and native_faults.rs presents itself as covering the Win32 calls on which containment is built. The NativeCall catalog covers creation, assignment, release, configuration, and termination, but the memory-accounting path still invokes QueryInformationJobObject and GetQueuedCompletionStatus directly. Those failure-capable calls therefore cannot be driven through the new fault-injection mechanism.

Location: crates/cargo-gamma-unsafe/src/native_faults.rs:17-55

Reproducible reasoning: The design assertion is introduced by this pull request. Comparing it with the new NativeCall enum shows no variants for querying job accounting or reading completion-port notifications. In Job::usage, both operations remain direct unsafe calls rather than wrappers that consult native_faults::fired. Consequently, the added tests can exercise failures while building and controlling a job, but cannot exercise failures while reading the peak or determining whether the job-memory limit fired. The implementation therefore does not complete the scope that the new design and module-level documentation claim.

Consequence: A regression in either accounting failure path could remain invisible until a real Windows API failure, potentially losing the measured peak or the evidence used to classify memory exhaustion while the design tells maintainers that every such failure arm is injectable and covered.

Recommended action: Either add fault variants, private wrappers, and focused tests for QueryInformationJobObject and GetQueuedCompletionStatus (and audit the remaining containment-related Win32 calls against the same promise), or narrow the design and native_faults documentation to the specific lifecycle calls intentionally covered and explain why accounting-call failures are excluded. Adding wrappers preserves the stronger verification claim at some maintenance cost; narrowing the claim is simpler but leaves these accounting failure paths without deterministic coverage.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md, Boundaries

Impacted locations:

• crates/cargo-gamma-unsafe/src/native_faults.rs:17-55
• crates/cargo-gamma-unsafe/src/job.rs:331-364

Reviewed in: #113

---

Title: Derive the debug type name instead of hardcoding it

[Copilot speaking]

The new manual Debug implementation passes the literal "PlatformError" to debug_struct, even though the Rust style rule requires type names in string literals to be derived from the type.

Location: crates/cargo-gamma-unsafe/src/platform_error.rs:79

Reproducible reasoning: At platform_error.rs:79, the struct name is duplicated as text. Renaming or moving the type can therefore leave its debug rendering with a stale name because the compiler cannot connect this literal to the type declaration. core::any::type_name::<Self>() (or a helper based on it when only the final path component is desired) keeps the emitted name tied to the actual type.

Consequence: A future rename can produce diagnostics that claim the value has the old type, making logs and error reports misleading.

Recommended action: Replace the hardcoded debug_struct("PlatformError") argument with a name derived from Self, importing type_name at module scope if needed.

Impacted locations:

• crates/cargo-gamma-unsafe/src/platform_error.rs:79

Reviewed in: #113

---

Title: Reserve refusal terminology for Situation::Refused

[Copilot speaking]

The new API documentation describes every PlatformError and every Situation as a refusal, even though the enum deliberately separates Unsupported, Refused, and Interrupted and assigns them different handling semantics.

Location: crates/cargo-gamma-unsafe/src/situation.rs:6

Reproducible reasoning: Situation::Unsupported documents a host capability that will not change during the run, Situation::Refused documents a particular operation that may succeed on another attempt, and Situation::Interrupted documents teardown already in progress. PlatformError::new and PlatformError::because accept all of these variants, so calling the type, its constructors, and the Situation enum forms of refusal collapses the distinction that the API is designed to expose. A neutral umbrella term such as platform error or failed platform operation preserves the taxonomy, while refusal can remain specific to Situation::Refused.

Consequence: A caller reading the rustdoc can mistake Unsupported or Interrupted for the retryable per-operation category described by Situation::Refused and choose the wrong scope or retry behavior when handling the error.

Recommended action: Rewrite the PlatformError, constructor, accessor, and Situation overview documentation to use a neutral umbrella term, and use refusal only when describing Situation::Refused.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md

Impacted locations:

• crates/cargo-gamma-unsafe/src/platform_error.rs:13-17
• crates/cargo-gamma-unsafe/src/platform_error.rs:31-53
• crates/cargo-gamma-unsafe/src/situation.rs:4-6

Reviewed in: #113

---

Title: Describe platform behavior with precise, defined terms

[Copilot speaking]

The new public documentation relies on conversational and undefined descriptions such as hosts that 'answer this way', a job that 'would not take its child', a signal 'taking this run apart', a process being 'free to die', and PlatformError being canonical to unnamed 'two platform crates'.

Location: crates/cargo-gamma-unsafe/src/situation.rs:16-17

Reproducible reasoning: These phrases require readers to translate metaphors and an undefined repository category back into concrete behavior. The code already has exact concepts available: capability classification, AssignProcessToJobObject failure, interruption-driven teardown, imminent process termination, and the named cargo-gamma-unsafe and cargo-gamma-process crates. Using those terms directly makes the distinctions reproducible and satisfies the requirement to define specialized terms before relying on them.

Consequence: Rustdoc readers cannot determine from these descriptions which operation failed, what interruption state guarantees, or which crates share PlatformError without searching the implementation and workspace structure.

Recommended action: Replace the conversational phrases with direct descriptions of the operating-system state and operation, and name the crates explicitly instead of referring to 'two platform crates'.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md

Impacted locations:

• crates/cargo-gamma-unsafe/src/platform_error.rs:13-18
• crates/cargo-gamma-unsafe/src/situation.rs:14-31

Reviewed in: #113

---

Title: Move native fault injection behind a platform-call abstraction

[Copilot speaking]

The new Windows fault-injection path changes the implementation compiled for unit tests. thread_snapshot, main_thread, open_thread, previous_suspension_count, Job::assign, Job::terminate, and the job-creation wrappers all contain #[cfg(test)] early returns driven by native_faults, while those branches do not exist in a normal build.

Location: crates/cargo-gamma-unsafe/src/job.rs:96-98

Reproducible reasoning: lib.rs compiles native_faults only for Windows test builds. Each affected production function then consults that test-only state before making the Win32 call and returns a synthetic failure when armed. Therefore the same public operation and arguments execute different control flow in test and non-test builds. The testing guideline permits injecting mocks or fakes through a dedicated abstraction, and permits test hooks that only observe execution, but explicitly requires the production logic itself to remain identical. These early returns are behavioral test hooks rather than observational hooks.

Consequence: The failure-path tests can pass against branches that are absent from the shipped binary, so changes in conditional compilation or in the synthetic branches can make test behavior diverge from the production implementation they are intended to validate.

Recommended action: Introduce an injectable Win32-call backend (production implementation plus a test fake) and keep the Job and release algorithms unconditional, so tests replace only the platform dependency rather than adding test-only control flow to production functions. Retain the current per-thread fault selection in the fake if isolation is needed.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md

Impacted locations:

• crates/cargo-gamma-unsafe/src/job.rs:96-98
• crates/cargo-gamma-unsafe/src/job.rs:303-305
• crates/cargo-gamma-unsafe/src/job.rs:379-381
• crates/cargo-gamma-unsafe/src/job.rs:404-452
• crates/cargo-gamma-unsafe/src/lib.rs:77-78
• crates/cargo-gamma-unsafe/src/native_faults.rs:1-88

Reviewed in: #113

---

Title: Define the trait-path matching term before using it

[Copilot speaking]

The refreshed README says a trait exclusion list contains "lexical terminal trait names", but it never defines terminal as the final segment of the trait path as written. The qualification examples narrow the behavior but still leave the specialized term implicit.

Location: crates/cargo-gamma/README.md:808-811

Reproducible reasoning: The configuration reference later explains lexical matching, aliases, and the final written segment, but the README is the primary user guide and uses terminal before that definition. Defining the term inline would make clear why qualified spellings match and why aliases or two different traits sharing a final name are treated lexically.

Consequence: Users can remain unsure whether matching uses the complete trait path, its final written segment, an imported alias's resolved target, or report text, leading to an exclusion broader or narrower than intended.

Recommended action: Define the match directly at first use, for example: "compares each configured identifier with the final segment of the trait path as written, without name resolution." Keep the existing examples to demonstrate qualification-independent matching.

References:

• crates/cargo-gamma/docs/CONFIG.md

Impacted locations:

• crates/cargo-gamma/README.md:807-811

Reviewed in: #113

---

Title: State the external-viewer security risk precisely

[Copilot speaking]

The new report explanation ends with the informal phrase whatever that script turned out to be. This wording gestures at the risk but does not state the relevant browser behavior: externally loaded JavaScript can access and disclose the report's embedded source and results.

Location: crates/cargo-gamma/README.md:1108-1110

Reproducible reasoning: The paragraph is explaining why Gamma intentionally provides no external-viewer mode, so the security boundary is part of the user-visible rationale. Hand all of it to whatever that script turned out to be relies on conversational personification and leaves all of it and the threat unspecified. Naming the external script's access to embedded report data conveys the same decision in professional, reproducible terms.

Consequence: Readers may understand only that the viewer could change, not that loading it grants remotely controlled code access to sensitive source embedded in the report, weakening the justification for the self-contained-report requirement.

Recommended action: Replace the informal clause with a precise statement, such as: Loading an external viewer would allow remotely supplied JavaScript to read and disclose the source and results embedded in the report.

Impacted locations:

• crates/cargo-gamma/README.md:1108-1110

Reviewed in: #113

---

Title: Define filesystem security terms at first use

[Copilot speaking]

The redirected-cache security contract uses "sticky" for Unix directories and the acronym "ACLs" for Windows permissions without defining either term in this document.

Location: crates/cargo-gamma/docs/DESIGN.md:385-397

Reproducible reasoning: A reader reaches these terms while learning why a cache path is accepted or refused. Earlier sections do not define the Unix sticky bit or expand access-control list, so understanding the security boundary currently depends on platform-specific background knowledge. A short definition at first use would make the ownership rules understandable without requiring external reconstruction.

Consequence: Readers unfamiliar with one platform may misunderstand which shared Unix directories are accepted or why the equivalent ownership check is not claimed on Windows.

Recommended action: Name and briefly explain the Unix sticky bit when introducing the exception, and expand "access-control lists (ACLs)" at its first use.

Impacted locations:

• crates/cargo-gamma/docs/DESIGN.md:385-397

Reviewed in: #113

---

Title: Use precise host terminology for capability-dependent tests

[Copilot speaking]

The capability-testing principle describes containment guarantees as being made about "somebody else's machine." This colloquial ownership wording is less precise than the host terminology used throughout the surrounding design.

Location: crates/cargo-gamma/docs/DESIGN.md:803-806

Reproducible reasoning: The section is explaining that the executing host may not provide the operating-system capabilities needed to verify containment, metering, or signal handling. "Somebody else's machine" does not identify whether the relevant boundary is a developer workstation, CI runner, operating system, or test host, whereas "host environment" directly names the concept already used elsewhere in the document.

Consequence: The informal phrase weakens an otherwise precise cross-platform contract and makes the scope of the unavailable capability less clear.

Recommended action: Replace the phrase with professional host-oriented wording, such as stating that these guarantees depend on capabilities supplied by the host environment.

Impacted locations:

• crates/cargo-gamma/docs/DESIGN.md:803-806

Reviewed in: #113

---

Title: State the stated-value restriction without promising its failure mechanism

[Copilot speaking]

The new const fn/empty-body bullet correctly defines where #[gamma::value] is unsupported, but it also promises that either misuse is a compile error and that cargo gamma list mutants reports it. Those are consequences of violating the input restriction rather than the restriction itself, so this API-facing mutator reference turns the current validation implementation into an additional contractual promise.

Location: crates/cargo-gamma/docs/MUTATORS.md:865-866

Reproducible reasoning: The contractual rule needed by callers is that a stated value may only annotate a non-const function or method with a non-empty body. Lines 865-866 then move beyond accepted-input semantics and prescribe two specific reporting paths: compilation must fail and the listing command must emit the error. The documentation-contract guideline distinguishes requirements from the consequences of violating them because error delivery and validation placement are implementation choices that can change while the accepted API remains identical. Keeping these sentences in the reference would make a future change from proc-macro rejection to an earlier configuration/discovery diagnostic appear to break the documented API even though invalid input remains rejected.

Consequence: A future validation refactor cannot change when or where this invalid annotation is diagnosed without contradicting the public reference, unnecessarily freezing the proc-macro and listing implementation details as API behavior.

Recommended action: Keep the restriction in contractual terms, for example that #[gamma::value] is only supported on non-const functions or methods with non-empty bodies. Remove the promises about a compile error and cargo gamma list mutants, or place diagnostic behavior in explicitly non-contractual implementation documentation if maintainers still need it recorded.

Impacted locations:

• crates/cargo-gamma/docs/MUTATORS.md:865-870

Reviewed in: #113

---

Title: Restore the checkpoint-and-resume backlog item until resumable campaigns are implemented

[Copilot speaking]

The change deletes F2 even though the backlog defines deletion as meaning that work is complete. The current implementation does not satisfy F2: the complete run record is stored only after execution finishes, in-run persistence records killer-test hints rather than reusable completed verdicts, and gamma-progress.log is a human-readable journal with no resume consumer. There is no periodic atomic partial record, bounded checkpoint cadence, restart path that validates and reuses completed work, or end-to-end interruption/resume coverage.

Location: crates/cargo-gamma/docs/TODO.md:8-35

Reproducible reasoning: F2 requires explicitly partial records that periodically preserve completed verdicts, compiler-confirmed unviability, probes, ordering and validation state, while retaining a previous valid checkpoint and marking incomplete reports accordingly. The run command constructs and stores the complete record only after the measured campaign returns; execution errors return before that publication. RunRecord::store_probes updates optimization hints during the sweep but does not persist score-bearing completed verdicts, and production records do not trust persisted test verdicts. VerdictLog::record appends display text to gamma-progress.log, but no restart path consumes it as campaign state. The reviewed behavior therefore provides diagnostics and incremental hints, not the resumable partial campaign required by the deleted done conditions.

Consequence: An interruption during a long campaign can still discard all score-bearing progress and rerun already completed work, while deleting the backlog entry tells maintainers that the bounded-work-loss and resume capability has been completed or intentionally resolved.

Recommended action: Restore F2 with its rationale and completion criteria, adjusting the wording or linking a concrete follow-up item if needed. Remove it only after implementing atomic periodic partial records, safe validation and reuse on restart, a bounded checkpoint cadence, and deterministic interruption/resume tests. If automatic resume is intentionally out of scope and the journal plus killer-test hints are the intended contract, document that narrower decision and explicitly track or reject the remaining resume requirements.

References:

• crates/cargo-gamma/docs/DESIGN.md, reporting contract for the progress journal
• crates/cargo-gamma/README.md, cache, progress-journal, and run-record behavior
• crates/cargo-gamma-lib/src/commands/run.rs, final run-record publication
• crates/cargo-gamma-lib/src/commands/verdict_log.rs
• crates/cargo-gamma-lib/src/discover/record.rs, persisted probes and verdict trust

Impacted locations:

• crates/cargo-gamma/docs/TODO.md:8-31
• crates/cargo-gamma-lib/src/commands/run.rs:801-853
• crates/cargo-gamma-lib/src/discover/record.rs:650-674
• crates/cargo-gamma-lib/src/commands/verdict_log.rs:41-48
• crates/cargo-gamma/docs/DESIGN.md:709-715
• crates/cargo-gamma/README.md:391-414

Reviewed in: #113

---

Title: Document why the child process needs the libtest arguments

[Copilot speaking]

The subprocess test explains why environment states are arranged in a child, but the command's --exact test filter and --nocapture flag are left without their essential rationale. These are not incidental harness options: the exact filter confines the child invocation to the reporting helper, while --nocapture makes the helper's println! reach the process stdout that the parent parses.

Location: crates/cargo-gamma/src/real_host.rs:105

Reproducible reasoning: The parent reads output.stdout and searches it for the helper's present=... and absent=None markers. A passing libtest test normally captures its own output instead of forwarding it to the test process's stdout, so the markers are observable here only because --nocapture is supplied. The fully qualified filter together with --exact ensures that the self-launched test binary runs only the helper rather than any other matching test. The surrounding documentation justifies using a child process for environment isolation, but it does not explain either part of this separate output-and-selection protocol, so a maintainer cannot reproduce why these literal arguments are required from the local comments.

Consequence: A maintainer simplifying apparently redundant test-harness flags could remove --nocapture, after which the helper still passes but its markers disappear and the parent test fails, or loosen the exact selection and unintentionally execute additional tests in the child process.

Recommended action: Add an inline comment immediately above the argument list explaining that --exact isolates the reporting helper and --nocapture exposes its printed protocol to Command::output().

Impacted locations:

• crates/cargo-gamma/src/real_host.rs:105

Reviewed in: #113

---

Title: Use the established host terminology instead of “adapter”

[Copilot speaking]

The added test documentation calls RealHost an “adapter” in four places, while the module, trait, type, and surrounding comments consistently describe it as a host, host implementation, or fake. “Adapter” is not defined here and does not identify a distinct component.

Location: crates/cargo-gamma/src/real_host.rs:47

Reproducible reasoning: The terminology guideline asks that specialized terms be defined before use and that existing terminology be preferred over introducing a synonym. A search of the cargo-gamma Rust sources finds “adapter” only in these newly added comments. In each occurrence it refers to the same RealHost value that the surrounding text already calls the real host or implementation, so the new word adds no distinction for the reader.

Consequence: A reader can reasonably infer that an additional adapter layer exists between RealHost and the Host trait, then has to inspect the code to discover that the comments merely renamed the same object.

Recommended action: Replace “adapter” with RealHost, “host”, or “host implementation” consistently in the added comments. This is a comment-only terminology change, so it removes the ambiguity without requiring the subprocess test behavior to be revalidated.

Impacted locations:

• crates/cargo-gamma/src/real_host.rs:47
• crates/cargo-gamma/src/real_host.rs:90-99
• crates/cargo-gamma/src/real_host.rs:115-116

Reviewed in: #113

[sandersaares]

[Copilot speaking]

Review findings (continued 6)

Title: Compare installed --version output with the product version

[Copilot speaking]

The custom states_a_version parser accepts values that are not Semantic Versions, including leading-zero numeric core or pre-release components, and it ignores every token except the last. Output from another tool or a stale but syntactically valid version can therefore pass even though the tests claim the installed cargo-gamma executable reports its own version.

Location: crates/cargo-gamma/tests/binary.rs:46-68

Reproducible reasoning: The integration test already knows the executable package version at compile time through env!("CARGO_PKG_VERSION"). Comparing the complete trimmed output with cargo-gamma {version} verifies the product name, the exact version, the output channel, and both supported invocation shapes without maintaining a partial SemVer parser. It also exposes an unintended divergence if the CLI parser’s package version and the executable package version stop moving together, which is part of the installed product contract rather than a reason to accept arbitrary version text.

Consequence: A release can identify the executable with the wrong name or a stale/malformed version while the installed-binary contract tests still pass, and local parsing code must continue tracking SemVer corner cases.

Recommended action: Remove states_a_version, valid_identifiers, and their parser-shape test. For both process invocations, compare trimmed stdout with format!("cargo-gamma {}", env!("CARGO_PKG_VERSION")) while retaining the exit-code and stderr assertions. This uses the executable package metadata as the single expected value and eliminates the duplicated SemVer implementation.

Impacted locations:

• crates/cargo-gamma/tests/binary.rs:41-108

Follows up on: #113 (comment)

Reviewed in: #113

---

Title: Name the run entry point instead of an undefined dispatcher

[Copilot speaking]

The new module documentation twice calls the library entry point “the dispatcher,” but this term is not defined anywhere in the cargo-gamma or cargo-gamma-lib documentation or source prose. The executable actually calls the public cargo_gamma_lib::run function, which is already named in the surrounding explanation.

Location: crates/cargo-gamma/tests/binary.rs:8-9

Reproducible reasoning: The focus guidance requires specialized terms to be defined before use and recommends reusing existing terminology. Searching both affected crate trees finds “dispatcher” only in this new test file, while cargo-gamma/src/main.rs imports and calls cargo_gamma_lib::run and cargo-gamma-lib/src/lib.rs publicly re-exports that function. A reader therefore has to infer that “dispatcher” means run, and the later phrase “a direct call to the dispatcher” repeats the ambiguity. Naming run directly communicates the same distinction without introducing a separate architectural label.

Consequence: Future readers can reasonably look for a dispatcher type or API that does not exist, making the test rationale harder to connect to the production entry point it protects.

Recommended action: Replace both uses of “dispatcher” with the existing API terminology, such as “calls cargo_gamma_lib::run directly” in the module overview and “a direct call to run” in the later test documentation.

Impacted locations:

• crates/cargo-gamma/tests/binary.rs:8-9
• crates/cargo-gamma/tests/binary.rs:97-98

Reviewed in: #113

---

Title: Preserve typed error identity until the presentation boundary

[Copilot speaking]

The new failure paths across the attribute validator, discovery engine, coordinator, process supervisor, and platform layer repeatedly represent distinct conditions as strings, generic io::Error values, or a catch-all PlatformError, and several coordinator paths convert the remaining structured platform error to display text. A typed leaf error is a concrete internal error type for one failure condition; an aggregate error retains that leaf as its source. The current paths discard leaf identity before policy and tests finish using it.

Location: crates/cargo-gamma-unsafe/src/platform_error.rs:13-23

Reproducible reasoning: Validation still builds prose for const functions, empty bodies, malformed multipliers, and invalid exclude-trait-impls entries; census and reader supervision reuse io::Error; cache and containment code construct generic aggregates directly; platform failures select a coarse Situation plus free-form text. Tests consequently search rendered messages. Where PlatformError carries a situation, native cause, and origin backtrace, build and verdict code still call to_string() before wrapping it, so those fields are no longer reachable.

Consequence: Wording changes can break correctness tests while a wrongly classified failure satisfies the same substring, and platform diagnostics lose their native cause, classification, and origin trace. Internal callers cannot reliably distinguish actionable cases such as an oversized census, invalid exclusion, unsupported containment, or output-reader startup failure without parsing prose.

Recommended action: Introduce crate-private typed leaf errors for newly added failure conditions and subsystem aggregate errors that retain those leaves and their structured fields. Convert to compiler diagnostics, verdict prose, terminal-safe text, or CI text only at presentation boundaries. Carry PlatformError as a source through coordinator errors and update focused tests to assert types, fields, situations, and causes.

References:

• crates/cargo-each/src/error.rs
• crates/cargo-gamma-lib/src/error.rs
• crates/cargo-gamma-unsafe/src/platform_error.rs
• crates/cargo-gamma-unsafe/docs/design/README.md
• crates/cargo-gamma-lib/Cargo.toml
• crates/cargo-gamma-process/docs/DESIGN.md, Boundaries

Impacted locations:

• crates/cargo-gamma-attrs-impl/src/implementation.rs:337-349
• crates/cargo-gamma-attrs-impl/src/implementation.rs:623-624
• crates/cargo-gamma-attrs-impl/src/implementation.rs:761-804
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:51-56
• crates/cargo-gamma-engine/src/ops/collect/stated.rs:123
• crates/cargo-gamma-lib/src/exec/build/invoke.rs:118-123
• crates/cargo-gamma-lib/src/exec/measure.rs:1043
• crates/cargo-gamma-lib/src/exec/verdict.rs:749
• crates/cargo-gamma-lib/src/exec/verdict.rs:1891-1901
• crates/cargo-gamma-lib/src/exec/census.rs:1012-1015
• crates/cargo-gamma-lib/src/exec/census.rs:1512-1514
• crates/cargo-gamma-lib/src/exec/census.rs:1536-1538
• crates/cargo-gamma-lib/src/exec/verdict.rs:777-785
• crates/cargo-gamma-lib/src/exec/verdict.rs:1242-1267
• crates/cargo-gamma-lib/src/exec/verdict.rs:1902-1915
• crates/cargo-gamma-lib/src/exec/workspace.rs:1217-1265
• crates/cargo-gamma-lib/src/exec/workspace.rs:1284-1316
• crates/cargo-gamma-lib/src/exec/workspace.rs:1081-1089
• crates/cargo-gamma-lib/src/suppress/directive.rs:253-256
• crates/cargo-gamma-lib/src/suppress/tests.rs:650-655
• crates/cargo-gamma-process/src/process_tree.rs:81-86
• crates/cargo-gamma-unsafe/src/platform_error.rs:13-48
• crates/cargo-gamma-unsafe/src/support.rs:33-61
• crates/cargo-gamma-unsafe/src/platform_error.rs:13-23
• crates/cargo-gamma-lib/src/config.rs:270-276
• crates/cargo-gamma-lib/src/exec/build/invoke.rs:103-123
• crates/cargo-gamma-lib/src/exec/verdict.rs:280-321
• crates/cargo-gamma-lib/src/exec/workspace.rs:759-797
• crates/cargo-gamma-lib/src/exec/workspace.rs:1079-1165

Reviewed in: #113

---

Title: Audit and pin unwind safety for every changed public type

[Copilot speaking]

The change adds, exports, or changes public types in several crates without a systematic compile-time unwind-safety contract. UnwindSafe and RefUnwindSafe indicate that owned and borrowed values can cross a catch_unwind boundary without exposing broken invariants. Most new identity, schema, command, iterator, testing, process, cgroup, and situation types satisfy both traits structurally, while public error types containing unconstrained dynamic, io::Error, or PlatformError causes do not. Freshness changes additionally export SpawnedCommand, SpawnFailure, and OutputError.

Location: crates/cargo-gamma-engine/src/error.rs:95-100

Reproducible reasoning: Auto-trait behavior follows private fields, so ordinary behavior and serialization tests do not pin it. Positive assertions can cover the value types that currently satisfy both traits, including the new post-spawn typestate where its fields permit it. Engine and coordinator errors, PlatformError, SpawnFailure, and OutputError carry dynamic or operating-system error representations whose unwind traits require an explicit contract decision. The freshness additions expand the public representation graph beyond the types named in the approved review.

Consequence: A later private-field refactor can silently remove an unwind-safety trait from otherwise stable public API, and callers of affected error or launch-state types may need AssertUnwindSafe without documentation establishing whether that limitation is intentional.

Recommended action: Add one API-contract test surface per affected crate that statically asserts both unwind traits for changed public types that support them. For engine and coordinator errors, PlatformError, SpawnFailure, and OutputError, decide whether their cause representations or positive unwind safety are intended; pin the chosen positive or negative assertions and document intentional limitations. Include platform-dependent process typestates on each supported target.

References:

• Rust standard library: std::panic::UnwindSafe
• crates/cargo-gamma-engine/src/model/identity.rs
• crates/cargo-gamma-unsafe/docs/design/README.md, Boundaries

Impacted locations:

• crates/cargo-gamma-engine/src/error.rs:95-100
• crates/cargo-gamma-engine/src/lib.rs:39-40
• crates/cargo-gamma-engine/src/model/identity.rs:15-170
• crates/cargo-gamma-lib/tests/units.rs:132-169
• crates/cargo-gamma-lib/src/model/identity.rs:6
• crates/cargo-gamma-lib/src/model/mod.rs:14-15
• crates/cargo-gamma-engine/src/model/identity.rs:28
• crates/cargo-gamma-engine/src/schema.rs:256-257
• crates/cargo-gamma-lib/src/commands/mod.rs:23-26
• crates/cargo-gamma-lib/src/discover/mod.rs:35-39
• crates/cargo-gamma-lib/src/discover/record.rs:501-506
• crates/cargo-gamma-lib/src/discover/mod.rs:37
• crates/cargo-gamma-lib/src/error.rs:16-29
• crates/cargo-gamma-lib/src/error.rs:29
• crates/cargo-gamma-lib/src/testing.rs:48-52
• crates/cargo-gamma-unsafe/src/cgroup.rs:572-590
• crates/cargo-gamma-unsafe/src/cgroup.rs:589
• crates/cargo-gamma-unsafe/src/platform_error.rs:23-28
• crates/cargo-gamma-unsafe/src/situation.rs:13-33
• crates/cargo-gamma-lib/src/config.rs:56-57
• crates/cargo-gamma-process/src/lib.rs:84
• crates/cargo-gamma-process/src/process_tree.rs:241-307
• crates/cargo-gamma-process/src/process_tree.rs:309-408

Reviewed in: #113

---

Title: Replace wall-clock test oracles with deterministic synchronization

[Copilot speaking]

The new cache-lock, output-reader, progress-encoding, process-containment, and Windows job tests depend on real clock reads, sleeps, polling deadlines, production run timeouts, or test-local watchdogs. A wall-clock oracle is a test decision that succeeds or fails because elapsed time crossed a threshold; a watchdog is only a last-resort guard against a hung test and must not be the semantic assertion or the mechanism that kills a mutation.

Location: crates/cargo-gamma-process/src/process_tree.rs:1857

Reproducible reasoning: The cache pause waits on a two-minute receive deadline if the phase is not reached; the reader-thread fault can block on a full pipe until the production run timeout; progress tests call Instant::now() even though their state is already unthrottled; escaped-process and job tests infer termination from fixed waits or polling deadlines. Separate process-tree wrappers also convert hangs into ordinary one-minute test failures during mutation runs. In each case the behavior under test has a direct state transition—lock ownership, phase arrival, pipe closure, process exit, job reachability, or progress state—that can be observed or controlled without measuring elapsed time.

Consequence: Failures and mutations can consume the full deadline, loaded hosts can produce false results, deterministic tests acquire unnecessary platform-clock dependencies, and mutation results can claim a semantic kill when only a test watchdog fired.

Recommended action: Expose deterministic synchronization at the controllable boundary: use injected phase callbacks or fake lock/platform backends for cache and job transitions, pipe or process-handle completion for containment, and a pipe-safe fixture or controllable reader-spawn abstraction for output supervision. Keep production behavior identical in test-enabled builds for the same inputs, with injected collaborators providing observation and control rather than changing orchestration. Remove the unnecessary progress-test clock reads. Keep any last-chance watchdog separate from the assertion, far above expected duration, and bypass it during mutation testing so campaign supervision—not the test deadline—classifies a hang.

References:

• gamma.toml

Impacted locations:

• crates/cargo-gamma-lib/src/commands/run.rs:798-800
• crates/cargo-gamma-lib/src/testing.rs:54-59
• crates/cargo-gamma-lib/tests/session.rs:378-426
• crates/cargo-gamma-lib/src/exec/verdict.rs:1197
• crates/cargo-gamma-lib/src/exec/verdict.rs:1906
• crates/cargo-gamma-lib/src/exec/verdict.rs:2469
• crates/cargo-gamma-lib/src/exec/verdict.rs:1939-1947
• crates/cargo-gamma-lib/src/report/progress.rs:1363-1370
• crates/cargo-gamma-lib/src/report/progress.rs:1406-1416
• crates/cargo-gamma-process/src/process_tree.rs:1857
• crates/cargo-gamma-process/src/process_tree.rs:2393-2395
• crates/cargo-gamma-process/src/process_tree.rs:2512-2514
• crates/cargo-gamma-unsafe/src/job.rs:937-960
• crates/cargo-gamma-unsafe/src/job.rs:1011-1030
• crates/cargo-gamma-unsafe/src/job.rs:960
• crates/cargo-gamma-process/src/process_tree.rs:1828-1861

Reviewed in: #113

---

Title: Make progress explicit in every new open-ended loop

[Copilot speaking]

Several new position, text-encoding, SARIF shrinking, and record-iteration loops terminate only because a mutable cursor, length, or iterator field is assigned in the body. The default stmt.delete_assign mutation removes plain and compound assignment statements, so these loops can revisit the same state forever when Gamma mutation-tests its own code.

Location: crates/cargo-gamma-engine/src/text.rs:71

Reproducible reasoning: Deleting line += 1 leaves the position sweep on the same line; deleting any encoder cursor update reprocesses the same byte; deleting length /= 2 rebuilds the same oversized SARIF document; deleting the assignment that advances Entries.files leaves mutants unset and the file iterator untouched. Existing tests reach these paths, but without an independent progress check they hang instead of rejecting the mutation. The common root cause is that loop termination and the mutation site are the same statement.

Consequence: Self-hosted mutation campaigns can stall until external supervision intervenes, and an encoder branch can continue allocating while making no semantic progress.

Recommended action: For each listed loop, assert a monotonic progress invariant that remains when the assignment is deleted—for example, require line or cursor advancement, a decreasing candidate length, or a shrinking remaining-file count before the next iteration. A bounded iterator or state-machine rewrite is also suitable when it makes revisiting the same state unrepresentable. Keep the guard in ordinary production logic and retain tests that execute every affected branch.

References:

• crates/cargo-gamma/docs/MUTATORS.md, stmt and mutator catalog sections
• crates/cargo-gamma/docs/MUTATORS.md, stmt.delete_assign
• crates/cargo-gamma/docs/MUTATORS.md, stmt documents default assignment-statement deletion

Impacted locations:

• crates/cargo-gamma-engine/src/schema.rs:115-119
• crates/cargo-gamma-engine/src/schema.rs:116
• crates/cargo-gamma-engine/src/text.rs:71-116
• crates/cargo-gamma-engine/src/text.rs:155-164
• crates/cargo-gamma-lib/src/ci/sarif.rs:151-164
• crates/cargo-gamma-engine/src/text.rs:71
• crates/cargo-gamma-lib/src/discover/record.rs:511-520
• crates/cargo-gamma-lib/src/discover/record.rs:519

Reviewed in: #113

---

Title: Define and enforce one cfg-gated stated-value boundary

[Copilot speaking]

The fused discovery pre-pass does not have one consistent contract or implementation for configuration-gated source. A cfg gate is an attribute such as #[cfg(...)] that removes a syntax node from the selected build; stated-value validation audits #[gamma::value(...)] attributes. The design alternately says validation covers active source and the whole file, the fused visitor omits a field-level gate that the indexer applies, and the new equivalence tests do not effectively exercise every independently implemented gate.

Location: crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:111-149

Reproducible reasoning: Standalone check_stated has no CfgSet and therefore walks the whole file, while workspace discovery uses the fused cfg-aware path. Within that fused path, item, associated-item, statement, and expression visitors each call skip_gate, but fields are reached through default traversal and audited even when their own cfg is false. Tests cover only some visitor levels, and the inactive-statement fixture leaks numeric evidence that no active candidate consumes, so removing the intended gate leaves its comparison unchanged. The split documentation, duplicated visitor checks, and non-observable fixture all stem from the absence of one named semantic boundary and one test oracle for it.

Consequence: Configured-out fields can still abort a campaign, inactive evidence can affect candidates after a future gate regression, and users cannot tell whether listing is intended to validate inactive and test-only stated values.

Recommended action: Choose and document the boundary for standalone validation, fused discovery, and mutant listing. Implement the fused boundary through a shared gating abstraction that covers every attribute-bearing syntax node before either audit or indexing sees it, including fields. Add focused fixtures for items, impl and trait members, fields, statements, and expressions in which leaked evidence or a malformed stated value changes the observable result, so each gate fails independently when bypassed.

References:

• crates/cargo-gamma-engine/docs/DESIGN.md
• crates/cargo-gamma/docs/MUTATORS.md
• crates/cargo-gamma-engine/src/ops/collect/traversal.rs
• crates/cargo-gamma-lib/src/discover/survey.rs

Impacted locations:

• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:23-29
• crates/cargo-gamma-engine/src/ops/collect/traversal.rs:54-60
• crates/cargo-gamma-engine/docs/DESIGN.md:23-30
• crates/cargo-gamma/docs/MUTATORS.md:863-870
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:111-149
• crates/cargo-gamma-engine/src/ops/collect/collector/indexes.rs:176-188
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3411-3508
• crates/cargo-gamma-engine/src/ops/collect/collector/phase_one.rs:117-150
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3485-3508
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3490-3507

Reviewed in: #113

---

Title: Keep test and implementation helpers outside the public API

[Copilot speaking]

The change repeatedly uses unrestricted pub for helpers whose only consumers are the defining crate, sibling modules, unit tests, or cross-crate agreement tests. Rust public API includes every externally reachable pub item; #[doc(hidden)] only hides navigation, and a private parent or optional internals feature does not change the declaration’s stated visibility.

Location: crates/cargo-gamma-attrs-impl/src/lib.rs:49-50

Reproducible reasoning: Repository-wide call sites show that the nesting scanner and allocation probe exist for tests, skip_gate, schema conversions, and Progress::relay need at most crate visibility, the viewer-version constant is a unit-test fixture, and corpus generators are confined to one integration-test crate. None supplies a supported consumer capability. The repeated pattern indicates test plumbing and internal coordination are being exposed instead of placed at the narrowest boundary that still permits their actual callers.

Consequence: Downstream or feature-enabled users can couple to parser limits, storage representation, conversion details, output routing, or fixture data, turning internal refactors into apparent API breaks and making generated API surfaces harder to understand.

Recommended action: Narrow sibling-module helpers to pub(crate), keep same-module helpers private, and compile fixture-only constants under cfg(test). Move the cross-crate nesting parity check into unit-test code with a development-only dependency, or provide a deliberately unsupported dev-only test abstraction rather than a root public re-export. Apply the same narrow visibility to every corpus generator.

References:

• crates/cargo-gamma-attrs-impl/docs/DESIGN.md
• crates/cargo-gamma-lib/tests/agreement.rs
• crates/cargo-gamma-engine/docs/DESIGN.md, Stability

Impacted locations:

• crates/cargo-gamma-attrs-impl/src/implementation.rs:149-155
• crates/cargo-gamma-attrs-impl/src/lib.rs:49-50
• crates/cargo-gamma-engine/src/cfg.rs:160-175
• crates/cargo-gamma-engine/src/cfg.rs:171
• crates/cargo-gamma-engine/src/model/identity.rs:47-54
• crates/cargo-gamma-engine/src/schema.rs:192
• crates/cargo-gamma-engine/src/schema.rs:268
• crates/cargo-gamma-lib/src/html.rs:36
• crates/cargo-gamma-lib/src/html.rs:227-234
• crates/cargo-gamma-lib/src/report/progress.rs:363-365
• crates/cargo-gamma-lib/tests/agreement.rs:83
• crates/cargo-gamma-lib/tests/agreement.rs:88
• crates/cargo-gamma-lib/tests/agreement.rs:93
• crates/cargo-gamma-lib/tests/agreement.rs:98
• crates/cargo-gamma-lib/tests/agreement.rs:103
• crates/cargo-gamma-lib/tests/agreement.rs:108
• crates/cargo-gamma-lib/tests/agreement.rs:119
• crates/cargo-gamma-lib/tests/agreement.rs:130

Reviewed in: #113

---

Title: Separate user-visible contracts from implementation and test mechanics

[Copilot speaking]

The expanded design documents, crate overviews, and public rustdoc across the engine, coordinator, process, platform, and CLI crates repeatedly describe replaceable internal mechanics: fused visitor topology and allocations, conditional module facades, cgroup descriptor choreography, prepared-command types, cache digest representation, fixed buffers, native-call fault injection, and test isolation. A design contract states behavior users may rely on; an implementation guide records how the code and tests realize that behavior.

Location: crates/cargo-gamma/docs/DESIGN.md:801-813

Reproducible reasoning: The durable contracts are cfg-aware discovery results, a deliberately bounded public facade, sealed or best-effort containment, structured platform failure classification, stable cache ownership, bounded census handling, and explicit capability reporting. The named visitors, macros, data structures, descriptor operations, digest truncation, buffer layout, and fixture architecture can all change while preserving those results. Each affected crate lacks the implementation guide needed to hold this high-level internal reasoning, so replaceable choices have accumulated in documents readers treat as API or product promises.

Consequence: Maintainers must reverse-engineer interacting implementation constraints from source while routine refactors appear to change supported behavior, and public-facing documentation can become stale even when the contract remains intact.

Recommended action: Add implementation guides for the affected engine, coordinator, process, platform, and CLI crates, covering only their relevant internal architecture and test strategy. Move the listed traversal, facade, descriptor, ownership-type, cache-representation, buffer, fault-injection, and fixture mechanics there. Keep design documents and public rustdoc focused on observable inputs, outputs, lifecycle states, ownership obligations, guarantees, and error categories; retain local comments only where they justify a nearby decision.

References:

• crates/cargo-gamma-engine/docs/DESIGN.md, lines 23-30
• crates/cargo-gamma-lib/docs/design/README.md

Impacted locations:

• crates/cargo-gamma-engine/docs/DESIGN.md:23-33
• crates/cargo-gamma-engine/src/cfg.rs:164-169
• crates/cargo-gamma-engine/src/ops/collect/collector.rs:268-284
• crates/cargo-gamma-engine/src/ops/collect/collector.rs:1023-1026
• crates/cargo-gamma-engine/docs/DESIGN.md:23-30
• crates/cargo-gamma-engine/src/ops/collect/collector.rs:268
• crates/cargo-gamma-lib/src/lib.rs:125-134
• crates/cargo-gamma-lib/src/lib.rs:257-282
• crates/cargo-gamma-process/src/process_tree.rs:44-49
• crates/cargo-gamma-process/src/process_tree.rs:446-463
• crates/cargo-gamma-unsafe/docs/design/README.md:27-30
• crates/cargo-gamma-unsafe/docs/design/README.md:42-44
• crates/cargo-gamma/docs/DESIGN.md:305-308
• crates/cargo-gamma/docs/DESIGN.md:361-372
• crates/cargo-gamma/docs/DESIGN.md:496-509
• crates/cargo-gamma/docs/DESIGN.md:566-581
• crates/cargo-gamma/docs/DESIGN.md:801-813
• crates/cargo-gamma-process/docs/DESIGN.md:28-41
• crates/cargo-gamma-process/src/lib.rs:52-58
• crates/cargo-gamma-process/src/process_tree.rs:225-264

Reviewed in: #113

---

Title: Represent the cgroup interrupt watch as one owned lifetime

[Copilot speaking]

The Linux interrupt registry combines a process-group identifier and a cgroup kill descriptor in one slot, while Cgroup stores only one copyable (slot, group) reminder. The cgroup watch is the registration that lets a terminal interrupt write to the already-open cgroup kill descriptor. It must be registered once and remain live until the owning Cgroup closes that descriptor, independently of when the leader process group is released.

Location: crates/cargo-gamma-unsafe/src/interrupt.rs:479-499

Reproducible reasoning: watch_cgroup can safely be called twice on the same Cgroup: the second call claims another slot and watched_at overwrites the first reminder, leaving the first raw descriptor registration unreleasable. Separately, ordinary forget clears the entire packed slot when the leader is reaped, removing the cgroup descriptor before Cgroup::drop; the later drop then finds no registration to discharge. Both failures come from treating two lifetimes as one packed, externally indexed value rather than an owned one-shot cgroup registration.

Consequence: A duplicate registration can retain a closed descriptor and later target an unrelated reused file descriptor, while an early process-group release can remove interruption coverage while descendants are still being drained or accounted for.

Recommended action: Create a non-copying RAII cgroup-watch guard owned by Cgroup and make registration a one-time state transition. Give the cgroup descriptor its own registry lifetime or descriptor-only state so releasing the numeric process group does not retract it; let the guard’s drop wait for active sweeps and remove the descriptor before the file closes. Add regressions for duplicate registration, process-group release followed by interruption, and final cgroup drop without disturbing a replacement slot.

References:

• crates/cargo-gamma-unsafe/docs/design/README.md:27-30
• crates/cargo-gamma-unsafe/src/cgroup.rs
• crates/cargo-gamma-unsafe/docs/design/README.md, Boundaries
• crates/cargo-gamma-process/docs/DESIGN.md, lifecycle boundary

Impacted locations:

• crates/cargo-gamma-unsafe/docs/design/README.md:27-30
• crates/cargo-gamma-unsafe/src/cgroup.rs:583-602
• crates/cargo-gamma-unsafe/src/cgroup.rs:778-784
• crates/cargo-gamma-unsafe/src/cgroup.rs:823-832
• crates/cargo-gamma-unsafe/src/interrupt.rs:489-500
• crates/cargo-gamma-unsafe/src/cgroup.rs:783-784
• crates/cargo-gamma-unsafe/src/interrupt.rs:212-229
• crates/cargo-gamma-unsafe/src/interrupt.rs:557-568
• crates/cargo-gamma-process/src/process_tree.rs:1001-1007
• crates/cargo-gamma-unsafe/src/interrupt.rs:479-499

Reviewed in: #113

---

Title: Base cache and lock identity on a stable filesystem workspace identity

[Copilot speaking]

The default cache name is a BLAKE3 digest of the absolute workspace-path spelling, and that name also selects the lock domain for source-changing commands. On a filesystem where case variants name the same directory, the current identity function hashes those aliases differently instead of making them converge on one physical workspace identity.

Location: crates/cargo-gamma/docs/DESIGN.md:361-369

Reproducible reasoning: gamma_base first makes the supplied path absolute and workspace_identity hashes root.as_str().as_bytes() without resolving case aliases. The reviewed checkout's filesystem accepts both C:\source\reviews\ox-tools-pr-113 and its uppercase spelling as the same directory while preserving the different spelling passed to the hash. The two invocations therefore derive different cache names and take independent locks for one checkout.

Consequence: Commands targeting one physical workspace through different case spellings can concurrently copy or modify it while each believes it owns the only workspace lock, and they can create duplicate persistent caches for the same checkout.

Recommended action: Derive the lock key from a stable identity for the actual filesystem object, or from a canonical representation whose alias behavior is probed for that directory, rather than hashing the caller's path spelling directly. Keep the ownership marker as collision defense, but make aliases of the same root reach the same lock before collision handling.

References:

• crates/cargo-gamma/docs/DESIGN.md — The scratch workspace
• crates/cargo-gamma-lib/src/exec/workspace.rs — gamma_base and workspace_identity

Impacted locations:

• crates/cargo-gamma-lib/src/exec/workspace.rs:699-742
• crates/cargo-gamma-lib/src/exec/workspace.rs:1382-1392
• crates/cargo-gamma/docs/DESIGN.md:356-369

Reviewed in: #113

---

Title: Describe current contracts instead of change history or review status

[Copilot speaking]

New comments, rustdoc, design text, and test rationales across the change repeatedly explain the code through former implementations, release-to-release comparisons, temporal markers such as “now”, “once”, and “remains outstanding”, or indirect CI metaphors such as tests staying green. Present-state documentation states what the current code guarantees and why, without requiring readers to reconstruct an earlier version or unfinished review work.

Location: crates/cargo-gamma-attrs-impl/src/implementation.rs:581-591

Reproducible reasoning: The parser, identity, cfg defaults, cache cleanup, SARIF selection, learned-killer, reader, HTML viewer, lineage, directive, agreement-test, containment, runtime, and report-ordering documentation all contain historical or status narrative even though the corresponding invariant can be stated directly. Several passages also leave prior concepts undefined in the current source. Rewriting each around its current input, output, invariant, or rationale preserves the useful technical explanation while removing facts that become stale independently of the code.

Consequence: Readers must reverse-engineer the current contract from a migration story, obsolete concepts remain in otherwise current API documentation, and status statements can survive after the described work is complete or abandoned.

Recommended action: Rewrite the listed passages in present tense around the current parsing, identity, test, cache, SARIF, scheduling, reader, viewer, lineage, directive, containment, runtime, and ordering invariants. State test outcomes directly, remove comparisons with previous implementations or releases and outstanding-work notes, and place any genuinely needed migration history in a changelog or dedicated compatibility document.

References:

• crates/cargo-gamma-attrs-impl/docs/DESIGN.md
• crates/cargo-gamma/README.md, HTML report description
• crates/cargo-gamma/docs/DESIGN.md, Identity and knowledge across campaigns
• crates/cargo-gamma-process/docs/DESIGN.md, Boundaries

Impacted locations:

• crates/cargo-gamma-attrs-impl/src/implementation.rs:581-591
• crates/cargo-gamma-attrs-impl/src/implementation.rs:1537-1540
• crates/cargo-gamma-attrs/tests/consumer.rs:6-10
• crates/cargo-gamma-attrs/tests/diagnostics.rs:95-101
• crates/cargo-gamma-attrs/tests/consumer.rs:10
• crates/cargo-gamma-engine/src/model/identity.rs:17-25
• crates/cargo-gamma-engine/src/model/identity.rs:189-193
• crates/cargo-gamma-engine/src/ops/collect/defaults.rs:29-33
• crates/cargo-gamma-lib/src/ci/sarif.rs:147-148
• crates/cargo-gamma-lib/src/exec/sweep.rs:2669-2676
• crates/cargo-gamma-lib/src/exec/verdict.rs:1213-1217
• crates/cargo-gamma-lib/src/html.rs:13-19
• crates/cargo-gamma-lib/src/merge/union.rs:419-430
• crates/cargo-gamma-lib/src/merge/union.rs:421-430
• crates/cargo-gamma-lib/src/suppress/tests.rs:634-637
• crates/cargo-gamma-lib/tests/agreement.rs:321-330
• crates/cargo-gamma-lib/tests/docs.rs:685-689
• crates/cargo-gamma-lib/tests/units.rs:132-153
• crates/cargo-gamma-lib/tests/units.rs:132-135
• crates/cargo-gamma-process/src/process_tree.rs:127-128
• crates/cargo-gamma-process/src/process_tree.rs:950-952
• crates/cargo-gamma-process/src/process_tree.rs:1869-1871
• crates/cargo-gamma-rt/src/runtime.rs:261-262
• crates/cargo-gamma-rt/src/runtime.rs:299-302
• crates/cargo-gamma-rt/src/runtime.rs:471-478
• crates/cargo-gamma-rt/src/runtime.rs:1210-1214
• crates/cargo-gamma/docs/DESIGN.md:720-726
• crates/cargo-gamma/docs/DESIGN.md:408-412

Reviewed in: #113

---

Title: Introduce enumerations without maintaining their item counts in prose

[Copilot speaking]

The change repeatedly states the number of fields, variants, fixtures, failure cases, outcomes, or suppression channels immediately before or beside the declaration or list that already defines them. The suppression guide already demonstrates the drift risk: its prose still says five channels while the expanded table contains six.

Location: crates/cargo-gamma/src/main.rs:791

Reproducible reasoning: Counts add no semantic information when the reader can see the fields, enum variants, array entries, or following list. They create a second value that must be updated whenever the structure changes, and several comments use the count instead of naming why the cases matter. The same maintenance pattern appears in engine errors and tests, sweep recovery, agreement and SARIF fixtures, runtime startup cases, runtime enums, the README, and the main design document.

Consequence: Documentation can contradict the adjacent code or table, and readers learn a quantity rather than the distinctions or coverage rationale that make the cases useful.

Recommended action: Replace numeric introductions with blanket statements and name the semantic roles when emphasis is useful. Describe why a struct uses named fields, which syntax roles fixtures cover, what invariant recovery preserves, and which representative SARIF cases matter, while leaving declarations and lists as the sole source of their current size.

References:

• crates/cargo-gamma-lib/src/ci/finding.rs

Impacted locations:

• crates/cargo-gamma-engine/src/error.rs:89-93
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3515-3516
• crates/cargo-gamma-engine/src/ops/collect/tests.rs:3540-3542
• crates/cargo-gamma-lib/src/exec/sweep.rs:818-819
• crates/cargo-gamma-lib/tests/agreement.rs:26-31
• crates/cargo-gamma-lib/tests/agreement.rs:182-194
• crates/cargo-gamma-lib/tests/sarif_contract.rs:36-47
• crates/cargo-gamma-lib/src/ci/finding.rs:54-66
• crates/cargo-gamma-lib/tests/sarif_contract.rs:36-37
• crates/cargo-gamma-rt/src/lib.rs:121-122
• crates/cargo-gamma-rt/src/runtime.rs:556-557
• crates/cargo-gamma-rt/src/runtime.rs:2431-2434
• crates/cargo-gamma/docs/DESIGN.md:382-388
• crates/cargo-gamma/docs/DESIGN.md:548-558
• crates/cargo-gamma/docs/DESIGN.md:720-726
• crates/cargo-gamma/README.md:793-811
• crates/cargo-gamma/src/main.rs:784-802

Reviewed in: #113

---

Title: Use an owned temporary directory for the overlapped-stderr test

[Copilot speaking]

The new Windows regression builds a filename from the process ID and SystemTime::now() in the shared operating-system temporary directory. The clock is unrelated to the behavior under test, and uniqueness and cleanup are managed manually even though the test only needs a private path on which it can set FILE_FLAG_OVERLAPPED.

Location: crates/cargo-gamma-rt/src/runtime.rs:2910-2914

Reproducible reasoning: Test code can obtain deterministic resource isolation without consulting a real-time clock. duration_since(UNIX_EPOCH).expect(...) adds an avoidable host-clock precondition, while a PID/timestamp name can collide with a stale file left by an interrupted earlier run after PID reuse or clock adjustment. A tempfile::TempDir already provides an owned unique namespace while preserving the ability to open a fixed child path with OpenOptionsExt::custom_flags.

Consequence: The regression can fail before exercising the overlapped writer because of host clock state or a stale filename, and a failure before the explicit removal leaves a shared-temp artifact that can affect later runs.

Recommended action: Create a tempfile::TempDir, open a fixed file such as stderr inside it with the existing create_new, write, and FILE_FLAG_OVERLAPPED options, then read the file after the child exits and let the temporary-directory guard clean it up. Remove the SystemTime and process-ID nonce construction.

Impacted locations:

• crates/cargo-gamma-rt/src/runtime.rs:2910-2919

Reviewed in: #113

[sandersaares]

[Copilot speaking]

Superseded by #134, #135, #136, #137, #138, #139, which split the approved review findings into issue-sized parts.