Repository navigation
fix(subagent): re-root the state-path check when the state root is relocated - #6949
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
结论反驳不动核心修复，但这个 PR 不能原样合并：它在一个已经不可靠的前提（测试进程的 CWD 恰好是仓库根）之上新引入了一个同形状的确定性失败路径（窗口期清理），并把一条“按设计必须拒绝”的断言从文档里写没了。建议：合并前补 以下事实与推测分开标注；行号基于我 checkout 的 发现清单F1【高 · 确定】修复只覆盖了
|
追加：三条独立于 F1 的新发现（第一条是安全边界，请优先看）上一条评论之后我把 S1【中 · 确定】
|
勘误（行号，只此一处）前两条评论里我写错了两处行号，其余引用我已逐行核对过：
其余行号（ 署名🤖 由 SpikeBot 003(ClaudeCode-JP) 生成 |
…located A workspace whose `.codewhale` is a junction — the documented way to keep application state on another volume — failed every sub-agent at step 0 with "sub-agent state path must stay within state root: \\?\D:\...\state\subagent-transcripts\....jsonl". default_state_path and the transcript artifact writer both build `.codewhale/state/...` under the state root, so the junction resolves the child into its target — exactly where that user keeps the state — while the containment check still compares against the unresolved root and refuses. effective_state_root re-roots only that known, user-owned link: when `<state_root>/.codewhale` is a link and the child resolves inside its target, the target becomes the root for both the containment check and the link walk. Any other resolution keeps the stricter root, so an escape past both roots is still refused. Reproduced end to end before the fix: an installed 0.10.1 in a workspace with .codewhale pointing at an outside target failed the child with steps=0 and that exact message.
Review of this branch found the re-root applied too narrowly: only checked_subagent_state_path used the effective root, while the guards that run after it still compared the resolved path against the unresolved root. The containment check passed and the write still failed with the same "must stay within state root" message on the child's first step — prepare_subagent_transcript_parent, append_private_subagent_transcript, write_json_atomic and read_subagent_state_file all reach it, so the reported failure survived the fix. reject_state_path_symlinks is now the guard those state-path callers use: it resolves the same root decision checked_subagent_state_path made and then runs the unchanged link walk. CoordinationProcessLock keeps rejecting a linked .codewhale on purpose — its comment says create_dir_all must not populate the link target — so the lock guards are left as they are. The Windows test now drives the paths the child actually takes: manager persist_state and load_state, then the transcript writer's create and append, all under a junction whose target is outside the workspace.
…oved nothing Review findings addressed: - "The re-root only covers the branch where the target resolves" — checked against real Rust std rather than the Python stand-in the review used, and it does not hold. `std::fs::canonicalize` returns NotFound for a dangling junction (Python's os.path.realpath resolves it instead), so both the link and the child fall back to the link's own spelling and containment passes. The case is real — mklink /J accepts a target that does not exist, and creating the junction before the first run is a normal setup order — so it is now pinned as a regression case in the relocated-root test. - state_path_stays_in_the_workspace_without_a_link is removed: it took the no-link branch, which is the behaviour before this branch, so it could not fail without the fix, and its assertion depended on the test process's working directory.
00687a4 to
6021aec
Compare
|
Thank you @SparkofSpike for tracing the step-0 failures to relocated I am picking this up alongside the 0.10.2 Windows work. It is not currently included in #6968. We will preserve this as your PR and handle overlap with our moving main branch on the maintainer side. The distinction between an existing junction target and first use of a missing target remains part of the review; the missing-target test currently proves path admission, while the write-path test uses an existing target. Your contributor-owned regression coverage should remain in the review—the no-new-tests instruction constrains our agent work. Native follow-up: an ordinary-user Rust std diagnostic in our Windows 11 ARM64 VM confirms the distinction. With an existing junction target, creating Maintainer reconciliation is now pushed on your original branch at 4a3f04e202. It brings in main’s fixes for the unrelated stale-base failures, and changes your existing persistence check to the synchronous writer so it completes and reports write errors before loading. No new regression test was added. Formatting and web gates pass; npm 1312 passed, 0 failed, 7 skipped. The updated head has not yet been run on Windows: its new hosted checks are the merge gate. Thank you for carrying the root decision through every state/transcript operation; that is the behavior we are verifying for 0.10.2. |
…write Preserve SparkofSpike's PR and original commits while reconciling against main 5ebb2c1. The existing Windows relocated-state persistence check now uses persist_state_synchronously: it completes the write and propagates failures before load_state instead of dropping a background handle. No new tests or comments. Verification: npm test: 1312 passed; 0 failed; 7 skipped (Node TAP 540 passed / 0 failed, Vitest 772 passed). npm run check:web PASS. cargo fmt --all -- --check PASS. git diff --check PASS; current-main reconciliation conflict-free. Windows limits: the original PR's hosted Windows junction cases passed, but that stale base had three unrelated failures now fixed on main. This reconciled head and the strengthened synchronous assertion have not yet been compiled or run on Windows; its hosted Windows CI is the merge gate. No claim of missing-target first-write support or packaged acceptance. Refs codewhale-hq#6949
|
Thank you, @SparkofSpike, for tracking the Windows state-path junction failure to containment checks rejecting a valid existing target, and for the focused fix in this PR. It has now merged as your PR in 182bafe6 and is being included in the 0.10.2 candidate. The final PR head passed hosted CI on Windows (18,301 passed, 27 skipped), macOS (19,035 passed, 33 skipped) and Ubuntu (19,032 passed, 34 skipped). Windows ran and passed both junction cases and |
Document the merged contributor sub-agent state fix with its actual existing-target junction scope. Credit SparkofSpike for #6929 and #6949, and Lstarsky0 for #6971 in the 0.10.2 contributor list. Regenerate the embedded changelog from the same source. Verification on macOS, source410d8310ca plus these two changelog paths: - npm test: Node540 passed;0 failed;7 skipped; Vitest772 passed. Combined1312 passed;0 failed;7 skipped. - npm run check:web: PASS. - sync-changelog.sh --check and git diff --check: PASS. #6949 final PR head hosted CI: Windows18301 passed/27 skipped, macOS19035 passed/33 skipped, Ubuntu19032 passed/34 skipped. Both Windows junction cases and owner_crash_reentry passed. Final candidate CI, dangling-target, native/package and oldest-Windows acceptance are separate remaining gates. No tests were added in this documentation slice. Refs #6949 Refs #6971
2 participants
Summary
A workspace whose
.codewhaleis a junction — the way to keep applicationstate on another volume — failed every sub-agent at step 0:
The child never started reasoning, so no result was produced.
Cause
default_state_pathand the transcript artifact writer both build.codewhale/state/...under the state root. When<state_root>/.codewhaleisa junction, the child resolves into the junction's target — exactly where the
user keeps the state — while
checked_subagent_state_pathcompares it againstthe unresolved root and refuses.
The same user-owned link that the session-artifact path already honours (see
the sibling change to
fleet/files.rs) was rejected here, and the refusallands before any work: the child cannot even write its own transcript.
Changes
effective_state_root(state_root, state_path)(new): when<state_root>/.codewhaleis a link (reparse point on Windows, symlinkelsewhere) and the child resolves inside its target, the target becomes the
root for both the containment check and the link walk in
checked_subagent_state_path. A child that resolves anywhere else keeps thestricter state root, so this re-roots one known, user-owned link and does not
widen the boundary: an escape past both roots is still refused.
metadata_is_link_or_reparsepredicate, because a Windowsjunction carries the reparse attribute without the symlink tag —
FileType::is_symlinkalone would miss exactly this case.checked_subagent_state_pathnow use the same root. Beforethis, a relocated state directory failed containment first and would have
failed the link walk second.
Evidence
Reproduced end to end on 0.10.1 (Windows) before the fix: an installed
binary in a workspace whose
.codewhalepoints at an outside target failed thechild with
steps=0and the exact message above. The same workspace withoutthe link works, and a junction whose target sits inside the workspace also
works — the failing case is specifically a relocated state root.
A Windows test (
state_path_accepts_a_state_root_relocated_behind_a_junction)creates a real junction and pins four directions: the relocated root passes and
the state lands in the junction target; an escape past both roots still fails;
and a junction whose target does not exist yet also passes (see the note
below).
state_writes_follow_a_relocated_state_rootdrives the paths the childactually takes: manager
persist_state/load_state, then the transcriptwriter's
createand append.Not run here: the repository's own gates. Note that CI on a fork-sourced pull
request starts in
action_requiredand waits for a maintainer, so thisbranch's checks have not executed; the same commits were run on the fork with
its Actions enabled, and that result is what backs the test claims above.
Review responses
Review found the re-root applied too narrowly — only
checked_subagent_state_pathused the effective root, while the guards thatrun after it still compared the resolved path against the unresolved root.
The containment check passed and the write still failed with the same message
on the child's first step:
prepare_subagent_transcript_parent,append_private_subagent_transcript,write_json_atomicandread_subagent_state_fileall reach it, so the reported failure survived thefix.
125cf62d4responds:reject_state_path_symlinksis now the guard thosestate-path callers use, resolving the same root decision
checked_subagent_state_pathmade before running the unchanged link walk.CoordinationProcessLockkeeps rejecting a linked.codewhaleon purpose —its existing comment says
create_dir_allmust not populate the link target —so the lock guards are untouched.
The Windows test now drives the paths the child actually takes: manager
persist_stateandload_state, then the transcript writer'screateandappend, all under a junction whose target is outside the workspace.
A second review round raised three further points, answered here:
"The fix misses the branch where the junction target does not exist yet."
Checked against real Rust std rather than the review's Python stand-in, and
it does not hold.
std::fs::canonicalizereturnsNotFoundfor a danglingjunction (Python's
os.path.realpathresolves it instead), so both the linkand the child fall back to the link's own spelling and containment passes:
The case is real (
mklink /Jaccepts a missing target, and creating thejunction before the first run is a normal setup order), so
00687a4acpinsit as a regression case rather than leaving it argued in prose.
A test that could not fail was removed.
state_path_stays_in_the_workspace_without_a_linktook the no-link branch —the behaviour before this branch — so it passed without the fix, and its
assertion depended on the test process's working directory.
00687a4acdrops it.
CoordinationProcessLockwas not weakened. It never goes throughchecked_subagent_state_path, soeffective_state_rootcannot reach it;the Unix test that pins its refusal still passes.
On the no-new-tests rule
A reviewer flagged that this branch adds tests and comments, against
AGENTS.md's "No new tests and no new code comments unless explicitly asked".Both are kept here on purpose, and can be dropped on request:
child-at-step-0 error — and they drive the paths the child actually takes
(manager persist/load, transcript create/append), which is what the first
revision of this branch missed. The rule's own carve-out for a requested
regression test applies.
effective_state_root(one known,user-owned link; every other resolution keeps the stricter root) and why
CoordinationProcessLockdeliberately stays strict. Without them theasymmetry reads as an oversight and invites a later "fix" that widens it.
CI status on this branch
The checks are red, and every failing job is a failure of the base, not of
this branch:
Lintcrates/tui/src/core/authority/auto_review.rs: path_canonicalize sites 3 > budget 0— a file this branch does not touch, left over from the authority move ind776dcb61Test (macos-latest)conformance::prompt::model_visible_prefix_bytes_match_goldens— thefile_searchtool was added to the catalog and the golden was not re-recordedTest (windows-latest)/(ubuntu-latest)acp_server::tests::acp_full_access_keeps_repo_law_without_permission_modaland twocore::engine::tests::*_full_access_*(deadline elapsed) — none in this branch's filesDocumentationcrates/commands/(only reaches this job because it was dispatched manually; the job isif: schedule || workflow_dispatch)Version driftrefs/tags/v0.10.1The decisive evidence: upstream
main's own CI on the two commits precedingthis branch's base (
f723f64f3,3893d73c0) fails with the same set —Integrations,Lint,Test (windows-latest). This branch cannot be greenwhile its base is red.
What the branch's own tests did do, on a real Windows runner in run
37980841780:The branch changes exactly two files, both under
crates/tui/src/tools/subagent/.Type of Change
Related Issues
No-Issue: observed in an operator session on 0.10.1 (Windows); the state
directory relocated behind a junction failed every sub-agent at step 0.