fix(ci): resolve SANDBOX_DOCKER_EXACT_PATHS crate prefixes dynamically - #7394
theredspoon wants to merge 2 commits into
Conversation
…l prefix SANDBOX_DOCKER_EXACT_PATHS hardcoded the crate-root prefix for every entry owned by ironclaw_cli, ironclaw_composition, ironclaw_config, ironclaw_host_runtime, ironclaw_runtime_policy, and ironclaw_sandbox. A future relocation of any of those crates (this repo has already moved ironclaw_wasm twice) would silently stop matching those files, quietly dropping them from the Docker sandbox lane with no error. Replace the constant with _sandbox_docker_exact_paths(), which keeps each entry's crate-relative suffix as a literal but resolves the crate's root directory through crate_directory(), matching the established pattern in _sandbox_docker_prefixes() and _webui_frontend_prefix(). Fails closed with a RuntimeError if any of the six crates can't be resolved. Non-crate-owned entries (Dockerfile.sandbox-worker, root-level tests/...) stay plain literals.
github-actions
Bot
added
size: M
📝 WalkthroughSummary by CodeRabbit
WalkthroughSandbox Docker exact-path routing now resolves crate-owned paths through ChangesSandbox Docker exact-path routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Planner
participant CrateInventory
participant TestPlan
Planner->>CrateInventory: Resolve owning crate directories
CrateInventory-->>Planner: Return crate paths
Planner->>Planner: Append crate-relative suffixes
Planner->>TestPlan: Build plan with resolved exact paths
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 4m 15s IronLoop completed the review and posted it to GitHub. 🔗 Result |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b08afe6a12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with mock.patch.object( | ||
| planner, "crate_directory", side_effect=fake_crate_directory | ||
| ): | ||
| exact_paths = planner._sandbox_docker_exact_paths() |
There was a problem hiding this comment.
This regression test calls _sandbox_docker_exact_paths() directly, so it remains green if build_plan() stops consuming the resolved set or fails to set run_sandbox_docker for a relocated path—the exact silent loss of Docker coverage this change is intended to prevent. Drive build_plan() with the relocated planner.rs/resolver.rs paths and assert the emitted Docker-lane flag instead of only inspecting the helper result.
AGENTS.md reference: AGENTS.md:L78-L83
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 77923ed. I reproduced the gap with a mutation test first: I removed the path in sandbox_docker_exact_paths arm from build_plan()'s consumption check (keeping the prefix arm intact) and reran the suite. test_sandbox_docker_exact_paths_resolve_through_crate_inventory_when_nested stayed green because it only asserts on _sandbox_docker_exact_paths()'s return value, never touching build_plan(). That's the exact silent-coverage-loss scenario this PR exists to prevent.
Added test_sandbox_docker_exact_paths_route_relocated_crate_through_build_plan, following the same pattern as test_sandbox_docker_prefix_follows_crate_inventory_when_nested: it mocks crate_directory to relocate ironclaw_runtime_policy and drives build_plan() itself instead of calling the resolver directly. With the mutation in place this new test fails (run_sandbox_docker is False); reverting the mutation makes it pass again.
One deliberate detail: the test also mutates the synthetic package's manifest_path to the relocated directory. Without that, build_plan()'s package-directory lookup can't find the moved crate, hits the "a crate path maps to no workspace package" arm, and falls back to _full_plan() — which sets run_sandbox_docker=True unconditionally regardless of the exact-paths logic. That fallback would make the assertion pass for the wrong reason and mask a real regression. The test also asserts plan["mode"] == "selected" (not "full") specifically to rule that out.
Reran both suites clean: test_reborn_pr_test_plan.py (75/75) and test_ws12_workflow_contracts.py (52/52).
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
🟢 No actionable findings
No actionable findings in the exact merge-base-to-head comparison.
Validation
- ✅ git diff --check — Completed without whitespace errors.
- ⚪ Focused behavioral validation — Not run. No concrete defect warranted execution; the review relied on static inspection of the complete diff, planner call paths, crate inventory behavior, and tests.
Review details
- Run:
73c22572-6f0d-4d36-a32f-1447ccf76639 - Workflow: Review
- Attempts: 1
_sandbox_docker_exact_paths_resolve_through_crate_inventory_when_nested only inspects the resolver's returned set, so it would stay green if build_plan() stopped consuming that set or stopped setting run_sandbox_docker for a matched path. Its sibling relocation test for _sandbox_docker_prefixes already drives build_plan() end-to-end; add the equivalent for the exact-paths resolver so a regression in build_plan()'s consumption of the resolved set is actually caught.
This branch has not been deployed
1 participant
SANDBOX_DOCKER_EXACT_PATHSinscripts/ci/reborn_pr_test_plan.pyhardcoded the crate-root prefix (crates/app/...,crates/kernel/...,crates/lanes/...) for every entry owned byironclaw_cli,ironclaw_composition,ironclaw_config,ironclaw_host_runtime,ironclaw_runtime_policy, andironclaw_sandbox. If any of those crates is relocated, the literal stops matching and the planner silently stops routing that crate's listed files to the Docker sandbox lane — no error, just quietly dark coverage. This is the same bug classironclaw_wasmhas hit at least twice already (538b1f021moved it intocrates/lanes/,8ed9d4afcmoved every crate into its family directory), each time requiring a follow-up fix (48e27e0f9,00d9679b3) to re-resolve the path dynamically.I replaced the constant with
_sandbox_docker_exact_paths(), matching the pattern already established in this file by_sandbox_docker_prefixes()and_webui_frontend_prefix():SANDBOX_DOCKER_EXACT_PATH_LITERALS— the 10 entries with no owning crate (Dockerfile.sandbox-worker, root-leveltests/...paths), unchanged as plain literals.SANDBOX_DOCKER_EXACT_PATH_CRATE_SUFFIXES— adict[str, tuple[str, ...]]mapping the 6 crate names to their 17 crate-relative suffixes, joined at call time withcrate_directory(crate, ROOT).RuntimeErrorif a crate can't be resolved, same message style as_sandbox_docker_prefixes().I verified all 27 original entries are represented 1:1 in the new split (10 literals + 17 crate suffixes), and independently confirmed
crate_directory()resolves all 6 crate names to the exact prefixes the old literals assumed.Tests: added
test_sandbox_docker_exact_paths_resolve_through_crate_inventory_when_nested(proves a relocated crate's entries follow it, old-location entries disappear) andtest_sandbox_docker_exact_paths_resolution_failure_fails_closed(proves theRuntimeErrorfail-closed path). Updated two pre-existing tests to mock_sandbox_docker_exact_pathssince it's now called unconditionally inbuild_plan()alongside their owncrate_directorymocks — verified this guard is load-bearing by removing it and confirming both tests fail for the expected reason. Addedtest_sandbox_docker_exact_paths_route_relocated_crate_through_build_planin response to CodeRabbit feedback that the first test above only pins_sandbox_docker_exact_paths()'s return value, which would stay green even ifbuild_plan()stopped consuming the resolved set or settingrun_sandbox_dockerfor a matched path — this test drivesbuild_plan()itself with a relocated crate and asserts the emitted Docker-lane flag, closing that coverage gap.test_reborn_pr_test_plan.py: 75/75 passing.test_ws12_workflow_contracts.py: 52/52 passing.