test: isolate Trace Commons coverage from local state - #6714
ogarciarevett wants to merge 5 commits into
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
size: XS
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTrace Commons coverage tests now execute in isolated child processes with a temporary ChangesTrace Commons coverage isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 0 | 0 | 8fec1561cfe0 |
Head: 8fec1561cfe00d467f4509e22f30d100fb66e038
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Approved after focused review of the test-only isolation change; no actionable regressions found.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/reborn_trace_first_party_tool_coverage.rs`:
- Around line 81-97: Replace the OnceLock-based process-global setup in
setup_trace_commons_base_dir with an isolated approach that does not mutate
IRONCLAW_BASE_DIR during tests: configure the variable before the test process
starts or pass an explicit base-directory path through the Trace Commons
harness. Ensure compute_ironclaw_base_dir and all related tests use that
injected/configured path without relying on test ordering or unsynchronized
environment access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a666ae20-db2d-4cc1-9077-fde51e52fdff
📒 Files selected for processing (1)
tests/reborn_trace_first_party_tool_coverage.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/reborn_trace_first_party_tool_coverage.rs`:
- Around line 84-87: Update the child-detection logic in the helper around
TRACE_COMMONS_TEST_CHILD so an inherited sentinel alone cannot bypass spawning.
Validate that the marker matches the freshly created temporary path or another
per-invocation value established by this helper, and only then use the inherited
IRONCLAW_BASE_DIR; otherwise continue through the normal spawn path without
panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22e67e13-3749-4c87-9058-a61e4f41fd1d
📒 Files selected for processing (1)
tests/reborn_trace_first_party_tool_coverage.rs
|
@ironloopai review |
|
Caution Command not authorized Only repository owners, members, and collaborators can start IronLoop work. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/reborn_trace_first_party_tool_coverage.rs (1)
92-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not equate child-process success with test execution.
libtest/Cargo report success when--exactmatches zero tests, so a staletest_namewould make this coverage gate pass without running. Capture the childstdoutand assert the child ran exactly one test, or emit and check a child completion marker.Repo invariant: landed tests must run in CI rather than silently skip.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/reborn_trace_first_party_tool_coverage.rs` around lines 92 - 99, Update the isolated test process launched by the test harness around the current_exe Command to verify that exactly one test actually ran, rather than relying solely on status.success(). Capture the child output and assert libtest’s executed-test count, or add and validate a dedicated completion marker, so stale test_name values cannot make the coverage gate pass without execution.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/reborn_trace_first_party_tool_coverage.rs`:
- Around line 92-99: Update the isolated test process launched by the test
harness around the current_exe Command to verify that exactly one test actually
ran, rather than relying solely on status.success(). Capture the child output
and assert libtest’s executed-test count, or add and validate a dedicated
completion marker, so stale test_name values cannot make the coverage gate pass
without execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a40a276f-0b6d-4196-bf3b-d6ecc4b527af
📒 Files selected for processing (1)
tests/reborn_trace_first_party_tool_coverage.rs
|
Addressed the latest CodeRabbit finding in 41f0c92. The isolated child now captures libtest output and requires the exact Mutation verification: replacing the child test name with a stale value now fails the parent and reports |
|
@ironloopai review |
|
Caution Command not authorized Only repository owners, members, and collaborators can start IronLoop work. |
This branch has not been deployed
2 participants
Summary
IRONCLAW_BASE_DIRconfigured before process startupChange Type
Linked Issue
Closes #6359.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildreborn_trace_first_party_tool_coveragebinary passed 26 tests with 2 intentional ignorescargo test --features integrationif database-backed or integration behavior changed — not applicable; this isolates a test harness path and changes no database behaviorpolicy.jsonunder a controlledIRONCLAW_BASE_DIR, then reran the same exact test GREEN; after review, both focused tests also pass with a deliberately hostile parent value because the child overrides it before startupreview-prorpr-shepherd --fixwas run before requesting review — not available in this environment;scripts/pre-commit-safety.shpassedAdditional required checks:
cargo test— 607 passed across the default suitesscripts/pre-commit-safety.sh— passedTest Strategy
User behavior: developers with prior Trace Commons enrollment can run the root Trace Commons parity coverage without their local policy changing the result.
Risk areas:
Tests added or updated:
tests/reborn_trace_first_party_tool_coverage.rs.What the tests prove: an enrolled policy outside the test cannot make the Trace Commons status result report
enrolled: true, and neither scenario mutates process-global environment while its test binary is running.Commands run:
Security Impact
None. The change only redirects test state to temporary child-process directories and does not alter production file access.
Reborn Trust-Boundary Checklist
N/A: test-harness isolation only; no production trust-bearing types, ingress, policy, runtime, status, or serialization behavior changed.
Database Impact
None.
Blast Radius
Only the two Trace Commons cases in
reborn_trace_first_party_tool_coverage; each child process owns one temporary directory.Rollback Plan
Revert these test-only commits.
Review Follow-Through
Child-process isolation uses the existing env-based API without adding a production seam. If the capability harness gains explicit path injection later, these tests can adopt it and drop the re-exec helper.
Review track: A (tests)