Repository navigation
fix(subagent): re-root the state-path check when the state root is relocated - #6949
SparkofSpike wants to merge 3 commits into
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
This branch has not been deployed
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.