fix(cargo-gamma): harden discovery and process cleanup - #147
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It includes release/versioning updates that don’t follow the documented release workflow and has documentation mismatches (e.g., README/docs comments not aligned with the new behavior/version).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens cargo-gamma discovery inputs and report generation against stale/oversized artifacts, strengthens process-tree cleanup error propagation, and aligns proc-macro nesting limits with the engine while updating associated design/docs.
Changes:
- Add bounded input reading for diffs and discovery/optimization artifacts; tighten diff-path resolution to workspace-safe candidates.
- Refuse report construction when source generation no longer matches the digests captured at discovery time.
- Propagate process-tree cleanup failures more consistently and align proc-macro nesting/chain limits with the engine (with new agreement tests).
File summaries
| File | Description |
|---|---|
| crates/cargo-gamma/docs/TODO.md | Adds new tracked TODO items for perf/doc/testing follow-ups. |
| crates/cargo-gamma/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-unsafe/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-rt/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-process/src/process_tree.rs | Propagates/threads cleanup errors through adoption/termination/observation paths. |
| crates/cargo-gamma-process/README.md | Expands crate-level documentation of process-tree lifecycle. |
| crates/cargo-gamma-process/docs/DESIGN.md | Updates design notes to reflect propagated sweep failures and reader-thread behavior. |
| crates/cargo-gamma-process/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-lib/tests/agreement.rs | Adds nesting-guard agreement cases for mixed operator/postfix/cast chains. |
| crates/cargo-gamma-lib/src/elements/report.rs | Refuses combining verdicts with source that changed post-discovery (digest check) + test. |
| crates/cargo-gamma-lib/src/discover/record.rs | Uses bounded input reads for persisted run records. |
| crates/cargo-gamma-lib/src/discover/mod.rs | Wires in new bounded-input helper module. |
| crates/cargo-gamma-lib/src/discover/input.rs | Implements bounded UTF-8 reads for discovery inputs/artifacts. |
| crates/cargo-gamma-lib/src/discover/hints.rs | Uses bounded reads for hints; rejects/handles oversized promotion inputs. |
| crates/cargo-gamma-lib/src/discover/diff.rs | Bounds diff reads; constrains diff path resolution and rejects traversal. |
| crates/cargo-gamma-lib/docs/design/README.md | Documents bounded reads, safe diff path resolution, and report source-generation checks. |
| crates/cargo-gamma-lib/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-engine/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-attrs/Cargo.toml | Bumps crate version to 0.2.0. |
| crates/cargo-gamma-attrs-impl/src/implementation.rs | Unifies expression-path chain accounting to prevent mixed-syntax limit evasion. |
| crates/cargo-gamma-attrs-impl/README.md | Expands crate-level documentation and stability guidance. |
| crates/cargo-gamma-attrs-impl/docs/DESIGN.md | Updates design to reflect combined chain-budget behavior. |
| crates/cargo-gamma-attrs-impl/Cargo.toml | Bumps crate version to 0.2.0. |
| Cargo.toml | Updates workspace-local dependency versions for the cargo-gamma family. |
| Cargo.lock | Updates locked versions for the cargo-gamma family. |
Review details
Suppressed comments (1)
crates/cargo-gamma-attrs-impl/README.md:62
- This README still links to the 0.1.0 docs.rs page for
cargo-gamma-attrs-impl, but the crate version is now 0.2.0.
[__link2]: https://docs.rs/cargo-gamma-attrs-impl/0.1.0/cargo_gamma_attrs_impl/?search=value
- Files reviewed: 24/25 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
af8755b to
a08da19
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It includes a release-style version bump that conflicts with the repo’s documented release workflow and it leaves newly added docs.rs intra-doc links pointing at the old 0.1.0 version.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Cargo.toml:41
- The workspace dependency versions for the cargo-gamma crate family are being bumped in the same PR as functional changes, but the repo’s documented release workflow expects version bumps to be done via
scripts/release-crate.ps1in a dedicated release PR after the desired changes are already onmain(see README.md “Publishing Crates”). Mixing the two makes it harder to cherry-pick fixes and to follow the release automation process.
cargo-gamma-attrs-impl = { path = "crates/cargo-gamma-attrs-impl", default-features = false, version = "0.2.0" }
cargo-gamma-engine = { path = "crates/cargo-gamma-engine", default-features = false, version = "0.2.0" }
cargo-gamma-lib = { path = "crates/cargo-gamma-lib", default-features = false, version = "0.2.0" }
cargo-gamma-process = { path = "crates/cargo-gamma-process", default-features = false, version = "0.2.0" }
cargo-gamma-rt = { path = "crates/cargo-gamma-rt", default-features = false, version = "0.2.0" }
crates/cargo-gamma-process/README.md:90
- These docs.rs intra-doc links still point at version 0.1.0 even though the crate version was bumped to 0.2.0 in this PR. That will send readers to the wrong API docs.
[__link1]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=MemoryRequest
[__link10]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=SpawnFailure
[__link11]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=PreparedCommand::backoff
[__link12]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=ProcessTree::adopt
[__link13]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=output
crates/cargo-gamma-process/README.md:99
- These docs.rs intra-doc links still point at version 0.1.0 even though the crate version was bumped to 0.2.0 in this PR. That will send readers to the wrong API docs.
[__link7]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=prepare
[__link8]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=PreparedCommand
[__link9]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=SpawnedCommand
crates/cargo-gamma-process/src/process_tree.rs:1222
killnow returnsio::Result<()>and will propagate sweep/kill failures, but the doc comment still implies it is purely best-effort when the subtree cannot be reached. Updating the docs will help callers and future maintainers understand that partial cleanup is surfaced as an error even though the child kill is still attempted.
/// Kills the child and every process descended from it.
///
/// Falls back to killing the child alone whenever the subtree cannot be reached, because a run
/// that cut off one process is still better than one that cut off none.
crates/cargo-gamma-process/README.md:95
- These docs.rs intra-doc links still point at version 0.1.0 even though the crate version was bumped to 0.2.0 in this PR. That will send readers to the wrong API docs.
[__link2]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=prepare
[__link3]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=ProcessTree::usage
[__link4]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=containment
[__link5]: https://docs.rs/cargo-gamma-process/0.1.0/cargo_gamma_process/?search=ProcessTree::sealed
- Files reviewed: 24/25 changed files
- Comments generated: 2
- Review effort level: Lite
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (97.5%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
=======================================
- Coverage 97.6% 97.5% -0.1%
=======================================
Files 298 299 +1
Lines 67348 67767 +419
=======================================
+ Hits 65763 66130 +367
- Misses 1585 1637 +52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a08da19 to
fd704c0
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive diff path resolution and cross-platform process-tree cleanup semantics, which warrants final human review of edge cases.
Review details
- Files reviewed: 36/37 changed files
- Comments generated: 0 new
- Review effort level: Lite
fd704c0 to
2ac2984
Compare
2ac2984 to
9e96948
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Error paths in ProcessTree::wait_with_output can discard termination/cleanup failures, reducing debuggability and potentially hiding operationally important cleanup errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
crates/cargo-gamma-process/src/process_tree.rs:965
- If
wait_for_outputfails andterminate()also fails, the current code returns only thewait_for_outputerror and discards the cleanup failure. That can make diagnosing failed process-tree cleanup harder; include the cleanup failure (or prefer it) when termination fails.
Err(cause) => {
if self.child.is_none() || self.terminate().is_err() {
// Cleanup could not prove that descendants released their pipe handles.
// Dropping a JoinHandle detaches the reader rather than blocking this error.
return Err(cause);
}
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Lite
9e96948 to
26179aa
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The cargo-gamma-lib design doc claims absolute paths outside the workspace are never accepted, but the implementation intentionally resolves them by unique suffix to workspace candidates, so the documented contract should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Lite
26179aa to
45059cd
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, well-covered by targeted unit tests in the touched areas, and the error/cleanup semantics are documented and implemented consistently.
Review details
- Files reviewed: 37/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It includes broad cross-crate behavioral changes in process cleanup/discovery/reporting plus release automation/versioning updates that warrant final human validation.
Review details
- Files reviewed: 48/49 changed files
- Comments generated: 0 new
- Review effort level: Lite
35d7013 to
779fe48
Compare
779fe48 to
e85fc9b
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent across code, tests, and design docs, and no concrete correctness or contract issues were found in the reviewed diffs.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
e85fc9b to
41e42ef
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad correctness/operational changes across process lifecycle, discovery/reporting integrity, and release artifacts, and one remaining cleanup-error propagation gap warrants human confirmation.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
ProcessTree output detachment can still re-retain bytes after discard due to a load-before-lock race in output_reader, undermining the “release before detachment” guarantee.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
Only a minor warning-message wording issue remains; the substantive hardening and release changes look internally consistent in the reviewed diffs.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It spans multiple crates and critical lifecycle paths (process containment/termination, discovery input hardening, vendored runtime packaging, and release automation), so it warrants final human review despite no specific defects found in the reviewed diffs.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Cgroup::kill should retry write(2) on EINTR to avoid spurious cleanup/termination failures now that kill errors are propagated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently harden discovery/process cleanup/report generation with accompanying tests and aligned design/docs, and no concrete defects were found in the reviewed diffs.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The PR makes broad, cross-platform lifecycle and packaging changes across multiple crates (notably process containment/termination), which warrants final human review despite the added tests and documentation.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent (docs/tests match behavior) and the prior hardening feedback threads appear fully addressed in the current diff.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
OutputReader’s manual Read::read loop no longer retries io::ErrorKind::Interrupted, which can cause spurious output-capture failures under signal interruption.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 1
- Review effort level: Lite
- restrict diff paths to workspace candidates and bound artifact reads - reject reports when source changes after discovery - align proc-macro nesting limits with engine behavior - propagate process-tree cleanup and reap failures safely - embed runtime assets through cargo-gamma-rt so crates.io installs build - release all cargo-gamma crates at 0.2.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ca9796b-96b2-41c1-b138-4ecbb21aac02
There was a problem hiding this comment.
🔵 Needs a closer look
It combines a coordinated multi-crate release with substantial cross-platform process-lifecycle and discovery hardening changes that warrant final human verification.
Review details
- Files reviewed: 49/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
cargo-gamma-rt, fixingcargo install cargo-gammafrom crates.ioValidation
cargo-gamma-libandcargo-gammabuilds completed successfully with local patches for the unpublished 0.2.0 sibling crates