feat(fork): multi-root workspace_roots authorization infrastructure (runtime lanes) on v0.9.12 - #54
qiuYliangM wants to merge 1 commit into
Conversation
|
Thanks @qiuYliangM for taking the time to contribute. This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered. Please read |
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Thanks for the careful port — I reviewed this end to end (protocol/persistence, engine/turn lifecycle, sandbox/execpolicy boundary, commit hygiene) with local compile and test verification. The layering is real, the single-root byte-equivalence discipline holds everywhere I checked, and the resume current_dir fallback fix is a genuine root-cause fix. However, I'm requesting changes: one workspace-level compile break, several verified boundary defects, and a rebase with a semantic-merge requirement.
Must fix
1. cargo test --workspace fails at HEAD (E0063 ×7)
ExecPolicyContext gained a non-defaulted field, but 7 existing construction sites were missed. They only compile under --all-targets, which is why targeted -p codewhale-tui --lib runs stayed green and PR CI (Contribution intake only) didn't catch it:
crates/app-server/src/lib.rs:2643, 2679, 2725, 2774crates/config/src/tests.rs:718, 850, 867
Repro: cargo check --workspace --all-targets. This will surface at the Merge Queue full gates. Please add workspace_roots: Vec::new() at each site and run a workspace-wide check before pushing.
2. Write carve-out: relative paths are approved against roots the write never touches
Approval side: crates/tui/src/core/authority.rs:526 judges a relative target against every root via .any(|root| carve_out_target_within_root(root, raw)), and carve_out_target_allowed joins the relative raw onto each root (authority.rs:551). Execution side: ToolContext::resolve_path joins relative paths onto the primary workspace only (crates/tui/src/tools/spec.rs:1088-1093), and a carve-out hit skips the modal (turn_loop.rs approval_required = false).
Scenario: primary root is not a git work tree, one attached root is. write_file("src/main.rs") passes the carve-out via the attached git root and writes — modal-free — into the non-git primary. Attaching a single git root silently disables write approval for all relative-path writes in the primary, defeating the carve-out's own "version-controlled edits stay reviewable and recoverable" rationale (authority.rs:509-512). Suggested fix: resolve relative targets against the primary root only (matching execution semantics), or resolve to absolute target paths before per-root judgement. Please add a test with non-git primary + git attached root + relative path asserting the modal is kept.
3. execpolicy: workspace-scoped allow rules now apply session-wide
Scope matching changed from "matches ctx.cwd" to "matches any attached root" (crates/execpolicy/src/lib.rs, pinned by workspace_scoped_rule_matches_any_workspace_root). For ask/deny this only adds prompts, but for allow rules it widens auto-approval: a rule {exec_shell, "git push", exact, workspace=/shared} now also approves git push executed with cwd = primary /workspace (exec always runs in the session cwd, see tools/shell.rs), where the remote may be a different repository entirely. This is a deliberate, tested semantic — but it needs either (a) an explicit narrowing for Allow (multi-root only for ask/deny), or (b) a documented sign-off that allow-scoped rules become session-scoped, reflected in the fork docs.
4. Rebase: the execpolicy conflict is a semantic merge, not mechanical
The branch is 13 commits behind pinvou3-clean; crates/execpolicy/src/lib.rs conflicts with #37. A correct resolution must keep both:
- #37's
read_rulesets()accessor — the rulesets field is now behind aRwLock, so this branch'sself.rulesets.iter()will not compile against current base; and - #37's
absolute_path_rule_matchesfallback for the path filter, OR-ed with the per-root loop. Dropping it makes pinned absolute-location ask rules (real home,/root, Windows profile) silently unmatched → fail-open under UnlessTrusted.
Sketch: per-root normalization match || absolute_path_rule_matches(pattern, call_path) evaluated on the original call path, keeping both sides' tests. I verified via merge-tree that all other auto-merged files have non-overlapping hunks, so this file is the only real rebase work.
5. forkguard_workspace_roots_sandbox_materializes_every_root is red on macOS
Verified locally (deterministic; 37/38 forkguard tests pass): get_writable_roots canonicalizes only the cwd parameter (plus /tmp and TMPDIR); additional roots from writable_roots stay in raw form, so on macOS (/var/folders/... vs /private/var/folders/...) the attached root's canonical spelling is missing and the assertion at core/authority.rs:970 fails. Seatbelt (seatbelt.rs:370) and bwrap re-canonicalize at consumption, so there is no functional hole — but the test and the implementation disagree, and "tui lib filtered groups green" holds only on Linux. Please align (canonicalize writable_roots in get_writable_roots, or relax the test to the guarantee the function actually makes) and confirm a macOS run.
6. exec --resume silently drops workspace_roots
SessionMetadata.workspace_roots is never written in production (session_manager.rs:904/2230 are the only writers, both Vec::new()). The runtime API save path copies snapshot.* fields into metadata but not snapshot.workspace_roots (runtime_api/sessions.rs), so the wiring in exec_agent.rs:209-212/378-385 reads a permanently empty field and a multi-root thread degrades to single-root on exec --resume with no error — which contradicts the body's "roots enter only via Runtime API/headless". Either thread the roots through the save path (small fix) or explicitly declare exec out of scope in the body plus a code comment so the dead pipeline doesn't mislead.
Should be split
e45d2d20d + a78c223de (project_instructions source label → file name) is an independent, user-visible single-root behavior change with its own rationale (KV prefix-cache stability). It is not required by multi-root: AGENTS.md discovery stays primary-root-only, so source_path doesn't move with the root set. Please split it into its own PR, or at minimum disclose the behavior change in this PR body. The rationale is also incomplete: <codewhale_repo_constitution source="{abs path}"> (project_context/constitution.rs:248) sits in the same cache-stable region with the same problem and is untouched.
Non-blocking
WORKSPACE_ROOTS_SYMBOLhas no runtime consumer (definition + doc link + value-pinning test only), and its doc ("surfaces carry this symbol") overstates: every materialization point hand-rollsnormalize_workspace_roots. Delete it or reword; the pinning test is tautological.- ACP dead data flow:
prepare_acp_tool_admissionreadsregistry.context().workspace_roots, but the registry context is built withoutwith_workspace_roots, andload_sessiondrops persisted roots — permanently empty. Same scope decision as #6. - Runtime fork: does not inherit the parent thread's roots and still falls back to
current_dirfor cwd (same class as the resume bug fixed here); theThreadForkParamswiring has no test. - The resume running-threads branch (applying
resolve_resume_rootsto the cached thread) has no direct test. - Thread DTO always serializes
"workspace_roots": []while params skip when empty — inconsistent wire convention; addskip_serializing_ifor a pinning test. update_thread_workspace_rejects_active_turnwas not extended to the roots field; nothing pins the mid-turn SyncSession deferral ("effective next turn" currently rests on the op-loop architecture);op_to_protocol(SyncSession)mapping has no parity test.session.project_contextbecomes write-only after theworkspace_changedgate (no readers anywhere) — clean up or comment.- Body numbers don't reproduce: forkguard tests go 37→44 (not 31→38), and "7 new" is 6 new + 1 rename.
- ~24 stray blank lines after
workspace_roots: Vec::new()(rustfmt-stable, but noise against base). - Commit messages are all Chinese while
pinvou3-cleanpractice is all-English conventional commits (no written rule — flagging for consistency).
Verified green locally
protocol / state / core / execpolicy suites all pass on this branch; cargo fmt --all --check is clean; the SQLite v5 migration (guarded ALTER, idempotency test) and the three SELECT column orders check out; legacy-decode defaults for all JSON/DTO stores are tested; single-root byte-equivalence holds for policy values, wire frames, and path decisions.
f252619 to
e2dc637
Compare
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-1 remediation verified end to end — all six must-fix items are genuinely fixed. Confirmed locally on macOS: cargo check --workspace --all-targets green (the E0063×7 break is gone), cargo fmt --all --check clean, the forkguard / carve_out / sandbox::policy / workspace_roots filtered groups pass (87/0, which covers the previously red forkguard_workspace_roots_sandbox_materializes_every_root on macOS), and the protocol / state / core / execpolicy suites are green. The carve-out fix is red-green correct and matches execution semantics for both relative and absolute targets; the Allow narrowing is structural (scope and rooted-path match both primary-only, ask/deny still span); the #37 semantic merge preserved both read_rulesets() and the absolute_path_rule_matches fallback on the original call path; the macOS canonicalization is in.
However, I'm requesting changes again on one cluster: the R1-6 fix ("closing the silent exec --resume degradation to single-root") landed only on the paths named in round 1. Three sibling paths of the same class still silently drop workspace_roots, and two of them durably erase roots that are already persisted on disk.
Must fix
1. POST /v1/sessions drops the thread's workspace_roots
create_saved_session_with_id_and_mode hardcodes workspace_roots: Vec::new() (session_manager.rs:2185/2230), and the save-thread-as-session handler (runtime_api/sessions.rs:401-419) stamps provider route, title, and system_prompt onto the metadata — but not detail.thread.workspace_roots, which is right there on ThreadDetail.thread. Repro: create a multi-root thread via the runtime API → run a turn → POST /v1/sessions {thread_id} → the session carries no roots → a later exec --resume silently degrades to single-root. This is exactly the R1-6 defect through a sibling path, and it contradicts the body's remediation claim, which is accurate for PUT /v1/sessions only.
2. POST /v1/sessions/{id}/resume cannot carry roots — and the round-1 save fix then erases the persisted set
resume_session_thread (runtime_api/sessions.rs:288-325) copies workspace, model, provider, mode, and system_prompt from the session metadata, but cannot copy roots: CreateThreadRequest (runtime_threads.rs:2134) has no workspace_roots field at all (unlike UpdateThreadRequest), and create_thread hardcodes Vec::new() (runtime_threads.rs:5446). Worse, the fixed save path now works against this surface: the next save copies the (empty) engine snapshot roots over the persisted metadata (sessions.rs:777/795/817), so resuming a multi-root session via the HTTP API and saving once permanently erases the roots on disk. The same session file resumed via exec --resume keeps its roots — the two resume surfaces disagree, with no error on either.
3. TUI resume + autosave launder a multi-root session to single-root and erase the persisted roots
Every TUI-side engine (re)build passes workspace_roots: Vec::new() (frame.rs:716, session_state.rs:600/649, handlers.rs:1188/2221, apply.rs:1099/1236/1408/2297, event_loop.rs:733/1149/3699). build_session_snapshot (frame.rs:881) builds metadata through the same empty-roots constructor, and merge_persisted_lifecycle (session_manager.rs:1665-1678) restores lifecycle fields (title/archived/created_at/fork lineage) but not roots. So: open a multi-root session (saved by exec or the runtime API) in the TUI session picker → roots are stripped in memory → the next autosave rewrites the session file without workspace_roots → durable on-disk erasure. The body's boundary ("No multi-root UI in the TUI; roots enter only via Runtime API/headless") covers running single-root in the TUI; it does not cover destroying roots that another host persisted, and the erasure is undisclosed.
Suggested fix shape
One shared fix covers all three: stamp the roots wherever a session/thread is rebuilt from a source that carries them (thread detail → session; session metadata → created thread; persisted metadata → snapshot merge), plus a regression test per surface. If any surface is deliberately out of scope, it needs both (a) a guard that refuses to overwrite non-empty persisted roots with empty, and (b) an explicit declaration in the body's known-scope list. Please also add a test locking the round-1 save stamping itself — commit b24cd70d currently has none, so the behavior the body advertises is unregressed.
Non-blocking (follow-ups, does not gate merge)
import_foreign(session_manager.rs:904) drops recoverable roots even though the export container serializes them.Runtime::invoke_toolpassesworkspace_roots: Vec::new()on the stateless app-server bridge (core/src/lib.rs:1391), so path-scoped ask/deny rules never fire for that surface; worth a doc note or threading roots throughToolCallRequest.- Read-only Scout cannot
cdinto an attached root:resolve_pathis multi-root aware but the readonly cwd check (shell.rs:3888-3892) is primary-only. Fail-closed, but inconsistent with the "full accessible root set" framing. - No test pins the Allow path narrowing (
&roots[..1]) — only the workspace-scope narrowing is pinned — and no test pins a deny rule scoped to an attached root;authorization_order.rshas no multi-root case. - Parent-repo registration: the new
forkguard_workspace_roots_*tests are not yet in thefork-guard.shregistry /docs/fork-modifications.md; the stacked pinvou-agent PR should land alongside or before this merges to avoid a guard-drift window.
Verified green locally on macOS: cargo check --workspace --all-targets, cargo fmt --all --check, forkguard/carve_out/sandbox::policy/workspace_roots groups (87/0), protocol/state/core/execpolicy suites.
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Thanks for the thorough PR description and the two rounds of remediation — the test discipline here is genuinely good, and the byte-identical-when-empty claims hold up under independent verification (including a cross-branch harness diff of the wire frames). Requesting changes for the items below; most are small fixes with in-PR precedents.
Must fix before merge
1. Both TUI fork paths silently drop the parent's workspace_roots — contradicting the round-2 commit message
crates/tui/src/commands/groups/session/session.rs:174 (fork_from_session) forwards forked.metadata.workspace_roots.clone(), and session.rs:294 (fork) forwards parent.metadata.workspace_roots.clone() — but both forked and parent are freshly constructed by create_saved_session_with_id_and_mode, so their roots are always empty; the cached-metadata backfill block in fork copies created_at/title/parent_session_id/forked_from_message_count but not roots. The correct sources are source_session.metadata.workspace_roots and app.workspace_roots respectively.
Commit 98f57ffd8 explicitly states "forks carry the parent's [set]", so this reads as an implementation slip, not a design choice. Worse, it's sticky: the forked file is saved to disk with the empty set, and merge_persisted_lifecycle treats the disk record as authority — any later autosave re-erases an in-memory correction. The fix needs both the forwarding source and a stamp on forked.metadata.workspace_roots before save_session.
2. switch_workspace keeps the old primary root as an attached writable root
crates/tui/src/tui/ui/session_state.rs:649-657 sends Op::SyncSession { workspace: <new>, workspace_roots: app.workspace_roots.clone() } where app.workspace_roots is the full normalized set with the old workspace at position 0. The engine's normalize_workspace_roots then yields [new_ws, old_ws, ...] — after /cd, the previous directory stays accessible and writable.
This diverges from the other two primary-swap paths in the same PR, both of which filter the old primary out: the PATCH workspace-only branch (runtime_threads.rs:5851-5863) and resolve_resume_roots (crates/core/src/lib.rs:101-110). It's also a silent widening relative to pre-PR single-root behavior, where /cd removed the old directory from the workspace. Note begin_launch_session in the same file resets roots to empty, so the PR makes two opposite choices for two similar transitions. Please pick one semantics; if keeping the old root is intentional it needs a comment and a forkguard test.
3. LoadSession failure path pollutes app.workspace_roots and can push a foreign root set into the current session's engine
crates/tui/ui/apply.rs:1192-1194: the seeding app.workspace_roots = session.metadata.workspace_roots.clone() runs before Config::load and apply_loaded_session_config_snapshot, both of which can fail and return early. On failure the App state has already been mutated: the current session's workspace_roots now holds the set of a session that was not loaded. Any subsequent routine re-sync (provider switch, profile switch, backtrack, plugin activation — all forward app.workspace_roots.clone()) then pushes that foreign set into the live engine, widening its writable scope; the next fork also inherits it. Disk-side has the merge_persisted_lifecycle backstop, engine-side has none. Move the seeding after the load steps succeed, before the respawn (respawn's build_engine_config still reads it).
4. Resume/fork params use Vec instead of Option<Vec>, losing upstream's "explicit empty clears roots" semantics
crates/protocol/src/lib.rs:181 (ThreadResumeParams.workspace_roots: Vec<PathBuf> + skip_serializing_if = "Vec::is_empty") and crates/core/src/lib.rs:91 (resolve_resume_roots treats empty as "not provided"). Upstream codex models this as Option<Vec<AbsolutePathBuf>>: Some(roots) replaces, Some([]) explicitly clears, None inherits persisted values. The fork merges "absent" and "explicitly empty", making it impossible to shrink roots back to [cwd] via resume — clearing is only reachable through a turn-level SyncSession. That contradicts the "ported 1:1 from the upstream codex workspace_roots mechanism" claim. Either switch to Option<Vec> (small change), or amend the PR description to state this as a deliberate simplification.
5. The running-threads resume branch applies cwd/roots overrides only to the return value — never to the cache or DB
crates/core/src/lib.rs:666-685: the running branch clones the thread, mutates cwd/workspace_roots, and returns it via NewThread, but neither running_threads.insert nor persist_thread. The persisted branch does both. Consequence: resuming a live thread with a roots override is correct for that call (SyncSession lands next turn), but a subsequent parameterless resume falls back to the stale cached roots, and any cache-authoritative persist writes the old set back to the DB, erasing the override. The base branch had this asymmetry for cwd; this PR extends it to roots. Please either write the override back to the cache (DB persistence can align with the autosave path) or document and test the transient semantics.
Should fix or explicitly schedule
- exec_agent workspace/roots pairing (latent):
latest_workspacefollowsEvent::SessionUpdatedmid-run whilelatest_workspace_rootsis frozen at resume, and the event carries no roots (crates/tui/src/exec_agent.rs:368,925,1035). Today unreachable (exec only sends the startup SyncSession), but the first mid-run SyncSession produces a persisted(workspace=new, roots=[old_ws,...])pair that widens access on the next resume. Cheap fix: add the field to the event. - Subagent with an explicit
cwd:silently inherits the parent's full root set (crates/tui/src/tools/subagent/mod.rs:9308): onlycontext.workspaceis swapped. Not a privilege escalation (the model already holds parent permissions), but it partially dissolves subagent filesystem isolation and is undocumented/untested. - ACP host answers the same question two ways (
acp_server.rs:703-741): ask-rule checks useregistry.context().workspace_roots, whileAutoReviewContext::from_tool_callpasses&[]for the carve-out judgment. Conservative direction, but same fact, two answers — unify or comment. Alsobuild_acp_tool_registry(acp_server.rs:2111) never callswith_workspace_roots, so the ask-rule wiring is always empty today. - Display paths hardcode
&[](underwater.rs:882,commands/groups/config/status.rs:282,lib.rs:12088): resumed multi-root sessions get single-root filesystem-scope summaries. Display-only, but inconsistent with the "don't lose, don't invent" posture elsewhere. - Allow+path primary-root narrowing has no negative regression test (
execpolicy/src/lib.rs:503-507): the narrowing is correct (and the workspace-scope direction is locked byworkspace_scoped_allow_rule_stays_primary_root_scoped), but a future refactor could silently regress the path direction. One test pinning "Allow + relative path rule does not auto-approve under an attached root" would close it. - State layer swallows invalid JSON silently (
crates/state/src/lib.rs:2049-2052): the fallback direction is safe (empty = single-root legacy), but a writer-side serialization bug would present as threads silently degrading to single-root with no trace. Atracing::warn!matches the crate's existing tolerance-with-visibility precedent. CreateThreadRequest.workspace_rootslacksskip_serializing_if = "Vec::is_empty"(runtime_threads.rs:2154-2160), unlike every other DTO in the PR. Harmless on the request path; worth aligning.- Test quality nits: the AGENTS.md lock test's "byte-identical" half is tautological (both calls take identical inputs; only the negative assertion does work), and the protocol byte-parity assertion is
!encoded.contains("workspace_roots")rather than a golden string. Neither blocks merge; both are one-line improvements.
Known-boundary accuracy check
The self-declared non-blocking items were verified as accurate, with two corrections: update_thread_workspace_rejects_active_turn is in fact already extended to roots (the fence covers workspace_changed || roots_changed; only a roots-scoped test case is missing), and the execpolicy lexical-normalization gap (a deny rule missing because an attached root is configured via a symlink spelling) exists identically for single-root cwd in base — the PR widens the exposure surface without introducing the flaw.
Commit hygiene
63f4d15bbleaves the tree uncompilable under--all-targets(E0063 ×4 in app-server, ×3 in config tests) until058d9b37bsix commits later;ca702df9eis a similar rebase fixup. Confirmed by checking out15cd15eab.058d9b37b,ca702df9e, ande2dc637a0(the born-deadWORKSPACE_ROOTS_SYMBOL) should be squashed into the commits that introduced the breakage if history rewriting is acceptable pre-merge.- ~10 struct literals carry a stray blank line after
workspace_roots: ...;authority.rs:1050-1062has mangled test indentation; one unrelated blank line removed inexecpolicy/src/lib.rs:2417. Pure diff noise — please sweep. - The v5 migration block doesn't update the local
user_versionvariable (harmless today, a trap for the v6 migration).
What checked out
For the record: cargo check --workspace --all-targets and cargo fmt --all --check are clean; protocol (20), state (29), core (122), execpolicy (122+), and the tui forkguard/workspace_roots/carve_out/sandbox::policy/runtime_threads groups were all re-run green independently of the PR's own claims. The empty-set byte-identical guarantee was additionally verified by harness-diffing serialized frames across branches. The snapshot semantics (mid-turn SyncSession effective next turn) are structurally guaranteed by the single op-loop (turn_loop never drains rx_op; only two same-frame assignment sites exist), not by convention. No out-of-topic functional code was found in any of the 51 files. The Allow-primary-root-scoping security design is correct with no residual cross-root leakage path (absolute-path pinning is a deliberate, spec-preserved carve-out).
Verification was done on Linux; the PR description still notes the round-2 tree awaits a macOS pass — please confirm that before merge given the canonicalize changes.
98f57ff to
890d5f6
Compare
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-4 review (fresh pass, all layers re-swept independently). Head 890d5f61d is already a fast-forward descendant of the current pinvou3-clean tip (ae7e3fb36), so no rebase was needed. Verified on macOS: cargo check --workspace --all-targets green, cargo fmt --all --check clean, protocol (88) / state (29) / core (88) / execpolicy (124) suites and the forkguard / carve_out / sandbox::policy / workspace_roots / runtime_threads filtered groups all green — this also supplies the macOS pass the description was waiting on.
The five round-3 must-fixes are genuinely fixed: both fork paths stamp real sets before save_session, switch_workspace applies the same primary-swap semantics as the PATCH branch and resolve_resume_roots, LoadSession seeds only after every fallible step (all three early returns precede the seed), ThreadResumeParams is Option<Vec> with a real explicit-clear test, and the running-threads resume writes the override back to the cache. No scope creep: all 51 files serve the topic, DCO and the English-comment policy are clean, and the execpolicy Allow-narrowing design holds on every path I could attack (both the rulesets path and the absolute-path fallback verified primary-root-only; ask/deny spanning only ever adds a prompt or a block).
However, requesting changes again: sweeping the full roots-carrier surface with fresh eyes found two more holes of exactly the class this PR exists to fix, plus one fork surface left behind.
Must fix before merge
1. Session-picker switch never seeds app.workspace_roots — cross-session sandbox leak
ViewEvent::SessionSelected (crates/tui/src/tui/ui/handlers.rs:1139-1196) loads the target session, respawns the engine via build_engine_config (:1170), and sends Op::SyncSession { workspace_roots: app.workspace_roots.clone() } (:1188) — but nothing in this path assigns app.workspace_roots. The complete writer set is app/init.rs:822 (construction), session_state.rs:592 / :649, apply.rs:1220 / :1282; none is reachable from the picker, and build_engine_config (frame.rs:709) feeds that same field into the engine's materialized sandbox policy. Consequences:
- Switching from multi-root session A to session B spawns B's engine carrying A's root set (normalized under B's primary), so B's writable sandbox roots silently include A's attached directories for the whole run — a cross-session (the picker's
atoggle even allows cross-workspace,session_picker.rs:70-88) privilege widening with no user-visible trace. - The reverse direction silently strips B's persisted roots for the run; disk survives via the autosave merge, memory/engine does not.
/resume, /load, and the launch card all route through the fixed AppAction::LoadSession path — only the picker bypasses it. Fix by seeding from session.metadata.workspace_roots (mirroring apply.rs:1220) before the respawn, plus a forkguard test.
2. Startup --resume / --continue runs the whole TUI session single-root
event_loop.rs:568-608 loads the resumed session via apply_loaded_session_with_goal, which never touches workspace_roots; the subsequent build_engine_config (:705) and startup Op::SyncSession (:726-733) then forward the still-empty app.workspace_roots. A multi-root session resumed from the CLI loses its attached roots in the engine until process exit — sandbox writable roots, exec policy, and the model-facing "Accessible folders" line all degrade. This contradicts the PR's own "a resume neither drops nor invents them" guarantee on the most basic resume flow. Same fix shape as #1.
3. CLI codewhale fork still erases the source session's roots
fork_session (crates/tui/src/lib.rs:8348-8403) builds the fork from create_saved_session (fresh, always-empty metadata) and saves it without forked.metadata.workspace_roots = saved.metadata.workspace_roots.clone(). The in-app /fork fixed in this very PR carries a comment explaining why this erasure is sticky ("the disk-authority lifecycle merge keeps re-erasing any later correction"); the rationale applies verbatim to the CLI surface. Note also that the "runtime fork does not inherit parent roots" known-boundary wording does not cover this: RuntimeThreadManager::fork_thread (runtime_threads.rs:6043) clones the full ThreadRecord and does inherit — the non-inheriting surfaces are the session-level forks, of which the CLI one is the unfixed one.
Should fix (non-blocking)
- The "single-root frames are byte-identical" claim is overstated for the Thread DTO.
normalize_workspace_rootsprepends the cwd and both spawn paths persist the full set, so a newly created single-root thread carriesworkspace_roots: [cwd]on the wire and in the state row —skip_serializing_ifnever fires for threads created by this build (protocol/src/lib.rs:94-97comment vscore/src/lib.rs:600). Additive JSON that legacy decoders tolerate, but either strip the singleton at the DTO boundary or correct the claim and the comment; the key-absence parity test (parity_protocol.rs:213-226) is built on a hand-crafted empty set and does not describe real spawn output. The claim does hold for policy values, path decisions, and params/SyncSession frames. - Two conventions under one field name.
Op::SyncSession's doc says "Additional workspace roots" while its only producer sends the full set (runtime_threads.rs:8260) — idempotent today only because the consumer re-normalizes.switch_workspacelikewise stores a full normalized set intoapp.workspace_roots(documented additional-roots-only,app.rs:1594-1597) and into session metadata, so /cd'd session files contain the duplicated primary; and thecore/session.rs:171-175doc comment says "additional" while the engine stores and renders the full set (engine.rs:4153,skip(1)). Align the convention or fix the docs. normalize_workspace_rootsdedups lexically, not canonically (core/src/lib.rs:71-79):/var/x+/private/var/xboth survive into the model-visible line and the writable-root enumeration. Every security consumer re-canonicalizes (verified — carve-out, sandbox policy,resolve_path), so there is no hole, but canonicalizing once at intake would close the whole class, including the pre-existing execpolicy lexical-matching gap this PR widens.- The round-3 must-fixes carry no red-green tests. Reverting the fork stamps, the
switch_workspaceswap, or theLoadSessionordering order breaks nothing today — the description's "locking regression test" claim covers only the round-2 surfaces. Given these are sticky disk-authority fixes, each deserves one test; the cache writeback inresume_thread_with_history(core/src/lib.rs:684) is likewise untested (a spawn → resume-with-roots → parameterless-resume sequence would pin it). - Disclosure drift: the "display paths hardcode
&[]" list is stale —underwater.rs:882andconfig/status.rs:282were fixed in890d5f61d, and the remaininglib.rs:12079is not a display path but headless enforcement (build_direct_workflow_tool), which silently denies the workflow tool write access to attached roots the session legitimately holds. Conservative, but the scheduled follow-up should target enforcement, not display. - Smaller: the
SyncSessionhandler records the action's set before the fallible provider-identity return (apply.rs:1282vs theOk(false)return at:1306) — same foreign-set class as round-3 item 3, replicating an accepted pre-existing shape; exec re-stamps the pre-normalization metadata set verbatim (exec_agent.rs:368→lib.rs:12454);ThreadMetadata.workspace_rootslacks#[serde(default)](latent,state/src/lib.rs:79); the round-3 commit message says the v5 migration "updates its localuser_versionmirror" while the change deliberately does the opposite (code and its commentstate/src/lib.rs:659-664are correct — the message is inverted); dead store infork's cached-metadata backfill (session.rs:232-240, cloned then unconditionally overwritten by the App stamp).
What checked out (for the record)
Allow-scoped execpolicy narrowing verified with no widening path on either matching path, and the three round-3 execpolicy tests are non-tautological (control + negative legs). The write carve-out judges relative targets against the primary only, matching ToolContext::resolve_path execution semantics, with no permission/execution split found in either direction; canonicalization covers all enforcement consumers. Resume three-state semantics and the persisted-cwd fallback removal are real root-cause fixes, structurally pinned; the active-turn fence covers roots with a strong test (persisted state untouched, 100ms negative-wait proves no op reaches the engine); SyncSession snapshot semantics hold structurally (single op-loop, all mid-turn reads frozen); compilation enforces coverage of all Op::SyncSession producers. The v5 migration is guarded and idempotent with consistent row plumbing; protocol/state serde defaults are correct and legacy payloads decode. Scope: zero out-of-topic functional code across 51 files. macOS: check/fmt/tests green.
890d5f6 to
28ba314
Compare
|
Thanks for the round-4 review — and for the macOS verification, which closes the last platform question. All three must-fixes are addressed in 28ba314 (pushed, CI green):
Non-blockings also landed: the SyncSession handler records the action's set only after the fallible provider-identity step; the dead cached-backfill clone in Two of your corrections are applied as well: the round-3 commit message no longer claims the user_version mirror update (the code deliberately does the opposite), and the body now reflects that the runtime thread fork inherits roots while Scheduled follow-ups as you listed: canonicalize once at |
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Thanks for the continued discipline — round 5 was a fresh end-to-end pass (protocol/persistence, core semantics, execpolicy/sandbox, carve-out/resolve_path, TUI carrier surfaces, runtime persistence, prompts/turn_meta, commit hygiene), with independent local verification on Windows: cargo check --workspace --all-targets green, cargo fmt --all --check clean, protocol (88) / state / core / execpolicy (122) suites and the forkguard (54) / carve_out (12) / sandbox::policy (15) / workspace_roots (16) / runtime_threads (157+2 ignored) filtered groups all green, plus CI green on 28ba314eb.
What holds up under re-verification: the layering is real; the single-root byte-identity invariant is true everywhere I checked (wire frames, SandboxPolicy, the carve-out predicate, and the execpolicy filters all reduce to the base expressions with an empty set); the resume fallback_cwd removal is a genuine root-cause fix (base re-persisted the process cwd on every parameterless resume); and commit hygiene is clean — commit-by-commit audit found zero smuggled changes (all 51 files serve the multi-root theme, nothing to split), DCO exact per commit, house style, "every commit compiles" spot-checked at 5 points, no debug leftovers/secrets/dependency churn, and the project_instructions label change confirmed absent (lives in #59).
Requesting changes for one durable root-set loss and three semantic gaps:
Must fix before merge
1. /cd never persists its root-set swap; the next autosave durably rewrites the stale pre-/cd set, and resume resurrects the abandoned directory as a writable root
switch_workspace(crates/tui/src/tui/ui/session_state.rs:642-649) performs the primary-swap in memory only — nothing on the path saves.- The autosave path stamps the live set (
build_session_snapshot,crates/tui/src/tui/ui/frame.rs:915) but then unconditionally merges disk over it (merge_persisted_lifecycle,crates/tui/src/session_manager.rs:1681:metadata.workspace_roots = persisted.workspace_roots). For any session that already exists on disk, the stamp is dead code and the stale set is what gets written — and re-written on every subsequent autosave. - Concretely: disk has
{workspace:/A, workspace_roots:[/A,/B]};/cd /Cruns with[/C,/B]in memory; autosave writes{workspace:/C, workspace_roots:[/A,/B]}; restart + resume re-normalizes to[/C,/A,/B]— the directory the user explicitly left is silently a writable additional root again and re-appears in the "Accessible folders:" line. This is the exact carrier-mangles-roots class rounds 2–4 closed, in the write-back direction, and it makes the round-3 claim that/cd"applies the same primary-swap semantics as the runtime PATCH branch" only half-true: the runtime lane persists its swap (crates/tui/src/runtime_api/sessions.rs:779stamp → directsave_sessionat:826, no merge), while/cdcannot persist at all. - The merge comment's own rationale ("a host that does not know about multi-root … must not erase the set another host persisted") no longer covers this case: post-round-2 the TUI is a roots-aware owner, and the merge cannot distinguish a roots-unaware writer from the owning App deliberately changing the set.
- The fix pattern already exists in this PR: the fork paths stamp and then save directly, before the disk-authority merge can re-erase the correction.
/cdneeds the same (a direct save on swap), or the merge must respect a non-empty incoming set from the live owner. Also missing: any test covering the/cd→ autosave → restart round trip; the only merge test (merge_persisted_lifecycle_restores_persisted_workspace_roots) cements the overwrite against a roots-blind rebuild, not against a legitimate TUI-side mutation.
Should fix
2. Runtime::invoke_tool seeds the exec-policy check with an empty root set — the app-server lane never matches attached-root ask/deny rules
crates/core/src/lib.rs:1398-1406 hardcodes workspace_roots: Vec::new() in the ExecPolicyContext, and the app-server bridge calls it in production (crates/app-server/src/lib.rs:717). Consequence: an ask/deny rule scoped to an attached root fires in the TUI/ACP/subagent lanes but silently never matches on the app-server lane — the same multi-root thread gets different approval behavior per dispatch path (under-prompting relative to the new semantics; no widening, byte-identical to base). The Known-scope section discloses the ACP/MCP registry ToolContext and build_direct_workflow_tool, but not this surface. Either thread the roots through invoke_tool (its cwd: &Path signature currently structurally cannot carry them) or add this lane to the disclosure with the scheduled follow-up.
3. Ask/deny scope and path filters match against independently-chosen roots, so a rule scoped to repo R can fire on a call under a different root
In matching_ask_rule (crates/execpolicy/src/lib.rs:468-517) the workspace-scope filter and the path filter are separate .filter()s, each with its own any() over the root list. A rule {tool, workspace: "/shared", path: "deploy/config.yaml", action: deny} matches a call to /workspace/deploy/config.yaml when cwd=/workspace and /shared is attached: scope matches via /shared, path matches via /workspace (the relative rule and the relative call normalize identically under the primary root). The workspace field is documented as "limits this rule to one repo", so this over-blocks outside the rule's repo. Direction is fail-closed (ask/deny only add prompts/blocks; Allow uses the primary root on both filters), but the semantics are wrong: matching should pair scope and path per root (rule matches root R iff scope matches R AND the path normalizes under R). No test covers a rule carrying BOTH workspace and a relative path, which is why this survives.
4. Worktree-isolated subagent children silently inherit the parent's attached root set
child_runtime() clones the full tool context including workspace_roots, and the spawn path swaps only .workspace (crates/tui/src/tools/subagent/mod.rs:9295-9309), so a worktree=true child's boundary becomes {worktree} ∪ parent_roots: it can resolve_path into attached roots and, under the carve-out posture, write there modal-free — at base a worktree child was confined to its worktree. No escape of the user-configured set (and the child is strictly more restricted w.r.t. the parent's primary root), but it contradicts the isolation feature's promise and extends the disclosed explicit-cwd: case without being disclosed. Either clear workspace_roots for worktree children, or pin the behavior with a test and a disclosure line. (Related coverage note: the three workspace_roots wiring hunks in tools/subagent have no subagent-layer test touching roots.)
5. The Some([]) explicit-clear leg of the resume contract is unpinned on the wire
parity_protocol.rs round-trips only Some([non-empty]) and absent→None. The three-state contract (crates/protocol/src/lib.rs:184-188) is only pinned in-process by resume_with_empty_roots_clears_back_to_bare_cwd, which never serializes. If a refactor changed the skip attr to drop empty vecs inside the Option (or added a []→None deserialize adapter), explicit-clear would silently degrade to inherit with every test green. One assertion closes it: {"kind":"resume","thread_id":"t","workspace_roots":[]} decodes to Some(vec![]) and re-encodes containing the key. (Nit in the same test: the assert message at parity_protocol.rs:130 claims the set "is serialized (non-empty)" but the encoded form is never asserted.)
Non-blocking (round-5 notes)
- core
ThreadManager::fork_threadkeeps the exact fallback-cwd anti-pattern this PR removes from resume (crates/core/src/lib.rs:757-783; the Runtime passes process cwd at:1158) and does not inherit parent roots — inconsistent with the TUI/runtime fork paths, which do. Pre-existing for cwd; roots are new, so inherit-by-default would have been free. normalize_workspace_rootsneither absolutizes nor lexically normalizes; a client sending a relative root via Start/Resume params stores it verbatim and it later resolves against process cwd at check time. Distinct from the disclosed symlink-canonicalization follow-up.- The running-branch roots override is cache-only; a later resume carrying
historyskips the running branch, resolves from the stale store, and overwrites the cache — reverting the override. Untested and undocumented (the writeback test pins only the parameterless path). fork_from_sessionstamps the source's set without re-normalizing against the fork's actual workspace (session.rs:154), so the durable record can haveroots[0] != workspace, violatingSessionMetadata's documented invariant. Consumers self-heal via normalize; one-line fix.save_current_session's update branch stamps new roots onto a possibly-stale workspace (sessions.rs:779never refreshesmetadata.workspace), amplifying a pre-existing staleness.- The new
workspace_changedgate (crates/tui/src/core/engine.rs:3505-3517) also skips the AGENTS.md re-read for a same-pathSetWorkspace— a base behavior change beyond multi-root (mid-session instruction edits no longer picked up when a host resends settings with an identical path). Unflagged. - The forkguard AGENTS.md lock test's second assertion ("byte-for-byte the same context") is trivially satisfied by the loader cache; a
project_context_cache::clear()between the loads (as the neighboring test does) would make it meaningful. SyncSessionfailure arms can leave the previous session's roots attached to the already-switched session (apply.rs:1286-1307; same half-applied-transition shape as messages/workspace). Disclose or guard.- Layer-before-action winner selection (
max_by_keyon(layer, action, specificity)) means a higher-layer ask spanning an attached root can outrank a lower-layer deny — a deny→ask downgrade. Latent today (production authors only User-layer rulesets), but "ask/deny only ever add a prompt or a block" is not strictly true; worth a PR-description line. get_writable_rootscanonicalization doesn't dedup (macOS/var/xplus a cwd under/var/xboth become/private/var/x); harmless today, pairs with the disclosed intake-canonicalization follow-up.- Cosmetic: stray blank lines inside struct literals at
apply.rs:1100,handlers.rs:1196,session_state.rs:604,provider_routes.rs:745,core.rs:184,undo.rs:269,session.rs:181/273/306/370.
Verified clean, for the record: disclosed scope boundaries (ACP registry ToolContext, build_direct_workflow_tool, Event::SessionUpdated carrying no roots, exec syncing only at startup, the execpolicy lexical-normalization gap pre-existing in base) all check out accurate against the code.
|
Thanks for the round-5 review — the Windows pass completes platform coverage, and the per-commit hygiene audit is much appreciated. All four items are addressed in 12b8867 (pushed, CI green):
Non-blockings landed: |
12b8867 to
33b23a4
Compare
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-6 review — a fresh end-to-end pass (protocol/persistence, core semantics, engine/turn lifecycle, execpolicy/sandbox/carve-out, the full TUI carrier graph, runtime-API persistence, prompts/turn_meta, design, and commit-by-commit hygiene), with independent re-verification on Linux at 12b8867b2: cargo check --workspace --all-targets green, cargo fmt --all --check clean, protocol (89 — the description's 88 is an undercount) / state (29) / core (89) / execpolicy (124) suites and the forkguard (54) / carve_out (13) / sandbox::policy (15) / workspace_roots (16) / runtime_threads (163+2 ignored) filtered groups all green. I also verified composition with the current pinvou3-clean tip (92427bd8d, i.e. after the #56–#61 batch): the merge is clean, cargo check --workspace --all-targets passes on the combined tree, #59's project_context changes coexist with this PR's without duplication, and the forkguard additions (6 here vs 74 on main) have zero name overlap.
What holds up under re-verification: all five previous rounds' must-fixes are genuinely fixed (the /cd swap + lifecycle merge rule with both-direction locking tests, the execpolicy scope+path one-gate pairing on the single matching path, worktree subagent root isolation restored to base semantics, and the Some([]) wire pinning). The no-roots byte-identity discipline holds everywhere I attacked it (empty-set wire frames, SandboxPolicy values, the carve-out predicate, execpolicy filters — Allow stays primary-root-scoped on every path). The carrier graph is now complete as far as I can enumerate it: every Op::SyncSession construction, every engine spawn, every fallible load path, and every HTTP save/resume/fork surface carries the real set; normalization has exactly one implementation. Commit audit: zero smuggled changes across all 51 files, DCO exact on all 17 commits, no dependency churn, no secrets, English comments throughout, and both "every commit compiles" spot checks pass. The design-level verdict also holds: base has no config key for sandbox writable roots (CLI sandbox run only) and its approval-side paths are cwd-only, so this PR is a genuinely new capability axis, not a reinvented wheel.
One item to address before I can approve:
Must fix
1. The workspace_changed gate in the Op::SyncSession handler is an undisclosed single-root behavior delta
crates/tui/src/core/engine.rs:3490 and :3508. Base reloaded the project context unconditionally on every Op::SyncSession; this branch reloads only when workspace_changed. Concretely: on same-workspace re-sync edges (undo/backtrack re-syncs, provider re-syncs, runtime re-syncs), a single-root session no longer re-reads AGENTS.md from disk — mid-session instruction edits are picked up only on a workspace change or engine respawn. That is a behavior change for every session, not just multi-root ones, and it contradicts the PR's own no-roots ⇒ base-identical discipline. The accompanying comment ("Project context derives from the primary root only; an additional-roots update must not re-read it") describes a narrower gate than the code implements — the gate also suppresses the reload on roots-blind, same-workspace syncs that base always performed. No test locks either direction.
Requested fix: revert to the unconditional reload. It is one line; a roots-only sync re-reading identical content costs one cheap disk read, and roots-only syncs are new territory with no base contract to preserve. If you strongly prefer keeping the gate, it must be disclosed in the description and locked with a test — but the revert restores exact base identity and is my recommendation.
Body corrections (no code changes)
- "With no roots configured, wire frames … are byte-identical to single-root behavior" is overstated. It holds for params and legacy records, but not for frames this build emits: a thread created by this build always carries at least
[cwd], so Thread DTO andOp::SyncSessionframes gain theworkspace_rootskey (additive, legacy decoders tolerate it — exactly what the DTO comment now correctly states). Please narrow the claim in the Behavior boundary section to match. - Round-5 section: "fork_from_session re-normalizes the stamped set against the fork's actual workspace" is inaccurate. The stamp at
session.rs:153-155clones the source's set verbatim; re-normalization happens transitively in the engine onSyncSessionand on the next autosave. Please reword. - The
Runtime::invoke_tooldisclosure comment says "under-prompting only" — a Deny rule scoped to an attached root would also never fire on that lane (under-blocking). The lane cannot attach roots today, so the delta vs base is genuinely zero; please reword to "attached-root ask and deny rules never fire" for accuracy.
Non-blocking (for the record / follow-up candidates)
/cdat a bare prompt (no session yet) unconditionally saves and mints an orphan session file per invocation (session_state.rs:655-673; the engine-sideOp::SyncSessionright below is guarded by!app.api_messages.is_empty(), the save is not). Acurrent_session_id.is_some()guard (or adopting the minted id) closes it.- Cross-workspace
/fork <id>stamps the source's raw set, so the durable record transiently violates primary ∈ roots. The code comment says "same accessible set", so this reads as intended — but it diverges from the/cd/ PATCH /resolve_resume_rootsprimary-swap semantics; worth pinning as the documented choice. PUT /v1/sessionsupdate branch stamps fresh roots over the staleworkspace(the workspace omission is pre-existing at base, but the mixed-vintage record lets a later resume silently revert a PATCH'd primary; one line:updated.metadata.workspace = snapshot.workspace).- No App-layer test locks the seeding order — the exact layer five rounds converged on. The engine and storage layers are well locked.
- Commit messages
003bb4d39/2ec068ed7describe aWORKSPACE_ROOTS_SYMBOLanchor that does not exist anywhere in the branch, and635a47822's "fork flow inherits the parent's roots" only became true indfc5866c1. Optional to fix (a squash merge makes it moot), worth acknowledging. - The macOS pass on the final tree is still outstanding, as disclosed.
Once item 1 and the three body corrections land, I'll approve.
|
Thanks for the round-6 review — and for the composition check against the post-#56–#61 tip. The must-fix and both wording corrections are addressed in 9abaa87 (pushed):
Non-blocking items acknowledged for the follow-up list: the bare-prompt Verified on Linux at 9abaa87: |
…fy base 本分支与 base `feat/projects-unify` 各自独立地包含了同一套 projects-unify 血统（各自 rebase 并演化），因此普通合并产生 457 处冲突 / 约 11000 行冲突 代码。按“以 base 的演化为基线、只移植本分支独有功能”的方向解析： - 树以 base 94f07b3 为准（含主线 #445/#460/#462/#471/#473/#530 与第 3-6 轮 review 修复），丢弃重复血统的旧实现。 - 重新移植本分支独有的“单一入口工作区”功能（P1）：工作区选择器弹窗、 keychain 胶囊与对齐命令、管理文件夹面板、文件夹项目自动物化、 反物化排除表（never-materialize）、项目记忆主文件夹（last_primary_root）、 create_session/create_codex_acp_session 携带 cwd + 钥匙串快照。 - 语义随之更新（并同步改写锁定旧语义的测试）：跨项目 root 重叠合法化 (§9.9)、删除项目写显式移出 tombstone、根移除时移出自动成员、 仅剩 tombstone 时保留 projects.json。 - CodeWhale 指针保留 9abaa87e6（= Pinvou/CodeWhale#54 的 head）：本功能依赖 底座 workspace_roots 多根 API，base 钉的主线 92427bd8d 不含该字段， 否则父仓无法编译。待 #54 合入 pinvou3-clean 后重钉。 - web 边界：钥匙串快照与 workspace_binding 同一套降级，逐项只留末级目录名。 验证：cargo check --lib --tests 通过；cargo test --lib 2241 passed / 2 failed （余下 2 个 legacy-table 权限用例在未修改的 base 上同样失败：测试进程有 root 级写权限，0o555 目录挡不住写入）；cargo fmt --check 干净； python3 scripts/architecture-guard.py 通过。 Signed-off-by: qiyue <1461574375@qq.com>
…fy base 本分支与 base `feat/projects-unify` 各自独立地包含了同一套 projects-unify 血统（各自 rebase 并演化），因此普通合并产生 457 处冲突 / 约 11000 行冲突 代码。按“以 base 的演化为基线、只移植本分支独有功能”的方向解析： - 树以 base 94f07b3 为准（含主线 #445/#460/#462/#471/#473/#530 与第 3-6 轮 review 修复），丢弃重复血统的旧实现。 - 重新移植本分支独有的“单一入口工作区”功能（P1）：工作区选择器弹窗、 keychain 胶囊与对齐命令、管理文件夹面板、文件夹项目自动物化、 反物化排除表（never-materialize）、项目记忆主文件夹（last_primary_root）、 create_session/create_codex_acp_session 携带 cwd + 钥匙串快照。 - 语义随之更新（并同步改写锁定旧语义的测试）：跨项目 root 重叠合法化 (§9.9)、删除项目写显式移出 tombstone、根移除时移出自动成员、 仅剩 tombstone 时保留 projects.json。 - CodeWhale 指针保留 9abaa87e6（= Pinvou/CodeWhale#54 的 head）：本功能依赖 底座 workspace_roots 多根 API，base 钉的主线 92427bd8d 不含该字段， 否则父仓无法编译。待 #54 合入 pinvou3-clean 后重钉。 - web 边界：钥匙串快照与 workspace_binding 同一套降级，逐项只留末级目录名。 验证：cargo check --lib --tests 通过；cargo test --lib 2241 passed / 2 failed （余下 2 个 legacy-table 权限用例在未修改的 base 上同样失败：测试进程有 root 级写权限，0o555 目录挡不住写入）；cargo fmt --check 干净； python3 scripts/architecture-guard.py 通过。 Signed-off-by: qiyue <1461574375@qq.com>
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Fresh full re-review of the round-6 tree (head 9abaa87; merge-base is the current pinvou3-clean tip 92427bd, so the branch already sits on latest main and no rebase was needed). Verified locally on macOS: cargo check --workspace --all-targets green, cargo fmt --all --check clean, and every listed suite green (protocol 89, execpolicy 124, tui forkguard 72 / carve_out 13 / sandbox::policy 15 / workspace_roots 16 / runtime_threads 163) — this also closes the outstanding final-tree macOS pass noted in the Tests section. The core/state/engine/execpolicy layers hold up: the six prior must-fix rounds landed for real, the execpolicy single-gate rewrite is sound, the three-state resume contract is the right design, and the commit chain is thematically pure (all 18 commits on-theme, zero smuggled changes, DCO clean).
Requesting changes for three remaining carriers of the PR's own defect class, plus non-blocking items.
Must-fix
M1 — the app-server bridge executes every turn single-root, even for multi-root threads. create_runtime_thread (crates/app-server/src/lib.rs:1528-1550) creates the turn-executing runtime thread via POST /v1/threads with only {model, workspace, mode, archived}; RuntimeThreadHint (:205-208) and record_stdio_thread_hint (:1332-1341) persist only model + cwd. run_bridged_turn (:1134) is documented as "the only way any app-server surface runs a model". So a thread/start declaring workspace_roots stores the full set on the parent record, and then every actual turn runs on a child thread materialized with [cwd] only: writes under attached roots are denied/prompted while thread/read still shows the declared set — a silent mismatch. This is the same carrier class fixed across rounds 2-6 and is not covered by any disclosed empty-root lane (invoke_tool / ACP registry / workflow tool are separate surfaces). CreateThreadRequest already accepts the field, so the fix is plumbing the hint through.
M2 — /save drops the root set, durably in two common shapes. save (crates/tui/src/commands/groups/session/session.rs:20-105) builds fresh metadata via create_saved_session_with_mode (empty roots), stamps provider route/cost/artifacts/work_state/auto-route — but not app.workspace_roots — and then switches app.current_session_id to the roots-less copy. The managed-dir copy heals only if a later autosave runs; /save → quit is a durable single-root record (shutdown only flushes queued actor writes, it never builds a final snapshot), and an explicit-path /save is never healed by autosave at all. exec --resume of such a copy then runs single-root and stamps the empty set back (stamp_exec_session_metadata, crates/tui/src/lib.rs:12462). Same class as the round-4 CLI-fork fix: stamp before the write, exactly like the fork paths.
M3 — the protocol fork lane inherits the provider but not the roots, and the wire cannot express "inherit". Runtime::fork_thread (crates/core/src/lib.rs:757-786) inherits model_provider when absent and falls back for cwd, but passes ¶ms.workspace_roots through verbatim — a bare thread/fork (the historical shape; the field is new) silently degrades a multi-root parent to [fallback_cwd]. Reachable from the app-server stdio surface (thread/fork → ThreadRequest::Fork, crates/app-server/src/lib.rs:2046). ThreadForkParams being a plain Vec leaves no way to say "inherit", while the parent record is loaded in the very same function — the body's rationale that "there is no persisted set for the distinction to matter" is not accurate. Either inherit-when-absent (mirroring the provider) or switch to Option<Vec> like Resume; at minimum, correct the disclosure.
Non-blocking (fix or disclose)
- N1 —
/fork <id>cross-workspace re-admits the source's abandoned primary:fork_from_sessionstamps the source set verbatim (session.rs:154) while the fork's workspace is the current one; engine-side normalization yields[current, source-primary, ...], and the disk record persistently violates the workspace-is-primary invariant (session_manager.rs:161-167). This contradicts the primary-swap semantics the PR itself implements in/cdand PATCH. The round-5 body wording ("re-normalization ... transitively") discloses the mechanism but not this consequence. - N2 —
/rename//titlesyncmetadata.workspacefrom the live App but not the paired roots (rename.rs:117; the pre-first-snapshot fallback constructor at rename.rs:158-166 also omits them). After a/cdwhose direct save failed, a subsequent/renamewritesworkspace: newwith the stale pre-switch set — resurrecting the abandoned directory on the next resume. - N3 —
/resume <export>re-pointscurrent_session_id/api_messageswithout resettingapp.workspace_rootsor syncing the engine (resume.rs:113-116); the next autosave stamps the previous session's set onto the imported session's record (the engine desync itself is pre-existing; the persisted roots bleed is new). - N4 — repo law stays primary-root-only (turn_loop.rs:3835 passes
session.workspace; repo_law.rs:183 strips that prefix), so workspace-anchored constitution globs never fire on attached-root writes — under-prompting, and unlike the AGENTS.md decision there is no disclosure comment or pinning test. - N5 — the
Op::SyncSessionapply path mutatesapp.workspacebefore the fallible provider step but records roots only after it (apply.rs:1284 vs :1316): on a provider-restore failure the next re-sync sendsworkspace: newwith roots whose primary is the old directory. - N6 — the state write path degrades roots to
'[]'silently on serialization failure (state/src/lib.rs:730-733), while the reader logs a warning; non-UTF-8 paths lose the set with no visibility. - N7 — test-evidence gaps beyond the disclosed TUI-seeding follow-up: the exec stamp test passes roots but asserts nothing about them (crates/tui/src/lib.rs:16278 — deleting the stamp line stays green); the PUT
/v1/sessionsOk-branch stamp is satisfied vacuously (the POST stamped the file first, runtime_api/tests.rs:4355); the worktree-subagent root clearing (subagent/mod.rs:9325-9333) has no test.
Body corrections needed before merge
- "a golden frame in the AGENTS.md lock test" (round 3 non-blocking list) is a phantom item: the lock test is real, but no golden frame exists anywhere in the PR and the round-3 commit never touches project_context.rs.
- "the v5 migration updates its local user_version mirror" states the opposite of the code: the v5 block deliberately does not touch the local mirror (state/src/lib.rs:660-663, following the v4 precedent; the round-3 commit message itself says "deliberately does not touch").
- The verification paragraph ("The reviewer confirmed the round-3 tree green on macOS ... The round-2 tree awaits one more macOS pass") is internally contradictory and now outdated — the round-6 head is verified green on macOS (check/fmt/all listed suites) as noted above.
- Commit messages: 5ba7666 and 6c20ae2 cite a
WORKSPACE_ROOTS_SYMBOLconstant that exists nowhere in the PR; 4c3a7ea claims a golden frame that landed nowhere; 33b23a4 claimsfork_from_session"re-normalizes the stamped set" — the diff only stamps verbatim there.
Nothing here changes the assessment of the foundation itself: the design is sound, and this carrier tail is exactly the class this PR has been closing round by round. M1-M3 plus the body corrections are what stand between this and a merge from my side.
9abaa87 to
88405ff
Compare
|
Thanks for the round-7 pass — M1–M3 and the non-blocking tail are addressed in Must-fix
Non-blocking
Body correctionsThe phantom "golden frame in the AGENTS.md lock test" and the inverted "v5 migration updates its local Verification (head
|
88405ff to
98b87d5
Compare
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Fresh full re-review of the round-7 tree (head 88405ffbc). The branch had fallen behind pinvou3-clean by two commits (#62, #57); I rebased locally onto the current tip ce783728c — zero conflicts, and the rebased tree passes cargo check --workspace --all-targets and cargo fmt --all --check. The round-7 remediation is real: the bridge hint carries the response thread's roots through create_runtime_thread (empty set stays off the wire), the /save stamp is pinned by a test I re-verified red-green (deleting the stamp line fails with left: []), and a bare thread/fork genuinely reads the parent record it already loads, filters the parent primary, and re-normalizes with the fork cwd in the lead. The commit chain stays pure — 23 commits, all on-theme, DCO clean, no phantom references, no non-Rust files, per-commit compiles spot-checked — and the Known-scope-boundaries disclosures match the code.
Requesting changes for one remaining carrier of the PR's own defect class, one standard-consistency item, and two body corrections.
Must-fix
M1 — /title still drops the live root set on its main write path; the round-7 claim that both commands were fixed is not accurate. Round 7 records "/rename and /title synced metadata.workspace from the live App but not the paired roots ... both paths now carry the live set." Only the rename.rs paths carry it (rename.rs:122-125 main path, rename.rs:177-180 first-snapshot fallback). /title is a separate mirror implementation, title.rs set_window_title_with_manager, and it still does exactly the pre-fix shape: loads the disk record via manager.load_session (title.rs:115), overwrites session.metadata.workspace.clone_from(&app.workspace) (title.rs:162), and saves (title.rs:167) — grep workspace_roots title.rs has zero hits; the file is not in the PR diff at all. (Its NotFound fallback delegates to the fixed rename::live_session_before_first_snapshot, so only the loaded-from-disk path is broken.) The exposure is the one rename.rs's own fix comment describes: /cd whose direct save failed only posts a status message and continues (session_state.rs:662-679), so the disk record keeps the pre-switch set; a /title before the next autosave rewrites workspace: new next to the stale pre-switch set, /title then quit is durable (no turn checkpoint heals it), and the next resume re-admits the abandoned directory as a writable root through normalize_workspace_roots. Every other disk writer in this PR stamps (/save, both forks, autosave, picker rename, /cd direct save); /title is the last gap. Same fix as rename.rs:123-125.
M2 — ACP session/load drops the persisted root set: same shape the PR itself must-fixed twice, and the only reason it reads as acceptable is the body disclosure. session/load takes only the cwd from the loaded record (acp_server.rs:1581) and build_acp_tool_registry (acp_server.rs:2114-2116) never calls with_workspace_roots — the only production call site of that builder is engine.rs:6724 — so the registry context the ACP ask/deny checks (:703, :712), repo law (:752-757), and auto-review gate (:730-737, literal &[]) read is permanently empty. Since ACP shares the session store, session/load can resume a multi-root session created by the TUI or the Runtime API; the attached-root writes that session legitimately held then fail with PathEscape, and an attached root's constitution never holds there. The body discloses this lane ("populating them needs the load_session roots plumbing"), but rounds 4 and 7 treated the identical shape — exec --resume, then the session picker and startup --resume — as must-fix and fixed it. Please either (a) land the plumbing now — feed saved.metadata.workspace_roots through with_workspace_roots when building the registry in session/load, mirroring the engine — or (b) keep it as a disclosed follow-up but justify the inconsistency in the body and add the same site comment Runtime::invoke_tool has, including the user-facing consequence (a multi-root session resumed over the IDE bridge silently loses its attached roots). Option (a) is a few lines; if there is a reason it is harder than that, the body should say what it is.
Body corrections needed before merge
- Tests section:
forkguard (72)— measured on this head:cargo test -p codewhale-tui --lib forkguard= 79 passed (base 73 + 6 new). 72 matches no point in the chain; it is a stale number from an earlier round. - Tests section: the filter
commands::groups::session::session::sessionmatches 0 tests; the correct filtercommands::groups::session::sessionmatches exactly 31 (the number itself is real). - Round-7 remediation section: "
/renameand/title... both paths now carry the live set" — see M1; only/renamewas fixed.
Non-blocking (fix or disclose)
- N1 — the running-cache resume override is cache-only and can be silently reverted in-process. The cache branch writes the resolved cwd/roots back to
running_threadsbut never persists (core/src/lib.rs:669-685, comment: "(Persistence aligns with the autosave path, as for cwd.)"), while a subsequent history-carrying resume bypasses the cache (:669params.history.is_none()), reads the stale DB row, and re-inserts it into the cache (:703-713) — the earlier override is undone with no warning, and a process restart loses it too. Persisting in the cache branch (the persisted branch already callspersist_thread) or documenting the boundary would do. - N2 — the AGENTS.md primary-root forkguard test is hollow at runtime:
project_context.rs:1305-1344only ever calls the loader with the primary path; the attached root never enters any exercised function, so its assertions cannot fail. The real lock is the loader's&Pathsignature (compile-time), which is fine — but the test name promises runtime evidence it does not contain. - N3 — a relative-path write target is judged against every root's constitution (
repo_law.rs:73-98,push_normalizedkeeps the relative tail per root), while execution resolves relative paths against the primary root only (spec.rs:1088-1092) — so an attached root's law can hold a write that physically lands under the primary. Fail-closed, andstrongest_hold_wins_across_rootspins it as intentional, but the doc atrepo_law.rs:44-51("a root's constitution holds writes under that root") is narrower than the pinned behavior. - N4 —
Op::SyncSession.workspace_rootshas no wire-level parity case;every_variantonly counts it with an empty vec, so a future change to itsdefault/skip_serializing_ifattributes would not be caught (Start/Resume/Fork/DTO are all pinned; this one is not). - N5 —
build_direct_workflow_tool's empty roots (tui lib.rs:12034) and the ACP empty context have no disclosure comment, unlikeinvoke_tool(core/src/lib.rs:1422-1427). Moot for ACP if M2 lands as option (a). - N6 —
normalize_workspace_roots's doc says "duplicates removed" unconditionally; the symlink dual-spelling residue (/var/x+/private/var/xboth survive) is only disclosed in the body's follow-up list. One doc line would close it. (Trivial:25a655669is typeddocs(tui)but carries a behavior-locking test.)
The foundation itself remains sound from my side: the three-state resume/fork contract, the allow-primary-only structural short-circuit, the scope+path single gate, and the per-root repo-law namespace are all verified with negative + control tests, and this round's delta contains no new carrier beyond M1. M1, the M2 decision, and the three body corrections are what stand between this and a merge from my side.
Verified locally on the rebased tree (macOS): cargo check --workspace --all-targets green, cargo fmt --all --check clean, and the body's suite numbers reproduced exactly for protocol (91), core (96), state (29), app-server (106), carve_out (13), sandbox::policy (15), workspace_roots (17), runtime_threads (163), repo_law (15), and session (31) — the two exceptions are the corrections above.
|
Thanks for the round-8 pass — the must-fix, the M2 decision, and the non-blocking tail are addressed in Must-fix
Non-blocking
Body corrections
Note on the push: the branch was sitting on a detached HEAD at |
0d855be to
0aea9fe
Compare
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-15 review — fresh full re-review of head a1bd01ae3 (unchanged since round-14)
Method. The head is byte-identical to the round-14-reviewed tree, so this round had two jobs: (1) independently re-verify every round-14 blocking/major claim before your fix round builds on it, and (2) another from-scratch hunt with new shard angles — specifically the round-14 B14-1 class (semantically coupled files that were never in the diff). Thirteen parallel shards: shell.rs trust-boundary verification, /cd verification, three not-in-diff coupled-file sweeps (tools/, core/engine/authority/, TUI/session), adversarial root-set semantics, concurrency/lifecycle interleavings, static mutation-testing of the test suite, base-parity per-hunk comparison, i18n sweep, body/docs claims audit, commit hygiene, and a hunk-level smuggling falsification pass — plus my own line-level verification of every new blocking/major finding below.
Round-14 re-verification: all four claims stand (one erratum)
- B14-1 (Blocking) confirmed on all three legs, including the base fail-closed comparison (shell.rs is byte-identical base↔head). Added evidence from re-verification: pipeline commands run under a real shell whose child PATH retains attached-root dirs, so pipeline stages reproduce the program-resolution and PATH legs without going through
resolve_readonly_program; execpolicy is text-pattern-only and cannot catch any of it. One scope note: the OS sandbox layer also has zeroworkspace_rootsreferences (WorkspaceWrite writable roots derive fromspec.cwd) — a fail-closed mirror of leg 3 in a different layer; disclose it, don't fold it into this fix silently. Fix-shape traps: the normalizer is deliberately lexical, so threading its raw output into the shell checks leaves the symlink-spelling hole open — canonicalize per root with raw fallback exactly likeToolContext::boundary_roots(); strip Windows verbatim prefixes; the two directions are opposite (deny program resolution/PATH retention under attached roots, allow operands/cwd) and need separate pins, including a pipeline-stage pin. - M-1 confirmed. One mechanism nuance: the op channel is bounded, not unbounded — irrelevant, since
reserve()with free capacity doesn't yield and the toast promoter runs in the same task. Classification makes it worse: the failure receipts classify Error/sticky while the finalWorkspace: …classifies Info/4s, so on double failure the user gets a success-looking toast. Display wiring remains untested (only the pure composer is pinned). - M-2 confirmed with an erratum to my round-14 text.
:761("Failed to snapshot workspace switch") is absent fromMessageIdand all 15 packs, as claimed.:765is not an absence:MessageId::SessionsDirectoryFailedexists and is translated in all 15 packs — the code bypasses it (and the en template has no{error}placeholder, so a call-site-only fix would drop the error detail). The breach stands on both lines; the fix differs per line. - M-3 confirmed, with a refinement: the live claim rides the orphan only until the next snapshot — the first prompt after a bare
/cdmints a second session and re-points the claim, so the 409 window is/cd→next snapshot; the 0-message "New Session" orphan file itself is permanent in picker and Runtime API either way. Every sibling mint-and-adopt path was checked; only/cdfails to adopt.
New Blocking
B15-1 — repo law judges roots under their raw stored spelling only, while execution and the carve-out accept canonical aliases: a symlink-alias spelling silently bypasses an attached root's constitution and writes modal-free
repo_law_plan_decision loops the raw normalize_workspace_roots output; push_normalized (repo_law.rs:216) does path.strip_prefix(root) on the raw root, and the .. re-entry leg added by the round-13 B2 fix (repo_law.rs:245-255) is lexical too — no canonicalization anywhere in repo law. Meanwhile the carve-out accepts canonical aliases (carve_out_target_allowed strips against workspace_lexical or workspace_canonical_lexical, then reality-checks against workspace_canonical, authority.rs:580-601) and resolve_path contains via candidate_canonical.starts_with(root_canonical).
Trace (verified line-by-line): primary /w (git), attached root stored as /alias → symlink → /real (git, constitution blocks vendor/**). Target /real/vendor/lib.rs: repo law's tail for root /alias becomes real/vendor/lib.rs — vendor/** never matches, no hold; the carve-out passes via the canonical arm (.git stats fine through the symlink, root_canonical = /real, lexical gate passes against /real) → modal-free; resolve_path contains it → the write lands, and the block-class invariant was silently skipped. The reverse direction (root stored canonical, target spelled via the alias) bypasses identically. This is realistic on macOS (/var→/private/var, /tmp→/private/tmp).
The single-root shape of this exists at base, so it is not strictly a regression — but this PR writes the contract ("a root's constitution holds the writes that land under it", repo_law.rs:44-48) and extends the broken boundary to attached roots whose spellings arrive un-canonicalized from clients (normalize_workspace_roots is deliberately lexical; /cd canonicalizes, PATCH/resume/fork do not). This is the same root-cause family as B14-1: the boundary must judge the spelling execution accepts. Fix: canonicalize per root in the repo-law loop (mirroring boundary_roots()), judge both spellings, pin with a symlinked-root test.
New Majors
M15-2 — an empty-string root, accepted by every intake, nulls resolve_path's containment for reads in every posture
start_thread (runtime_threads.rs:5447-5448) and the PATCH arm (:5849+) normalize without per-entry validation — workspace has a not-empty check, roots entries have none. "" survives normalize, is persisted to the v5 column, and reaches boundary_roots() as ("", "") (canonicalize fails → raw fallback). Path::starts_with("") is true for every path, so both the escape check and the existing-path re-check pass for any absolute path: read_file (approval Auto — never prompts in any posture, including Never) reads arbitrary filesystem paths subject only to the filename denylist. Writes stay fail-closed (the "" root fails the carve-out's .git stat). The TUI cannot produce this; app-server/runtime-API clients can. Fix: reject empty (and relative) entries in normalize_workspace_roots — the single chokepoint every consumer already routes through — plus a pin.
M15-3 — revert_turn //undo snapshot and restore the primary only, then report "Workspace files reverted" while attached-root writes persist
Capture is primary-bound (turn_loop.rs:4728 clones session.workspace → pre_tool_snapshot, turn.rs:398 → SnapshotRepo rooted at the workspace), and so is restore (revert_turn.rs, undo.rs open the repo at context.workspace/app.workspace). This PR makes attached-root writes modal-free and advertises them: a turn that writes /repo-a/f1 (snapshotted) and /repo-b/f2 (not) reverts only f1 and reports success — model and user are told everything rolled back. Sibling surfaces compound the false completeness: /export and the activity-detail checkpoint timeline describe only primary restore points while claiming "every new turn records another restore point"; the /relay handoff packet and export header omit the root set entirely (the archive manifest does carry it). Fix direction: extend snapshot/restore to the root set, or report the boundary of what was restored.
Docs must-fix (one-liners; both falsehoods are added by this PR's own diff)
- AUTHORIZATION_ORDER.md:44-45 — "a scoped
allownever auto-approves a write under an attached root" is false as an absolute: the absolute-path fallback (execpolicy/src/lib.rs:533,:1215-1227) is root-independent, and the body repeats the same overstatement. Narrow (exact-path match only), but it is a fail-open direction on a security-claim sentence. Qualify it in both places. - RUNTIME_API.md:605-607 — attributes a tri-state
workspace_rootsrequest field to HTTP/forkand/resume, which take no request body (runtime_api.rs:4339-4361); a client sending{"workspace_roots": []}silently gets plain inheritance. Point the tri-state at the app-server ops and PATCH.
Should-fix (new this round)
- Elevation retry drops the session's attached roots:
elevation.rs::to_policybuildsWithNetwork → workspace_with_network()(no roots at all) andWithWriteAccess(paths) → paths + base_cwd, called with&app.workspace. A sandboxed call denied for network that also legitimately writes an attached root gets a retry policy that denies that write, nudging toward FullAccess. Fail-closed, but a PR-introduced divergence from the turn policy. - "Always allow" can never save for attached-root writes:
overlays.rs:494builds the ApprovalRequest against the primary;normalize_workspace_relative_pathreturnsNonefor attached-root absolutes so the ask-rule set comes back empty — and one attached-root path in a multi-file patch discards the rule set for the whole patch (ask_rules.rs:193-215early return). Every attached-root write re-prompts forever under Ask. - The
/cdfailure path is reachable in-process and invisible:build_session_snapshotfails while Work state is mid-publish (frame.rs:886-891), landing in the clobbered:761leg, while:787-789unconditionally appends "Switched workspace to X" to the transcript — neither status bar nor scrollback ever records the failure. - The
:638remainder underflow has a concrete path:event_loop.rs:593assignsapp.workspace_rootsstraight from a disk record without normalizing before rendering the notice. Switch to the sibling/statussaturating_subform. /cdguards onlyis_loading;/clearusessession_transition_blocked(compaction + queued tasks). A/cdduring idle compaction persists the new set and shuts down the engine mid-compaction, losing the result. Align the guard.- Bridge hint-change PATCH against an active runtime thread fails the whole next bridged turn with an opaque error (app-server.rs:1557-1595 → runtime_threads.rs:5905). Fail-closed is the right direction; consider queueing the PATCH until the turn settles.
- Test gaps that would hide a B14-1-class regression elsewhere: the engine's exec-policy glue (engine.rs:8116-8119 — every engine-level test passes
&[]), the auto-review bounded-write plumbing (ctx_forpassesNone, &[]), and the turn-loop repo-law call sites are pinned only one layer down; a mutation to&[]at any of them survives the whole suite. Also unpinned: the runtime-PATCHSome([])clear/evict leg, relative-path resolution under attached roots at the execution layer, and the notice's cap branch. (One quasi-nominal pin acknowledged:forkguard_workspace_roots_instruction_discovery_takes_only_the_primary_root— "the lock is the signature".) - Disclosure-list additions still absent from the body: image_analyze, read_lints, project_map, workflow
source_path, subagentresident_file+ cwd gate, @-mention/working_set, ACP non-disclosure,/export//relay/activity-detail primary-only; plus the undeclared behavior changes (resume keeps persisted cwd; bare fork anchors parent cwd) and theengine.rs::string_fieldtrim removal, which is load-bearing for the raw-spelling alignment and carries no comment.
Record corrections / credit
- Round-13's "83bf482e3's message is missing SF8" is disproven in its absolute form: the message contains "S8: qualify the state-crate warn comments…" — what it omits is the
/cdpersistence-actor item, which the body's existing SF8/S10-swapped admission already covers. No action. - The body's "20,636 registration basis" is reproducible and verifiably pinned (parent Hmbown#484's head tree pins
bbc90540a); round-14's skepticism on that number is withdrawn. Still stale: "~14.3× at 21,400" (head = 21,755 ≈ 14.5×) and "Hmbown#484's gitlink keeps pinning this branch's head" (it pinsbbc90540a, two commits behind). - Commit hygiene re-verified: DCO 43/43, linear, author=committer, no secrets/easter eggs/debug leftovers, no dependency/CI/lockfile churn;
a1bd01ae3's message fully describes its diff. - Smuggling re-falsified at hunk level: no weakened or deleted assertions (all four removed
assert!s accounted for), locale packs exactly 6 keys × 15 with zero other churn, no whitespace-masked semantics. Two defensible split-out candidates if you prefer: the 16 MiB-stack test-infra conversion (acp_server.rs, body byte-identical to base) and thePUT /v1/sessions409 live-session guard — both disclosed and pinned in place. - Concurrency core is sound: PATCH is serialized against turn start (lock order, active-turn bail, execution-state re-check including roots), the engine never drains ops mid-turn so approval and execution always read one set, the v5 migration is
BEGIN IMMEDIATE-safe, and engine double-spawn loser detection includes roots. No concurrency blocking issue found. - i18n beyond the two known strings is fully clean: 6 keys × 15 packs placeholder-exact, no dead keys, parity CI-gated; the turn-metadata English is deliberate prefix-cache stability.
Four-criteria verdict
- Real value / root cause: at risk. The mechanism is real and deep, but the root-cause discipline — every consumer of the boundary judges the same spelling and the same set that execution accepts — now has three open violations: shell.rs (B14-1), repo_law.rs (B15-1), and intake validation (M15-2).
- Elegance: PASS (unchanged from round-14).
- Smuggling: PASS (re-falsified at hunk level; two optional split-outs noted above).
- Defects: FAIL — B14-1 and M-1/M-2/M-3 stand; B15-1/M15-2/M15-3 are new; the two doc falsehoods are must-fix.
Convergence: one canonical-spelling fix pass (B14-1 + B15-1 + M15-2's intake validation, plus the plumbing pins from should-fix 7), the /cd trio (M-1/M-2/M-3), M15-3 (revert/undo honesty), the two doc one-liners, and the body refresh (figures, disclosure list, behavior declarations). Everything else above is should-fix/disclosure and can ride the same or a follow-up round.
Round-14 addressed (head
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-15 review — fresh full re-review of head 9f5f4d34a (the head advanced twice during this round)
Method. Another from-scratch pass, not a delta check: 28 parallel review shards over the full range — protocol wire, state/SQLite, core resume/fork, core PATCH/remap, app-server, engine/collectors, authority/spec/execpolicy, repo-law, runtime-threads, session manager/commands, TUI UI layer, i18n, ACP/relays, runtime API, subagent/worktree, docs accuracy, value/root-cause, elegance, commit hygiene, body-claims audit, lineage/DCO forensics, test quality, adversarial security, concurrency, consumer completeness, parent-repo coordination, base-parity regression — plus independent verification of every open round-14 finding. The review tree was refreshed to the final head (7bbf38fc76 16:44Z, 9f5f4d34a 17:08Z), and I read the full 875-line round-14-remediation delta line by line myself.
First, the credit — the round-14 remediation is genuine, verified on the final tree:
- B14-1 CLOSED.
canonical_readonly_rootsnow backs all three Scout legs — program resolution, PATH sanitization, operand/cwd gate — spanning the full declared root set: a planted attached-root executable is rejected, attached-only PATH fails closed, operands under attached roots are admitted. Both pins drive the real functions with real fixtures. - M-1 CLOSED structurally: the receipt is returned by the persist step and composed into the closing line, so the clobber is impossible by construction; pinned by
closing_status_carries_the_receipt. - M-2 CLOSED:
WorkspaceSwitchSnapshotFailed/WorkspaceSwitchSessionsDirFailedpresent in all 15 packs (verified per-pack), render pin included. - M-3 CLOSED: the persist step guards on
current_session_id;bare_cd_persists_nothing_and_mints_no_orphanpins it. The clippy follow-up (9f5f4d34a) is provably behavior-neutral. - SF-2/SF-3/SF-6 closed; the body now carries the consumer-hunt list (line 40), the single-root behavior changes (line 41), and the N3 remainder (line 42).
- Metadata reproduced: 45/45 commits author=committer=qiuYliangM with one matching sign-off, strictly linear, chain root =
6f780290f, behind 0; final drift 78 files +5882/−373 matches the body; no lockfile/CI/dependency churn; locale deltas exactly uniform (8 keys × 15 packs); Hmbown#553 resolved as claimed. CI 6/6 green at review time.
Blocking
B15-1 — repo law judges targets against the raw root spelling only; a canonical or absolute-.. target into an attached root misses that root's law entirely, and the write lands modal-free
push_normalized (crates/tui/src/repo_law.rs:216) strips targets against the raw root spelling only; repo_law.rs contains zero canonicalize calls. The execution-resolved re-judge (:243-257) fires only when the collapse leaves a leading .. and the spelling is relative (!path.is_absolute()), so an absolute canonical spelling never re-judges. Execution resolves writes canonically: an existing candidate under a boundary root canonicalizes with no containment re-check in the follow_symlinks branch (tools/spec.rs:1116-1131). The carve-out is safe because carve_out_target_allowed dual-strips against lexical and canonical spellings and re-checks the resolved path (core/authority.rs:583-601); repo law has no equivalent.
Traced vectors (all verified line-by-line): (A) attached root /w/linked → symlink to git repo /real/b whose constitution blocks vendor/**; write_file path="/real/b/vendor/lib.rs" — the constitution loads through the symlink, strip_prefix("/w/linked") fails so the tail becomes real/b/vendor/lib.rs, the anchored glob never matches, the re-judge is skipped (absolute) → no hold; carve-out qualifies via canonical strip; execution lands inside /real/b/vendor/ — block-class law bypassed, modal-free under default Ask, and in trust/yolo/headless with no receipt. (B) interior symlink in the primary (linked2/vendor/x): judged tail never matches, execution canonicalizes into the target repo. (C) absolute .. spelling /w/x/../../real/b/vendor/lib.rs: strip succeeds against the primary, the collapse leaves a leading .., but the !path.is_absolute() guard skips the re-judge while execution's normalize_path pops the .. and admits it.
Base-inherited for N=1 (base repo_law.rs:176-183 has the same raw-only strip), but this PR makes N constitutions bypassable per session, and the fail-open direction is undisclosed — body line 42 discloses only the fail-CLOSED symlink gaps. Repo law is now the single lane this PR left raw-spelling while canonicalizing everywhere else (shell, spec, authority, sandbox) — inconsistent with core's own doc ("Callers that enumerate writable roots canonicalize per root"). Fix shape: compute each root's canonical form once in repo_law_plan_decision and dual-strip in push_normalized; drop the is_absolute guard so absolute .. collapses re-judge; re-judge the resolved candidate against the canonical form exactly as authority.rs does. Pin with a symlinked-root fixture — the existing dotdot test (relative spelling, canonical tempdirs) cannot catch any of the three vectors.
B15-2 — the PUT /v1/sessions update path persists workspace_roots beside a stale workspace; resume-thread resurrects the abandoned workspace as a writable primary root
runtime_api/sessions.rs:784-797 stamps updated.metadata.workspace_roots = snapshot.workspace_roots.clone() (:796) while update_session (session_manager.rs:2261-2311) never touches metadata.workspace. Trace: session saved at W1 → PATCH moves the thread to W2 (engine evicted; new snapshot carries workspace=W2) → PUT re-save persists {workspace: W1, workspace_roots: [W2, …]} → POST /v1/sessions/{id}/resume-thread feeds both into create_thread, whose normalizer puts the stale W1 in the primary slot (runtime_threads.rs:5447-5448 + core/src/lib.rs:77-85) → W1 returns as a writable root the caller never listed. Fail-open. This is the exact failure mode the /fork path's own comment documents and avoids (commands/groups/session/session.rs:158-171), and the exact hazard save_thread's outside-roots warning guards (runtime_threads.rs:1431-1451) — save_session has no guard, so the mispaired record lands silently and is served verbatim by GET. Stale metadata.workspace alone predates the PR but was benign/consistent then; this PR converts it into a contradictory trust-bearing pair. Fix shape: stamp workspace from the same snapshot beside the roots (or re-normalize the set against the saved workspace like /fork does), plus a pairing test — the current resave test asserts only the roots side.
B15-3 — the normal exec lane's cwd: operand resolves into attached roots while the approval context never sees it
shell.rs:5084 resolves the exec cwd: operand through the roots-aware ToolContext::resolve_path and executes there — base refused the same call with PathEscape. The approval side never sees the operand: exec_shell_ask_rule_decision_for_policy (core/engine.rs:7999-8014) feeds only command into tool_ask_rule_decision_for_context, passing None for the path (engine.rs:8009). A scoped allow rule for git push therefore auto-approves git push with cwd:<attached repo> — a different repository — that base refused outright. This contradicts the invariant execpolicy itself encodes ("exec always runs in the session cwd", execpolicy/src/lib.rs:469-471) and the body's carve-out-reach claim ("nothing auto-approves execution under an attached root"). The round-14 B14-1 fix covered only the read-only Scout posture (direct_argv); the normal lane is untouched. Fix shape: include the resolved effective cwd in the exec approval context (rule matching and/or the approval key), or refuse non-primary cwd: resolutions under allow-matched commands — plus a pin that would catch a re-widening. Update the body's carve-out-reach paragraph either way.
B15-4 — the 07:19Z "correction" comment misdescribes the re-push it announces
The comment states the 0c366b9ed → 83bf482e3 re-push carried "No source change beyond that test body; the diff vs 0c366b9ed is the test only." The compare API shows 30 files, +845/−82, including the raw-spelling carve-out fix (authority.rs +46/−3) and the restructured v5 migration (state/src/lib.rs +191/−27); the two commit messages are byte-identical. Mitigation, stated plainly: the resulting tree was fully re-reviewed in rounds 13-15 and CI ran on 83bf482e3, so no unaudited code reaches merge — the defect is in the review record, not the tree. A corrective comment (not an amendment) closes this; future force-push descriptions must state the actual delta.
Should-fix
- Cross-process stale-roots resurrection. The cached-resume override writeback (core/src/lib.rs:695-698) routes through
persist_thread→upsert_thread_preserving_policy_and_archive, whose preserving arms cover only policy + the archive stamp (state/src/lib.rs:742-791);workspace_roots/cwdareexcluded.*passthrough, so a stale in-process cache carrying an override can silently revert a concurrent cross-process roots/cwd update. The cwd half is base-inherited; the roots column is newly consequential. Targeted UPDATE (cwd+roots+updated_at) or re-read inside the gate — or at minimum extend the body's flap disclosure to name roots/cwd, not just the archive flag. - RUNTIME_API.md, three spots (one introduced by the round-14 fix): (a) :1029 "single-root threads omit the key entirely" is inverted for the dominant POST path — normalize always yields ≥1 element, so POST-created single-root threads serialize
"workspace_roots": ["<workspace>"]; only legacy rows omit the key, and the same doc's :601 still says[workspace]— pick one; (b) :588-600 "the first entry is the primary root" — whenworkspaceis omitted the server injects its cwd, which wins the primary slot and demotes roots[0] to an additional root; (c) the sessions section omits POST/PUT /v1/sessions entirely and the 409 paragraph names only PATCH while PUT now also 409s. - Placeholder parity is not enforced for the five new
Workspace*keys: the only generic gate filtersStatus-prefixed Debug names (localization.rs:5216-5218). Packs are clean today (verified mechanically); add the prefix or extend the fixed list so a dropped{error}cannot ship. - Receipt severity is locale-dependent:
classify_status_textsniffs English keywords (status.rs:150-161), so the Japanese double-failure receipt becomes an ephemeral Info toast instead of a sticky Error. Route the composed receipt through the typed toast with an explicit level. - Resume faces seed the set unnormalized (event_loop.rs:594, handlers.rs:1172, apply.rs:1219 assign saved metadata verbatim). All shipped writers normalize today, so reachability is thin; normalize-at-seed is one line and retires the residue.
- Body line 40's consumer list is accurate but incomplete — add at least: default-rooted search/read tools (
grep_files/list_dirdefaultpath=".",file_searchdefaults to the primary, relative paths join the primary — the first gap a user hits in an attached repo), memory/knowledge keyed to the primary (native_memory.rs:102,164; remember.rs:134), skill discovery (tools/skill.rs), the whole LSP subsystem (the pool is rooted at the primary — line 40 names only read_lints), project-MCP cwd confinement (mcp.rs:4849-4864), hooks execution cwd, the git-coupled family (git_status/diff/log/show/blame, review, verify, run_verifiers, run_tests, task gate records), and the cwd-derived PTY/sandbox materialization whose writable set diverges from the roots-aware turn policy. All fail-closed; they belong in the disclosure, not the code.
Refuted this round (for the record)
A candidate build-vs-evict race (engine inserted after PATCH eviction without recheck) is closed by the full-record record_is_current recheck at runtime_threads.rs:8317-8325 before insert — every interleave resolves. Recorded so it is not re-raised.
Four-criteria verdict
- Real value / root cause: PASS. The engine-side rebuild from the persisted ThreadRecord alone remains the root-cause fix; the end-to-end chain holds (POST → write under attached root → modal-free carve-out → evict → rebuild → resume → fork → PATCH); empty ≡
[cwd]verified at wire/state/sandbox. - Elegance: PASS with recorded debt. Tri-state ×4, 26 stamp sites (14 compile-enforced), execpolicy copy layering-constrained; base facilities genuinely extended.
- Smuggling: PASS. 78/78 files classify; PUT 409 disclosed as breaking; locale deltas uniform; no dep/CI churn.
- Defects: FAIL this round — B15-1/B15-2/B15-3 must close, plus B15-4's record correction and the should-fix batch.
Convergence
The three code blockers are small and localized: dual-strip + guard-drop + symlinked-root pin (repo law); one-stamp pairing (sessions PUT); approval-context cwd (exec). Add the B15-4 corrective comment, refresh the three doc spots, extend the disclosure list — and this converges. The round-14 closure quality (every fix carrying a non-vacuous pin) is exactly the right bar; the three new blockers are the same class this review history keeps teaching: one lane frozen at the old boundary while the rest of the matrix moves.
| // Make root-relative when the tool gave an absolute path inside it. | ||
| let path = Path::new(&trimmed); | ||
| let relative = path.strip_prefix(workspace).unwrap_or(path); | ||
| let relative = path.strip_prefix(root).unwrap_or(path); |
There was a problem hiding this comment.
Blocking (B15-1). This strips against the raw root spelling only, and repo_law.rs has no canonicalize anywhere; the re-judge at :243 additionally skips absolute spellings (!path.is_absolute()). Consequence: an attached root that is itself a symlink (/w/linked → git repo /real/b, constitution blocking vendor/**) lets write_file path="/real/b/vendor/lib.rs" through with no hold — the tail becomes real/b/vendor/lib.rs, the anchored glob never matches — while the carve-out qualifies via its canonical dual-strip (authority.rs:583-601) and execution canonicalizes into /real/b/vendor/ (spec.rs:1116-1131, no containment re-check in the follow_symlinks branch). Modal-free bypass of block-class law under the default Ask posture. Same shape works with an interior symlink in the primary and with absolute .. spellings (the :243 guard skips them while execution's normalize_path pops them). Base-inherited for N=1, but this PR makes N constitutions bypassable per session and the body discloses only the fail-closed symlink gaps. Fix: dual-strip against the root's canonical form (mirror carve_out_target_allowed), drop the is_absolute guard, re-judge the resolved candidate; pin with a symlinked-root fixture — the existing dotdot test (relative spelling, canonical tempdirs) cannot catch any of the three vectors.
| snapshot.model_provider_id.as_deref(), | ||
| ); | ||
| updated.metadata.mode = Some(snapshot.mode.clone()); | ||
| updated.metadata.workspace_roots = snapshot.workspace_roots.clone(); |
There was a problem hiding this comment.
Blocking (B15-2). This stamps workspace_roots from the snapshot while update_session (session_manager.rs:2261-2311) never touches metadata.workspace. Sequence: session saved at W1 → PATCH moves the thread to W2 → PUT re-save persists {workspace: W1, workspace_roots: [W2, …]} → POST /v1/sessions/{id}/resume-thread feeds both to create_thread, whose normalizer puts the stale W1 in the primary slot — the abandoned directory comes back as a writable root the caller never listed. This is exactly the failure the /fork path's own comment documents and avoids (session/session.rs:158-171), and save_thread warns about it (runtime_threads.rs:1431-1451) while save_session stays silent. Stamp workspace from the same snapshot (or re-normalize the set against it like /fork does) and add a pairing test — the current resave test asserts only the roots side.
| @@ -7949,6 +8008,7 @@ pub(crate) fn exec_shell_ask_rule_decision_for_policy( | |||
| command, | |||
| None, | |||
There was a problem hiding this comment.
Blocking (B15-3). The exec approval context passes None for the path, so typed rules and the approval key never see the resolved cwd: operand — while the normal exec lane now resolves cwd: through the roots-aware resolve_path (shell.rs:5084; base refused it with PathEscape). A scoped allow rule for git push therefore auto-approves git push with cwd:<attached repo> — a different repository — that base refused outright, contradicting execpolicy's own invariant ("exec always runs in the session cwd", execpolicy/src/lib.rs:469-471) and the body's "nothing auto-approves execution under an attached root". The round-14 B14-1 fix covered only the read-only Scout posture. Include the resolved effective cwd in the approval context (rule match and/or key), or refuse non-primary cwd: under allow-matched commands, plus a pin.
| - **ThreadRecord** — `id`, `created_at`, `updated_at`, `model`, | ||
| `model_provider` (generic kind), `model_provider_id` (optional exact configured | ||
| route), `workspace`, `mode`, `task_id`, `system_prompt`, `latest_turn_id`, | ||
| route), `workspace`, `workspace_roots` (the full accessible root set, |
There was a problem hiding this comment.
Should-fix. Inverted for the dominant path: workspace_roots skips serialization only when the vec is empty, and POST/PATCH normalize to ≥1 element — so a POST-created single-root thread serializes "workspace_roots": ["<workspace>"] (key present). Only legacy pre-multi-root rows omit the key. Also reconcile with :601 ("exactly the historical single-root thread ([workspace])"), which this line now contradicts.
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-16 review — fresh full re-review of head 9f5f4d34a (unchanged since round-15)
Method. Another from-scratch pass, not a delta check: 19 parallel review shards over the full range — protocol wire, state/SQLite, core resume/fork, engine/turn, authority/execpolicy, repo_law (deep dive), shell/sandbox, spec/subagent, runtime API, runtime threads, session manager, TUI UI layer, i18n, ACP/app-server/CLI, adversarial security, concurrency, full-tree consumer census, base parity (per-hunk), value/root-cause, elegance — plus my own line-level verification of every blocking/major finding below. No rebase needed: merge-base is the current pinvou3-clean tip (6f780290f), 0 behind, MERGEABLE. CI 5/5 green on this head.
Verdict: Request Changes. The feature's value and root-cause placement remain solid, and this round re-confirmed the enforcement core (execpolicy matching, resolve_path, sandbox materialization, single-root equivalence) — but three merge-blocking defects are open, all with small, surgical fixes.
Must fix
1. Blocking — repo_law's push_normalized judgment is spelling-based; two fail-open shapes escape the per-root constitutions
crates/tui/src/repo_law.rs:243 gates the execution-resolved re-judgment behind && !path.is_absolute(), so only the relative-.. shape (fixed in round-14) is re-derived from where execution actually lands. Two sibling shapes still miss:
- Absolute path containing
..that lands inside an attached root.write_file {"path": "/P/../A/secret/x"}with attached root/Aholding{"paths": ["secret/**"], "action": "block"}: against/A,strip_prefixfails and the lexical collapse keeps the foreign prefix (…/a/secret/x), so no anchored glob fires; against/Pthe tail keeps a leading..marker and is skipped by the same absolute gate.resolve_pathnormalizes to/A/secret/x, finds it contained, and permits the write. Both constitutions miss — a block-level bypass by spelling. - Relative target landing under an ancestor root. Session cwd
/repo/sub, attached root/repowith law globsub/**: inputx/yis judged by its raw tail (x/y) against/repo, sosub/**never matches, while execution writes/repo/sub/x/y. The module's own contract ("the worst case is an extra prompt or block, never a missed hold", repo_law.rs:50-55) does not hold for nested root sets.
The fix formula already exists at line 249 — normalize_lexical_components(&workspace.join(raw)) then per-root strip_prefix. Drop the is_absolute gate and judge the execution-resolved shape for every spelling (keeping the raw tail for the across-roots fail-closed judgment). Existing tests pin only disjoint roots and the relative-.. shape, so neither vector is caught today.
2. Blocking — an empty-string root passes intake and vacuously satisfies every containment check: the file-tool boundary becomes allow-all, silently
normalize_workspace_roots (core/src/lib.rs:77-85) dedups but never validates, and nothing in the intake chain rejects junk entries (workspace is checked empty at runtime_threads.rs:5749-5753; roots entries are not). PATCH /v1/threads {"workspace_roots": [""]} (or POST, or the app-server bridge) survives to ToolContext. In boundary_roots (spec.rs:1078-1086), canonicalize("") fails and the raw "" is kept in both slots of the pair; Rust's Path::starts_with("") is vacuously true (an empty component list prefixes everything), so the containment gate at spec.rs:1136-1139 and the exists-lane recheck at :1159-1163 both pass for any absolute path. Net effect: reads anywhere the user can read, writes anywhere in non-Ask postures, with no warning — the opposite direction of the body's "junk roots fail closed" disclosure.
Every other layer fails closed on "" (execpolicy matching, carve-out canonicalize, sandbox materialization), which makes this the one silent fail-open in the chain. Fix: reject empty/non-absolute roots at intake (mirroring the existing workspace empty check), or drop roots whose canonicalize fails in boundary_roots (precedent: canonical_readonly_roots, shell.rs:4038-4045). Either way, also correct the body's disclosure sentence.
3. Blocking — PUT /v1/sessions update path writes workspace_roots without deriving workspace, persisting a stale pair
sessions.rs:796 stamps updated.metadata.workspace_roots = snapshot.workspace_roots.clone() on the update branch, but update_session never touches metadata.workspace. Scenario: POST /v1/threads (W1) → save session → PATCH /v1/threads/{id} {"workspace": W2} → PUT /v1/sessions. Persisted pair: (workspace=W1, roots=[W2]). On resume-thread or exec --resume, normalization resurrects the abandoned W1 as the primary writable root. This is the only one-sided writer in the tree (POST and both other PUT branches write both halves), it contradicts the pair invariant the PR itself documents, and it is still open from round-15. Fix is one line (updated.metadata.workspace = snapshot.workspace.clone();) plus a test — the current test never changes the thread's workspace and never asserts metadata.workspace after resave, so this hole passes green.
Should fix (majors)
4. P2 — "nothing auto-approves execution under an attached root" is false as written (adjudicated down from round-15's P1, with reasoning)
Confirmed mechanics: typed allow rules never see the call's cwd (path: None, engine.rs:8005-8013), and the approval-cache grouping key is shell:<prefix> with no cwd (approval_cache.rs:83-115) — so an allow rule or remembered grant approved at the primary auto-approves the same command with cwd inside an attached root (cwd admission is roots-aware, shell.rs:5084). Downgrade rationale: base's contract already ignored per-call cwd (any in-primary cwd was auto-approved), and the sandbox layer makes attached roots writable by design anyway, so this is contract drift, not a new privilege. But two load-bearing statements are now false: the execpolicy comment "exec always runs in the session cwd" and the body's stronger sentence. Either feed cwd into exec rule evaluation (and the cache key) or fix the comment and the body.
5. P2 — the body's consumer-hunt list (line 40) is materially incomplete; four omissions have user-visible consequences
Full-tree census found ~10 primary-only consumers not in the recorded list: skills discovery (SkillRootCatalog::build(workspace, …)), slash commands (.codewhale/commands), project config (load_project_config_outcome reads only the primary's .codewhale/config.toml), workspace-scope memory keying (NativeMemoryStore::workspace_id(&context.workspace) = SHA of the primary's git origin — memories about an attached root land under the wrong key), review/verify diff evidence (resolve_diff_target(context.workspace, …) — an attached-root change under review presents the primary's diff as evidence), PTY start dir, hooks cwd, test_runner, js_execution, verifier, diagnostics. The first four fail in user-visible, safety-adjacent ways (memory mis-keying, silently ignoring an attached root's approval-tightening config, partial review evidence, skills invisible to the model), and none is disclosed. Minimum bar: extend the disclosure list; memory keying and project config deserve a fix or an explicit follow-up issue.
6. P2 — RUNTIME_API contract statements: one false, one missing
- "single-root threads omit the key entirely" (RUNTIME_API.md:1029-1030, and the parity-test comment at parity_protocol.rs:269-279) is inverted: both spawn paths normalize to at least
[cwd], so every record written by this build carries the key; the field's own doc comment at protocol lib.rs:94-98 states the correct semantics. - The new 409 on
PUT /v1/sessionsfor live sessions (sessions.rs:749-757) is implemented and tested but documented nowhere; the docs name only PATCH.
7. P2 — /undo, snapshots, and revert cover only the primary root while the carve-out auto-approves writes under attached roots
/undo opens the snapshot repo for app.workspace only (undo.rs:144-146); pre-turn snapshots are taken of the primary only (engine.rs:2162-2177); revert_turn likewise. Combined with the disclosed carve-out widening (modal-free writes under attached git roots), a turn's effects under an attached root survive /undo while the transcript reports the restore. Disclose, or extend snapshot coverage in a follow-up linked from the body.
8. P2 — body disclosure: the "single-root-visible behavior changes (recorded)" list is missing ≥5 real items
All intentional, in-code documented, and test-pinned — but absent from the recorded list: cached-resume cwd override now persists the row and bumps updated_at (core lib.rs:695-698; base did neither); missing-thread resume/fork changed from HTTP 200 + status:"missing" to 404, and from stdio success to -32004 (app-server lib.rs:691, 2098); /cd now persists synchronously and composes persistence receipts into the closing status; claim-less worktree-child resume flips isolated_worktree false→true via the launch-manifest fallback; the whitespace-spelling carve-out/string_field alignment (this also narrows whitespace-padded relative deny-rule matching — mitigated because the actual landing path is the padded junk name in both versions, but it belongs on the list).
Holds (verified this round)
- Value/root-cause PASS: engine-side persistence at protocol/SQLite-v5/runtime-record/session-file layers is the right layer; three end-to-end chains (API create→turns→restart→resume, TUI save→resume, fork inherit) verified link by link with no broken link. The two remaining carriers (task spawner
NewTaskRequest, export/import) are disclosed, fail-closed, and fine as follow-ups. - Enforcement core: execpolicy per-root matching (Allow pinned to
vec![0], ask/deny spanning with order-independent strongest-verdict merge),resolve_pathsymlink/../nonexistent lanes, sandbox materialization of every root, and the read-only Scout boundary all verified sound; symlink/TOCTOU classes are base-parity and closed for model-controlled inputs. - Concurrency: PATCH active-turn fence, cache-fill full-record recheck, turn-claim fence, and the writer table (11 sites) all cover the new field; the only stale-writeback paths (cross-process cache/recovery, mid-turn fork zombie) are base-inherited shapes.
- Hygiene: 45/45 commits Signed-off-by, all subjects on-theme, no smuggled content; the two CI/test-infra commits exist for this PR's own tests.
- i18n: all 8 new keys present in all 15 packs, placeholder parity exact (scripted).
Adjudication notes vs round-15
- Round-15 P1-1 (repo-law raw spelling): the relative-
..fix landed for real, but the two sibling shapes above show the!path.is_absolute()gate is the actual root cause — reopened as Must-fix 1. - Round-15 P1-2 (PUT sessions): unchanged — reopened as Must-fix 3.
- Round-15 P1-3 (exec cwd): mechanics confirmed but re-rated P2 (Should-fix 4) — base parity of the ignore-cwd contract plus by-design sandbox writability of attached roots; the false comment/body sentences remain must-correct.
Convergence order unchanged: fix round → squash → Hmbown#484 rewrite/re-pin → re-pin Hmbown#549.
Round-15 addressed (head
|
Round-16 addressed (head
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round-17 review — fresh full re-review of head 8ba62fd7e
Method. Another from-scratch pass, not a delta check: 19 parallel review shards over the full range (repo_law deep dive, intake/boundary containment, sessions pairing, exec/approval lanes, rollback disclosure, resume/writeback/remap, TUI commands + ACP + wire pins, delta test quality, shell read-only boundary, carve-out/sandbox, resolve_path, runtime HTTP API, fork/export/import/tasks, engine turn lane + worktree inheritance, state/core/fork-policy, PR-body claims audit, elegance inventory, smuggling census, parity + localization, plus a free-hunt sibling sweep), with line-level personal verification of every blocking finding below. Base closure re-verified (ahead 47 / behind 0 vs 6f780290f); CI 7/7 green on this head.
First, the credit that is due. All three round-16 blockers, both majors, and the should-fix batch are genuinely fixed at this head — each fix was verified against the code and each claimed red-green pin was audited by mentally reverting the specific fix hunk; every claim held (including the three new repo-law vector tests, whose fixtures and red legs are real). The PR body's remediation narrative, RUNTIME_API.md corrections, and consumer census are now accurate where earlier rounds found drift.
Verdict: Request Changes. Three new blocking defects are open — all small, surgical fixes, but two of them are residues of the exact root-cause classes this push claims to close, and the fix commit's own trust premise is falsified by the tree.
Must fix
1. Blocking — the trusted-cwd premise is false on six intake lanes; an empty workspace nulls containment exactly like the [""] root this push just fixed
normalize_workspace_roots (core/src/lib.rs:85-96) filters only the roots entries; the new doc comment asserts "The cwd argument is trusted (it carries its own intake validation)." That validation does not exist on most lanes:
POST /v1/threads {"workspace": ""}—create_thread(runtime_threads.rs:5425) defaults but never validates, while the siblingupdate_threadbails ("workspace must not be empty", runtime_threads.rs:5750-5755). One surface rejects the value, the other persists it silently.- App-server
thread/resumeandthread/forkcwdparams —resolve_resume_roots(core lib.rs:110-121) andfork_thread(lib.rs:820-826) feedparams.cwdstraight into the trusted slot; a stdio client sendingcwd: ""(an unset shell variable — the exact realism M15-2's own test cites) poisons the thread durably. - ACP
session/newcwd (acp_server.rs:1505-1509) andsession/load(metadata.workspace, :1590-1591) — raw, unvalidated. - TUI session-JSON resume faces — the same fix commit hardened these seeds against "a legacy or hand-edited record" for roots, but
metadata.workspacefrom the same record flows through unvalidated (apply.rs:2457/3446 → handlers.rs:1175 →[""]). - CLI
--workspace ""on the exec/serve/standalone-MCP faces —resolve_workspace(lib.rs:8095-8100) andmcp_server.rs:121use none of the existing guards (checked_workspace_path, mcp.rs:4807).
In every case boundary_roots() (spec.rs:1078-1086) receives ("", …), canonicalize("") fails, and starts_with(normalize_path("")) is vacuously true — the identical containment-nulling primitive the commit message pins as must-not-happen, now reachable through the sibling slot, persisted, and resurrecting on every resume. The state-reader warning covers only the roots column. Fix: validate the cwd/workspace slot at the same chokepoints that now validate roots (or filter it in normalize_workspace_roots and update the normalize(Path::new(""), &[]) == [""] pin, which currently enshrines the poison-passthrough).
2. Blocking — repo-law's landing judgment bails on ..-overshoot while execution clamps: the block class B15-1 claims closed still has a live sibling shape
normalize_lexical_components (repo_law.rs:322-336) returns None when a .. pops above the filesystem root; both landing judges (lexical tail and resolve_deepest_existing) sit under if let Some(candidate) (repo_law.rs:288-310), so the overshoot spelling produces no judged tail. Execution's normalize_path (spec.rs:1374-1378) instead clamps .. at the root. Concrete bypass: primary /w, attached git root /att with {"paths": ["vendor/**"], "action": "block"}, model calls write_file {"path": "/w/x/../../../att/vendor/lib.rs"}:
- Execution: clamps to
/att/vendor/lib.rs, containment matches, write lands in the attached repo. - Repo law: display tail
../att/vendor/lib.rs, candidateNone→ no tail matchesvendor/**→ no hold, in every posture. - Carve-out (
paths_within_workspace_write_carve_out→carve_out_target_allowed, authority.rs:563-605) uses the clamping normalizer, dual-strips successfully, andresolve_deepest_existing(OS canonicalize folds..) lands under/att→ returns true for a git attached root — so under the default Ask posture the write is modal-free and silent, not even prompted.
The module comment ("None when a .. escapes above the filesystem root: … the ordinary gates govern the call") is false — the ordinary gates admit it. The new pin absolute_dotdot_spelling_into_an_attached_root_is_held uses exactly two .. (no overshoot), so the sibling shape is untested, and the body's "an absolute .. spelling can no longer skip an attached root's constitution" overclaims. Fix: mirror execution's clamp in normalize_lexical_components (or fall back to the clamped candidate when the strict collapse returns None), and extend the pin with the overshoot spelling.
3. Blocking — a worktree child's exec lane inherits the parent's writable roots; the isolation claim added at both clear sites is false on the sandbox face
child_runtime clones the parent turn context wholesale (subagent/mod.rs:2853), including elevated_sandbox_policy built over the parent's full set (engine.rs:6811-6819). The worktree clear sites reset only context.workspace_roots (subagent/mod.rs:9371-9377 spawn, :5941-5948 resume — the latter comments "nor the gate's sandbox policy may resolve or write outside the worktree") and never rebuild the policy. The exec lane consumes the cloned policy verbatim (shell.rs:5071 → 2157 → WorkspaceWrite::get_writable_roots), so a worktree: true child running echo x > /shared/x writes into the parent session's attached root under any auto-exec posture — the explicitly requested isolation silently fails, and the file-tool lane (correctly confined) hides the discrepancy. The single-root shape of this mechanism pre-exists at base, but this PR (a) widens the leaked writable set to the new attached roots and (b) added the comments asserting non-carry-over, and the body's "worktree children clear the set" disclosure describes only context.workspace_roots. Fix: rebuild the child's sandbox policy from the cleared context (the workspace_write_policy idiom), or correct the comments and disclose the exec face honestly.
Should fix (majors)
4. P2 — the /cd degraded-success receipt is promoted to a sticky Error toast
apply_workspace_switch_closing_status (session_state.rs:811) classifies with let failed = receipt.is_some();, but the receipt producer has a success arm: (None, false) => "persisted, but the persistence actor is unavailable; the next autosave re-persists it" (session_state.rs:699-711) — the direct save landed, only the actor enqueue failed. That arm now renders as a sticky Error toast implying failure. The new pin covers only the definite-failure receipt and None. Classify on the message kind, not on is_some().
5. P2 — the /cd blocked message is hardcoded English and names the wrong blocker
workspace_switch_blocked_message (session_state.rs:789-792) returns the literal "Cannot switch workspace while a request is running." for every session_transition_blocked() leg — idle compaction, purging, queued tasks — where no request is running, and bypasses the locale packs that the sibling /clear message and the round-14-localized /cd failure strings all use. New typed MessageId + per-leg wording.
6. P2 — the body's fork-policy sentence asserts a registration that does not exist
The body states the retention reason and reduction order "are recorded in the parent register rewrite," but the parent repo has no workspace-roots entry in docs/fork-modifications.md and scripts/fork-guard.sh still pins EXPECTED_HEAD=6f780290f / EXPECTED_COMMITS=39 — merging #54 and re-pinning the gitlink reds guard layer-0 until a parent-side batch lands. The in-tree half is satisfied (10 forkguard_workspace_roots_* behavior tests, RUNTIME_API/SANDBOX docs). Rephrase the sentence to the actual sequence (registration lands with the parent re-pin PR), or land the parent-side registration first.
P3 inventory (recorded, non-blocking)
Whitespace-only path spellings skip all repo-law tails (pre-existing, repo_law.rs:245-248); runtime patch-undo and the revert_turn tool description have no boundary pins despite "pins on each face"; restore_covers_primary_only over-discloses for a symlink-alias spelling of the primary; exec save path round-trips unnormalized persisted sets (disclosed remainder); the normalize("") pin enshrines trusted-cwd passthrough (subsumed by fix 1); the disclosed exact-allow narrowing lacks a pin; the writeback clobber pin's red leg is archive-only; the state-reader warn is unpinned; the parity synthetic test exercises the helper not the gate loop; fixed non-unique temp paths in update_thread_explicit_empty_roots…; a stale test comment ("every other engine-level caller passes &[]" — none do); ACP load lane consumes persisted roots raw (safe at every consumer, inconsistent with sibling faces); exec --resume from a different cwd persists {workspace: invocation cwd, roots led by recorded workspace}; normalize_lexical_components divergence untested beyond two .. (subsumed by fix 2); follow_symlinks early-return judges only the primary for non-existent targets (fail-closed); exists-branch compares the un-normalized root (fail-closed, pre-existing); PATCH active-turn rejection maps 400 not 409 and the guard is untested; cross-process store sharing bypasses the active-turn guard (pre-existing class, extended to roots); /fork <id> primary-swap filter is lexical (alias spelling survives as attached); stream_turn has no roots flag (census wording); core invoke_tool feeds wire roots to execpolicy un-normalized (fail-closed via Allow narrowing); non-UTF-8 root degrades the whole persisted set to [] (documented); five duplication debts (the resolve_deepest_existing byte-twin, the five-site primary-swap rule — update_thread could simply call the existing resolve_resume_roots — the canonical-pair idiom, the shared resolve-and-contain leg, the seven-fold tri-state prose); zh-Hant 保存 drift extended into two new WorkspaceSwitch* keys; five real N=1 changes disclosed only outside the dedicated single-root list; js_execution/code_execution and project-config discovery missing from the consumer census; PATCH workspace: "relative" accepted (fail-closed); the exec judged-cwd stays lexical vs execution's canonical (fail-closed under exact-equality scoping except a rule authored naming the very symlink spelling).
Axis verdicts
- Value / root cause: PASS. Real problem, correctly placed in the engine/persistence layer, end-to-end chains verified; every earlier blocker is genuinely closed. The two residues above (cwd slot, overshoot) are incomplete root-cause closure of the same two families, not new design flaws — both fix surfaces are small.
- Elegance: PASS with debt. One chokepoint (
normalize_workspace_roots, 30 call sites), consistent idioms, minimal API surface, coherent test architecture; five copy-pasted predicates are standing drift invitations (inventory above), none divergent today. - Smuggling: PASS. All 92 files classify as core topic or directly-forced adjacent; zero class-C. The clippy commit is the only drive-by — disclosed, behavior-neutral (verified per hunk), CI-gate-forced; ideally a separate one-commit PR, but on a CI-gated long-running series keeping it disclosed on-branch is defensible. DCO verified on sampled commits; no secrets, no debug leftovers; CHANGELOG absence is compliance, not a gap.
- Defects: FAIL — three blockers above.
Convergence (unchanged): fix the three blockers (+#4-#6) → squash → parent re-pin/register batch → Hmbown#549 re-pin. The body should also pick up the fix-1 trust-premise sentence, the fix-2 "no longer skip" sentence, the worktree disclosure, and the fork-policy sentence.
Round-17 addressed (head
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 18 — full re-review from scratch at head d872c6663, no carry-over from earlier rounds. Ten independent audit lanes over the merge-base diff (6f780290f..d872c6663, 95 files, +8169/−460); every P0/P1 below I re-read and confirmed personally against the tree.
Verdict: Request changes. Two P0 boundary bypasses, one of which this PR introduces and one of which it introduces and then advertises as a fix. Beyond the defects, two structural objections that earlier rounds passed and I do not: the smuggling axis, and whether the feature is usable at all.
Three of my conclusions overturn a prior round's grade. I say so explicitly where that happens, with the trace, so the disagreement can be adjudicated rather than re-litigated.
Blockers
B18-1 (P0) — repo law can be bypassed by a symlink followed by ..; the law judges the lexical path, execution judges the kernel path
crates/tui/src/repo_law.rs:293 shadows candidate with its lexically-collapsed form, and :307 feeds that to resolve_deepest_existing:
let candidate = normalize_lexical_components(&candidate); // :293 — rebinds, the raw spelling is gone
...
if let Some(resolved) = resolve_deepest_existing(&candidate) // :307 — only ever sees the collapsed path
&& let Ok(tail) = resolved.strip_prefix(root_canonical)The function the comment at :333 says this mirrors does the opposite — crates/tui/src/core/authority.rs:580-595 keeps candidate raw and canonicalizes that, holding the lexical form in a separate binding:
let lexical = normalize_path(&candidate); // separate binding
...
let Some(resolved) = resolve_deepest_existing(&candidate) else { return false; }; // the RAW candidateLexical .. pops the previous text component. realpath(3) pops the parent of the resolved directory. They diverge exactly when a symlink precedes a .., and the execution lane is the realpath one: ToolContext::resolve_path (crates/tui/src/tools/spec.rs, exists-branch) canonicalizes the raw joined candidate and returns it.
Trace. Primary /home/u/app; attached root /home/u/vendorrepo whose .codewhale/constitution.json blocks vendor/**; /home/u/app/deps is a symlink to /home/u/vendorrepo/vendor (an ordinary vendored-deps layout — and one the agent can create with a single ln -s, or that arrives with a git checkout of a repo carrying a committed symlink). Write target: /home/u/app/deps/../vendor/crypto.rs.
Repo law, root = /home/u/vendorrepo:
| branch | value | strips against the attached root? |
|---|---|---|
raw tail (:253-277) |
home/u/app/vendor/crypto.rs |
no |
lexical candidate (:294-302) |
/home/u/app/vendor/crypto.rs |
no |
resolve_deepest_existing (:307) |
canonicalize of /home/u/app/vendor/crypto.rs |
no |
targets for that root never contains vendor/crypto.rs. No hold, in any posture. Execution canonicalizes the raw spelling: deps → /home/u/vendorrepo/vendor, .. → /home/u/vendorrepo, vendor/crypto.rs → /home/u/vendorrepo/vendor/crypto.rs. That is inside a declared root, so contained and under_root both pass and the write lands on the law-protected file.
Note that carve_out_target_allowed refuses this same spelling — its resolve_deepest_existing on the raw candidate strips against the primary and fails. Repo law is the outlier, and repo law is the gate documented at repo_law.rs:14-15 as the one no posture bypasses.
Why the suite is green: interior_symlink_target_lands_under_the_attached_roots_law (:826) uses "linked2/vendor/lib.rs" — no .., so the collapse is a no-op and resolve_deepest_existing sees the true spelling. Adding one .. after the symlink defeats it. No test in the PR combines a symlink with a ...
This is new in this PR: pre-multi-root the kernel-resolved target /home/u/vendorrepo/... would have hit PathEscape at the single-root check.
Fix: run resolve_deepest_existing on the pre-normalization candidate in addition to the current one (adding, not replacing — replacing breaks absolute_dotdot_overshoot_into_an_attached_root_is_held, whose non-existent prefix makes the raw walk return a ..-bearing path). Better: make authority::resolve_deepest_existing pub(crate) and call it with the same argument, deleting the byte-twin. Round 17's review already logged the twin as a "duplication debt"; what it did not notice is that the two callers pass different arguments, and that difference is this bypass. The debt was not cosmetic.
B18-2 (P0) — the same lexical/canonical split defeats B15-3, the fix this PR ships for it
Check side, crates/tui/src/core/engine.rs:8012-8023:
let effective_cwd = ["cwd", "working_dir"].iter()
.find_map(|name| tool_input.get(name).and_then(Value::as_str))
.map(|dir| { ... crate::tools::spec::normalize_path(&joined) }); // LEXICALUse side, crates/tui/src/tools/shell.rs:5083:
Some(dir) => { let resolved = context.resolve_path(dir)?; // CANONICALIZES
Some(resolved.to_string_lossy().to_string()) }Session: primary /ws, attached root /attached. The user holds the ordinary "always allow" grant, which build_persistent_allow_rules always scopes to the primary (into_exact_workspace_allow("/ws")). The agent creates /ws/sub → /attached/repo/x, then calls exec_shell {"command":"git push","cwd":"sub/.."}.
- Check:
normalize_path("/ws/sub/..")=/ws.workspace_scope_matchesis exact equality (execpolicy/src/lib.rs:1129), so it matches the primary,candidate_idx = vec![0], Allow fires, no modal. - Use:
resolve_path("sub/..")canonicalizes →/attached/repo.under_rootpasses.git pushruns in a different repository.
This is verbatim the scenario exec_shell_scoped_allow_rule_does_not_follow_cwd_into_attached_root claims to close; the test only uses the literal spellings "." and "/shared".
This overturns round 17's grade. That round logged "the exec judged-cwd stays lexical vs execution's canonical" as fail-closed under exact-equality scoping except a rule authored naming the very symlink spelling. The trace above needs no such rule: exact equality is what makes it fire, because the lexical collapse lands the operand exactly on the primary the grant is scoped to. The direction is fail-open, not fail-closed.
It is also a TOCTOU — the check runs in the turn loop, resolve_path runs later inside BashTool::execute, so sub can be re-pointed in between.
Same primitive, adjacent surface: crates/tui/src/tools/approval_cache.rs:103 keys remembered grants on the raw unresolved operand (shell:{prefix}@cwd:{cwd}, and shell_cwd_operand at :141 does no normalization at all). A grant approved for cwd:"work" stays keyed to work while the agent re-points the symlink — cache hit, no modal, different repository. The key is additionally non-injective: prefix is model-supplied lowercased tokens, @cwd: is not escaped, so {"command":"foo@cwd:/attached/repo"} and {"command":"foo","cwd":"/attached/repo"} build the identical key in both directions. Key on the resolved effective cwd; hash or length-prefix the components.
B18-3 (P0) — the PUT /v1/sessions 409 guard is bypassed by one leading space, and fails open on a poisoned lock
crates/tui/src/session_manager.rs:344-348:
pub fn is_live_session(session_id: &str) -> bool {
live_sessions().read().is_ok_and(|live| live.contains(session_id))
}Exact HashSet::contains on the raw string. But set_live_session (:317) inserts .map(str::trim), and validated_session_id (:986) resolves the write target with id.trim(). So:
PUT /v1/sessions
{"thread_id":"thr_...","session_id":" sess-x"}→ the guard at runtime_api/sessions.rs:749 sees " sess-x" ∉ registry → 200 → load_session(" sess-x") and the subsequent save_session both land on sessions/sess-x.json. The live session's document — including its workspace_roots — is overwritten from a stale engine snapshot. That is exactly the roots-erasing write B15-2 and the 409 exist to stop. The same bypass applies to the pre-existing PATCH /v1/sessions/{id} guard via %20sess-x; pre-existing there, but newly relied upon here.
Second defect in the same three lines: is_ok_and returns false on a poisoned RwLock, so any panic while the lock is held silently admits every subsequent external write. A guard whose stated justification (sessions.rs:743-748) is "fail closed" must not degrade permissive.
Third, and this reframes the disclosure rather than the code: set_live_session has exactly one non-test caller — crates/tui/src/tui/ui/frame.rs:988, inside the TUI's build_session_snapshot — and run_http_server has exactly one caller, Commands::Serve, a subcommand that never starts the TUI. The registry is a process-local static. So in codewhale serve --http, is_live_session is always false and the 409 can never fire. The body's caveat ("in a split TUI + serve --http topology it does not fire across processes") reads as an edge case; it is the only shipped topology. The disclosed breaking change therefore breaks nothing and protects nothing, while the write it exists to stop still happens.
B18-4 (P1) — the vacuous-root class is half-closed: /.. still nulls containment
crates/core/src/lib.rs:96-98 filters on is_empty() || !is_absolute(). Path::new("/..").is_absolute() is true, so /.. survives intake and is persisted. Downstream, normalize_path("/..") is / (RootDir sets is_root, the ParentDir arm finds an empty stack and is_root true, so nothing is pushed), and boundary_roots() canonicalizes /.. to / as well. Both legs of the containment test at tools/spec.rs:1136-1139 then read candidate.starts_with("/") — true for every absolute path. get_writable_roots likewise declares / writable.
POST /v1/threads {"workspace":"/repo/main","workspace_roots":["/.."]} is accepted and the thread has no boundary. The stored and displayed root reads /.., not /, so the audit trail misrepresents the grant. Same class the doc comment at core/src/lib.rs:73-79 describes for "" — one spelling was closed, not the class. Reuse a lexical normalizer at intake and reject a result that is the filesystem root or still carries ParentDir.
B18-5 (P1) — a relative cwd is accepted on every lane, collapses the set, and the two enforcement faces then disagree (regression vs base)
Every guard this PR adds checks is_empty() only — core/src/lib.rs:119 (resolve_resume_roots), :817 (fork_thread), :1196 (Start), runtime_threads.rs:5432 (create_thread), and the /tool handler checks nothing. But normalize_workspace_roots fails closed on empty or relative (:91-93). So POST /v1/threads {"workspace":"."} returns 201 with an empty set, and:
boundary_roots()is[]→containedisfalse→ everyFiletool call returnsPathEscape;SandboxPolicy::get_writable_rootsunconditionally pushes the cwd (sandbox/policy.rs:325-330) → the shell lane still has a writable root.
Base handled this fine (workspace.canonicalize().unwrap_or(...), and canonicalize(".") succeeds), so this is a regression: relative-workspace threads go from working to reading nothing. It is also reachable with no user input at all — core/src/lib.rs:1181 and :1201 fall back to PathBuf::from(".") when current_dir() fails.
The CLI path gets this right (tui/src/lib.rs:8106-8117 absolutizes via mcp::normalize_path_components). Apply the same treatment at the other five surfaces, or reject non-absolute alongside empty — and then the RUNTIME_API.md:1040 claim ("a thread created through POST /v1/threads serializes at least one element … only rows persisted before the field existed omit the key") becomes true instead of false.
B18-6 (P1) — ~14 new crates/core tests are deterministically red on Windows, and no Windows leg runs on this branch to catch it
On Windows Path::is_absolute() is has_root() && prefix().is_some(), so Path::new("/repo/main").is_absolute() is false and normalize_workspace_roots returns Vec::new(). crates/core/src/lib.rs:3517:
let cwd = Path::new("/repo/main");
assert_eq!(normalize_workspace_roots(cwd, &[]), vec![cwd.to_path_buf()]); // vec![] != vec!["/repo/main"]The test module at :2369 is a bare #[cfg(test)] with no platform gate. Affected: normalize_workspace_roots_puts_cwd_first_and_dedups, normalize_workspace_roots_drops_empty_and_relative_entries, spawn_thread_with_empty_root_persists_only_the_cwd, spawn_thread_with_workspace_roots_persists_normalized_set (this one panics on spawned.thread.workspace_roots[0], :3616), the six resume tests, the three fork tests. The diff adds 160 POSIX-absolute path literals against 5 cfg(unix)/cfg(windows) gates, with the same pattern in core/authority.rs, repo_law.rs, tui/ui/session_state.rs and core/engine/tests.rs.
The codebase already knows is_absolute() is OS-dependent and works around it (execpolicy/src/lib.rs:1198 is_windows_absolute_path, mcp.rs:206-208), so this is a lapse rather than a convention.
Why it was not caught, and why the test record reads stronger than it is. .github/workflows/ci.yml — which owns the Test matrix [ubuntu-latest, macos-latest, windows-latest], at :524/:603 — triggers on pull_request: branches: [master, main]. This PR targets pinvou3-clean, so that workflow has never run on this branch. The workflow that does run is fork-ci.yml: a single ubuntu-latest job (fmt + clippy + nextest + doctests). I confirmed against the API: d872c6663 has exactly 6 check runs — Check Signed-off-by, Gitleaks, check, gate, link, link. The body's "Linux CI remains the green authority" is accurate and honest; what needs adding is that Linux is the only authority there has ever been, and CodeWhale ships a Windows installer. A workflow_dispatch of ci.yml against this head before merge would settle it.
There is a production-side twin of the same issue: on Windows a root-relative path such as \repo is not is_absolute(), so B18-5's collapse is more reachable there.
Should-fix
- No bound on a client-supplied root set.
POST /v1/threadsaccepts an unboundedworkspace_roots;normalize_workspace_rootsdedups withVec::contains(O(n²)); axum's 2 MiB default body admits ~400k entries.boundary_roots()then pays onecanonicalize()per root, perresolve_pathcall. A cap at intake plus a set-based dedup closes both. Note the asymmetry: a display cap exists (MAX_LISTED_ROOTS = 5, three copies) while an intake cap does not. - The worktree-resume fail-open is not "low probability".
CoordinationLedger::register_claim(coord/ledger.rs:497-514) auto-evicts non-live owners' claims atCOORDINATION_RECORD_LIMIT = 128, andprune_worker_recordsat 256 — neither needs an explicit release, which "stale-released" implies. Read-only-role worktree children (Scout|Planner|Reviewer|Verifier|Consultant,mod.rs:9713-9728) are claim-less from birth, so the manifest is a single point of failure rather than the second half of a conjunction. Andreconcile_orphaned_workers_after_restartterminalizes the whole fleet immediately beforeprune_worker_recordsruns (mod.rs:4565-4566). A 300-child fanout reaches both caps with no operator action. Also: the fail-open applies toResumePolicy::InterruptedOnlytoo, not onlyInterruptedOrCompleted, and the two "end-to-end pinned" tests both driveInterruptedOnly— the policy named in the disclosure has no harness. Persist the worktree bit on the un-cappedSubAgentrecord, or clear the set whenagent.workspace != caller.workspaceand neither source is available. /cdblocks the UI task on a fsync plus a full sessions-directory scan.session_state.rs:777→save_session→write_atomic(fsync) +cleanup_old_sessions()→list_sessions()(parses every session file) +reclaim_orphaned_session_dirs()(directory walk), synchronously on the single TUI event-loop task. The same crate does this correctly atturn_loop.rs:4732(spawn_blocking). Also/cdis the only surface that mutates the set and the only one that discloses nothing:/cd /tmp/scratchfrom[/repo/main, /repo/lib]carries/repo/libinto the scratch session, writable and unrevertable, with output"Switched workspace to /tmp/scratch".- Cross-root over-block in repo law.
repo_law.rs:315-318pushes the collapsed raw tail unconditionally for every root, with no containment condition, andstrongest_hold_wins_across_roots(:717) pins it as intended. So attaching a read-only reference repo whose constitution blocks a common shape (src/**,docs/**,CHANGELOG.md) makes that path unwritable in the primary in every posture, with a hold reason naming a constitution the user never opened (:102-105hard-codes.codewhale/constitution.jsonwith no root path). The module doc at:47-49claims per-root namespaces; for relative spellings it is the union of N constitutions. Calling a hardBlock"at worst an extra prompt" (:54-55) understates it. /toolis the one lane with no cwd guard and no chokepoint.app-server/src/lib.rs:736-757:req.cwd.unwrap_or_else(...)letsSome("")through toExecPolicyContext.cwd(wherenormalize_workspace_relative_path'sworkspace.root.as_ref()?then makes every workspace-relative ask/deny rule silently fail to match), and&req.workspace_rootsgoes to execpolicy raw. Safe today only because execpolicy's separatecandidate_idx = vec![0]narrowing keepsAllowprimary-only — accidental, not structural.- The notice remainder arithmetic is wrong in one of its three copies.
session_state.rs:630-641computesremainderfrom the already-truncated list and hardcodes-1for the primary;status.rs:296-303andengine.rs:4178-4193compute it from the untruncated count. Feed it the seven-root shapestatus.rs:1009already constructs and it prints(+1 more)where/statusprints(+2 more)— one accessible root hidden. Latent today (all producers normalize), butapply.rs:1308is the seam where it goes live. One shared helper removes the defect and the duplication together. - Two rollback faces missed, two unpinned.
POST /snapshots/{id}/restore(runtime_api.rs:5863-5880) is byte-for-byte the same primary-boundSnapshotRepo::open_or_init+restore()as/restoreand/undo, takes noworkspace_roots, and emits no boundary note. And "one pin per face" is not met:revert_turn's tool description (revert_turn.rs:33) has no test, and the runtime patch-undo note has none either. The description is also the one face where the claim "single-root output is byte-identical" is false — it returns a'staticliteral, so the clause is unconditional and every single-root session's tool schema changed. - Performance regressions on the hot path.
repo_law_plan_decision(repo_law.rs:86-90) calls the filesystem-heavywrite_target_pathsbeforeload_repo_law_rules, so a root with no constitution pays the full canonicalize storm; base bailed onrules.is_empty()with zero syscalls.resolve_deepest_existingis recomputed per root althoughcandidateis root-independent.globset::GlobSetBuilder::build()runs per root per call, uncached. A 40-fileapply_patchat 3 roots is ~240 realpath chains plus 3 RegexSet compilations per tool call, synchronously insideasync fn plan_tool_callswith nospawn_blocking. Separately,carve_out_target_within_root(authority.rs:551-561) moved the git-marker stat and the root canonicalize inside the per-path loop, which base had hoisted:2 + Pbecomes~2·P·R. .gitas a one-byte carve-out marker, now × N roots.authority.rs:554accepts anythingsymlink_metadatacan stat — a dangling symlink, an empty file. Not silently manufacturable (is_carve_out_excluded_name(".git")modals the write path, and the shell path needs an approval), so not a blocker — but one approvedgit initin any attached root now converts that whole root to modal-free writes, against a user mental model that says "I attached a scratch folder, it isn't a repo".docs/AUTHORIZATION_ORDER.md— the repo's authorization single-source-of-truth — does not mention the carve-out widening at all;rg -rn "carve" docs/finds it only inRUNTIME_API.md:607.- Non-UTF-8 roots: tolerate-then-overwrite is data loss, not tolerance.
workspace_roots_to_json(state/src/lib.rs:2214) discards the entire set (including the primary) if one entry is non-UTF-8, and the read side tolerates any parse failure asVec::new(). That would be fine if the value were only read — buttouch_message(core/src/lib.rs:983-990) runsget_thread+upsert_threadon every user message, andupsert_threadwritesworkspace_roots = excluded.workspace_rootsunconditionally (:790). One message after a value this build cannot parse, the original is gone permanently. Preserve the raw column text when it fails to parse, or skip the column on a tolerated decode. update_thread_root_setno-ops silently on a vanished row.state/src/lib.rs:834-853discards rows-affected. Process B deletes the thread; A's resume updates its cache, writes zero rows, returnsOk(()), and reports the new root set to the client as applied.merge_persisted_lifecycleis the mirror of the bug B15-2 fixed.session_manager.rs:1675-1685takes the persisted roots without the persisted workspace, so an incoming metadata with a moved workspace and an empty roots set persistsworkspace: /Cbesideroots: [/A, /B].POST /v1/sessions/{id}/resume-threadthen normalizes to[/C, /A, /B]and the abandoned/Aresurrects as writable. Narrow (the TUI load path normalizes first), but the PR added a warn-on-unpaired guard for this exact shape on the thread side (runtime_threads.rs:1432-1450) and no equivalent here.record_stdio_thread_hintdegrades to the empty set.app-server/src/lib.rs:1370-1388:map_or_else(Vec::new, ...)on athread: Nonesuccess re-arms the Hmbown#5171 clobber chain thatensure_thread_foundwas added to break — andensure_thread_foundguards by string-comparingresponse.status == "missing", so a rename in core silently disarms it and this is what fires next. Skip or merge rather than wipe.- The bridge PATCH failure is sticky, not one-shot.
app-server/src/lib.rs:1554-1594returns via?without updatingthread_roots, so the next turn recomputesroots_changedand re-PATCHes. For as long as the runtime thread reports an active turn, every bridged turn fails, with no client-side escape hatch, surfaced asruntime_unavailable(−32005 / 503) for what is a semantic conflict. The body says "the next bridged turn". - Doc claims the code does not support.
RUNTIME_API.md:1040-1044(refuted by B18-5);:566-572asserts a 409 the shipped server cannot return and reinforces the wrong model with "A standalonecodewhale web… is never blocked" (refuted by B18-3) while omitting the process-locality caveat that is in the PR body;:604-618documents ordering, dedup and the empty-array clear but never states that empty/relative entries are silently dropped with a 201. - Weak or vacuous pins.
forkguard_workspace_roots_carve_out_branches_on_the_untrimmed_spelling(authority.rs:~1000) uses a plaintempdir()as primary, socarve_out_target_within_rootreturnsfalseat the git-marker check regardless of trimming — it passes either way.workspace_roots_default_for_rows_predating_the_column(state/src/lib.rs:2816) upserts a row whoseworkspace_rootsis already empty and asserts it reads back empty — it never inserts a pre-column row and never exercisesDEFAULT '[]'or the NULL path.spec/tests.rs:198-214assertsA || Bover the only two possible outcomes. The/cdroot-set transform (session_state.rs:755-767) — the one behaviorally significant piece of that command — has no test at all, while every safe piece around it was extracted and given six. - ACP enforces the set and never discloses it. Confirmed:
PromptSessionContext(prompts.rs:22-52) has no roots field (rg workspace_roots crates/tui/src/prompts.rsis empty) andsession_configurationoffers only model and mode. A loaded multi-root ACP session enforces attached roots the model is never told exist. - Stale rationale comments.
acp_server.rs:1504andmcp_server.rs:85-89justify their guards with "the vacuous containment root (starts_with(\"\")accepts every path)" — that describes base. After this PR an empty workspace makesboundary_roots()empty andresolve_pathreject everything (B18-5). The guards are belt-and-braces; the comments describe a world that no longer exists. Likewiseacp_server.rs:2640says "the product path runs on the 8 MiB main thread";CODEWHALE_MAIN_STACK_BYTESis 16 MiB (lib.rs:1673) and it is aBuilder::stack_size, not an OS default.
Axis verdicts
Value / root cause — FAIL on completeness
The authorization half is genuine and careful. I verified the load-bearing soundness argument and it holds: PermissionAction derives Ord as Allow < Ask < Deny and matching_ask_rule takes max_by_key, so widening rule candidacy to N roots can only raise a verdict; candidate_idx = vec![0] keeps Allow primary-only (execpolicy/src/lib.rs:475-487). The per-root write carve-out, the per-root constitutions, the untrimmed-spelling reasoning, and the honest ATTACHED_ROOTS_NOT_REVERTED_NOTE are all correct work. With R=1 the whole thing collapses to the base expression byte-for-byte at every enforcement surface. The worktree exec-lane confinement (rederive_sandbox_policy_roots) closes a pre-existing single-root hole — base handed the child writable_roots: vec![parent_workspace] — and the untested fresh-spawn call site is, I checked it line by line, correct: context.workspace is assigned before the re-derive reads it, prepare_child_workspace returns Some or Err so the if let cannot swallow the worktree case, nothing re-widens downstream, and flags are preserved.
But the capability half is not there, and I verified the body's own disclosure list item by item — every entry checks out, and the list if anything understates the consequence. The structural proof: the PR touches 9 of ~80 files under crates/tui/src/tools/ and adds zero new tools. Concretely, in a session with /api attached:
| the agent does | what happens |
|---|---|
grep_files/list_dir/file_search with the default path |
joins the primary (tools/spec.rs:1090, search.rs:116, file.rs:2686, file_search.rs:107) — searches the wrong repo and reports success |
read_lints on an attached file |
no reachable path: rejects absolute and .., then requires starts_with(primary) (lsp.rs:317,338) |
lsp diagnostics on an attached file |
one pool rooted at the primary (engine.rs:1957) — silently wrong or empty |
reads the attached repo's AGENTS.md/skills/.codewhale/commands |
never loaded; project_context.rs:484-490 pins primary-only as deliberate |
relies on the attached repo's .codewhale/config.toml |
ignored (config/src/lib.rs:3787) — a repo that tightens approval_policy loses it when attached while staying writable. Inconsistent with constitution.json, which is read per root |
subagent with cwd: into an attached root |
rejected; subagent/worktree.rs has zero root awareness |
review / verify |
evidence resolves against the primary (review.rs:767, verify.rs:439) — half a cross-repo change is unreviewable |
the user types /undo |
nothing to restore; snapshots are primary-rooted |
And the decisive one: there is no shipped way to create a multi-root session. No CLI flag — resolve_workspace returns a single PathBuf and there is no --add-dir anywhere. No TUI command — app.workspace_roots is initialized Vec::new() (app/init.rs:822) and thereafter only read from a resumed session; /cd can preserve a set but cannot add to one. ACP session/new hardcodes Vec::new() (acp_server.rs:1521,1541). The only producer is POST /v1/threads, so a terminal user must run codewhale serve, hand-craft an HTTP POST, save the thread to a session, and resume it in the TUI. That is not a complete user-facing loop, which this project requires of a community-edition feature.
Root-cause quality on the defects is likewise partial: B18-4 closes one spelling of the vacuous root rather than the class, and B18-5 hardens is_empty() at six surfaces while the chokepoint itself rejects is_empty() || !is_absolute() — the guards and the invariant they protect do not agree.
Elegance — FAIL
Upstream openai/codex already ships this concept, better typed: Config.workspace_roots: Vec<AbsolutePathBuf> (absoluteness by construction, not by filtering), one dedupe_absolute_paths, a turn_context.workspace_roots() method on the already-threaded context, workspace_roots as a symbolic term in the permission language, and a repeatable --add-dir shared across cli/exec/tui. The body's "upstream ships a parallel experimental mechanism" understates the overlap; this is closer to a larger, weaker-typed reimplementation of a superset, and the fork is already at ~16× its soft size limit.
This PR instead threads a bare Vec<PathBuf> through 23 production function signatures and 22 struct fields, of which 80 added lines are literal workspace_roots: Vec::new(), filler at unrelated construction sites, across 42 normalize_workspace_roots call sites. Five engine.rs free functions now take (workspace: &Path, workspace_roots: &[PathBuf]) side by side — two correlated parameters that must always be normalized together, which is the exact smell a value object removes. The PR itself demonstrates the alternative: ToolContext carries the set as a field and boundary_roots() derives from it, so no tool needed a signature change. EngineConfig and SessionState were equally available.
The "single chokepoint" claim (core/src/lib.rs:81-83) is architecturally impossible, not merely unenforced. crates/core/Cargo.toml:17 declares codewhale-execpolicy as a dependency of codewhale-core, so execpolicy can never call codewhale_core::normalize_workspace_roots without a cycle — which is precisely why execpolicy/src/lib.rs:439-447 hand-rolls its own root assembly with different semantics (no empty/relative filter, no fail-closed empty cwd). /tool bypasses it too. The invariant is convention across 42 sites plus one that structurally cannot comply, and the tell is the new tracing::warn! at runtime_threads.rs:1433 whose comment describes turning a violated invariant "into a searchable signal". The type-safe landing spot exists and is a leaf crate execpolicy could depend on: crates/paths, which already models PathOverrideErrorKind::Relative. A WorkspaceScope newtype there — absolute-by-construction elements, construction-time dedup and primary-first ordering, contains/canonical_pairs/attached/Display, Serialize/Deserialize — would own the invariant, replace all five canonicalization policies and all three dedups, and delete most of the 23 signature changes.
Duplication, counted: seven hand-rolled lexical normalizers with four incompatible .. semantics (four byte-identical normalize_path_components copies that silently drop a leading ..; normalize_lexically; normalize_path_lexically; spec::normalize_path), and this PR adds the seventh (repo_law.rs:329) in the same diff that makes mcp::normalize_path_components pub(crate) for reuse elsewhere — two opposite choices, one PR. resolve_deepest_existing duplicated byte-for-byte with the duplication documented rather than fixed, which is how B18-1 happened. Three dedups. Five root-canonicalization policies. Three copies of the "list 5 then +N more" renderer, one with a wrong remainder. Two clear-and-re-derive pairs open-coded at two security-critical subagent sites, where the same two sites have already diverged once: the fresh-spawn site re-scopes plugin_registry with an explicit isolation rationale (mod.rs:9418-9427) and the resume site does not, so a resumed worktree child keeps the parent's plugin authority receipts — the precise leak the fresh-spawn comment forbids. The body calls them "the same idiom"; they are not.
Smuggling — FAIL. I am overturning round 17's PASS here.
Round 17 graded "all 92 files classify as core topic or directly-forced adjacent; zero class-C". Walking every hunk, I count ~1091 of 8629 changed lines (~12.6%; ~22% of the production change) that are independent work:
| what | where | lines |
|---|---|---|
| Archive-stamp + policy-preservation SQL rewrite | state/src/lib.rs:696-830, core/src/lib.rs:1008-1044, 6 tests |
~290 |
| Empty/relative-workspace hardening across 8 intake surfaces | tui/src/lib.rs:8092-8120 + 11 call sites, mcp_server.rs, acp_server.rs, runtime_threads.rs, apply.rs, core/src/lib.rs |
~191 |
PUT /v1/sessions → 409, plus a silent metadata.workspace overwrite |
runtime_api/sessions.rs:739-808, session_manager.rs:354 |
~133 |
| Worktree launch-manifest isolation fallback | subagent/mod.rs:5878-5967 + test |
~128 |
Missing-thread resume/fork → HTTP 404 / stdio -32004 |
app-server/src/lib.rs:670-693, 2090-2097 + test |
~87 |
| Localization test-harness extraction + meta-test | localization.rs:5216-5290 |
~78 |
| Approval grouping-key rekey | approval_cache.rs:27,94-145 + test |
~53 |
| Untrimmed write target in auto-review carve-out | tui/auto_review.rs:389-396 + test |
~48 |
Exec ask-rule judges the cwd: operand |
engine.rs:8002-8024, 8116-8145 |
~28 |
| 16 MiB stack test wrapper | acp_server.rs:2634-2652 |
~18 |
/cd busy-guard widening + localization |
session_state.rs:741-747, 812-820 |
~17 |
| Three clippy-1.98 fixes | shell_dispatcher.rs:759, web_search.rs:4652, engine/tests.rs:24949 |
~10 |
writable_roots canonicalization |
sandbox/policy.rs:313-323 |
~9 |
string_field trim removal |
engine.rs:8188 |
1 |
The clippy commit is the cleanest violation in principle even though it is the smallest: shell_dispatcher.rs and web_search.rs appear in this 95-file PR for one lint each and nothing else, which is the author's own "in lanes this PR does not touch" — that phrasing is the argument for splitting, not for keeping. Disclosure does not convert a separable change into a scoped one.
The archive-stamp rewrite deserves a separate note, because it is both the largest ride-along and dead code. persist_thread has exactly two callers — core/src/lib.rs:659 (status built as Running at :645) and :764 (status forced to Running at :754) — so archived: matches!(thread.status, ThreadStatus::Archived) is always false and the new CASE WHEN excluded.archived = 0 THEN NULL ELSE threads.archived_at END arm resolves to the THEN NULL branch every time, byte-identical to base's archived_at = excluded.archived_at. The three tests that "prove" it hand-set payload.archived = true (state/src/lib.rs:2427, 2489, 2525), a shape no caller emits. So ~290 smuggled lines defend behavior that does not exist — and the related body claim ("a cached resume of an archived thread no longer nulls archived_at") describes a before-state that also does not exist: base's cached-resume branch returned early and never touched the store. The COALESCE(excluded.sandbox_policy, ...) half of the same rewrite is a real TOCTOU removal and is worth keeping; the archive arm should be deleted or given a caller.
Nine behavior changes visible to single-root users are not in the body's dedicated list: the auto_review.rs:392 trim removal (the body discloses only its engine.rs twin), the exec ask-rule move to the resolved cwd: operand, the approval-grant key change (which invalidates every stored shell grant carrying a cwd), the writable_roots canonicalization, the relative---workspace absolutization, the PUT workspace overwrite, the claim-less worktree-child isolation flip, the /cd guard widening, and the updated_at gating on parameterless cached resume.
And CHANGELOG.md is not touched at all, despite two hard protocol breaks. Round 17 graded "CHANGELOG absence is compliance, not a gap" — that is right for an ordinary feature on this fork, but a 200→409 and a 200→404/−32004 on documented client-facing surfaces are exactly the case the exemption does not cover. Either the breaks get an entry, or they get their own PRs where they can.
Defects — FAIL (three P0, three P1 above)
Convergence
- Close B18-1 and B18-2 by making
authority::resolve_deepest_existingpub(crate)and giving both callers the raw candidate, and by resolving thecwd:operand throughToolContext::resolve_pathon the check side so the ask rule, the grouping key, and execution all name one path. Hash or length-prefix the grouping key components. - Close B18-3: compare the validated (trimmed) id, and make lock poisoning fail closed. Then either give the guard a cross-process mechanism or stop describing it as protection.
- Close B18-4 and B18-5 at the chokepoint: lexically normalize every accepted root and reject a result that is
/or still carriesParentDir; reject or absolutize non-absolute cwds on all six surfaces (resolve_workspacealready shows the shape). ThenRUNTIME_API.md:1040becomes true. - Close B18-6: build the new tests' paths from an OS-appropriate absolute base (or
cfg-gate them), andworkflow_dispatchci.ymlagainst this head so a Windows verdict exists before merge. - Split out, in descending value: the preserving-upsert/archive repair (and delete the unreachable arm plus the three impossible-input tests); the empty/relative-workspace hardening; the worktree-isolation fallback; the 409 and the 404/−32004, each with a
CHANGELOG.mdentry; the approval-key rekey (it invalidates users' stored grants); the clippy fixes; the localization-harness and 16 MiB test-infra changes. - Decide the product question. Either land at least one shipped entry point (
--add-diron cli/exec/tui) and fix the default-path search/read tools so an attached root is reachable without absolute spellings — or reframe this as infrastructure, say so in the title and body, and stop describing an end-to-end capability. As it stands no user of the shipped CLI or TUI can turn the feature on, and the first thing an agent does in an attached repo — a default-path search — silently answers from the wrong repository.
Happy to re-review on the next push, and to take any of the split-outs myself if that is faster.
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 19 — fresh full review of d872c6663 (independent re-derivation, 7 parallel axes)
This round re-reviewed the PR from scratch: every load-bearing claim re-verified against the code, the full test record re-run locally (macOS arm64, warm cache), and four of the new guards re-proven red-green by mutation. Delta verdicts against round 18:
| Axis | R18 | R19 | Why |
|---|---|---|---|
| Value | FAIL | FAIL (unchanged) | The runtime-lane closure is real, honest, and root-cause-shaped — but the producer/completeness position is unchanged: still no entry point outside POST /v1/threads, consumers still primary-rooted |
| Elegance | FAIL | PASS-WITH-CONCERNS | Thread-level multi-root is the minimal sound design, layering is clean, and upstream has nothing to reuse. Residuals: same-crate helper copy; "single chokepoint" is aspirational for execpolicy |
| Smuggling | FAIL | PASS-WITH-CONCERNS | Smuggled content is now 0 lines (hunk-by-hunk classification); every formerly-flagged item is coupled or disclosed. Remaining gap: CHANGELOG (B19-3) |
| Defects | FAIL | FAIL (one blocker) | B18-1 verified fixed; no new P0/P1; B18-2 still open → the PR stays blocked |
Verified fixed — round-18 blocker B18-1 (repo-law lexical/kernel divergence)
Confirmed closed at this head, on all judgment inputs:
normalize_lexical_componentsdelegates tocrate::tools::spec::normalize_path(crates/tui/src/repo_law.rs:329-331) — the exact normalizer the exec lane applies to the candidate (tools/spec.rs:1102,:1359-1398), so law and gate can no longer drift by construction.- Candidate derivation mirrors
resolve_path(absolute as-is / join onto the raw workspace, untrimmed,repo_law.rs:283-288); roots are canonicalized once with raw fallback — the sameboundary_rootsidiom execution uses (repo_law.rs:85vsspec.rs:1082-1086);resolve_deepest_existingstrips against the canonical root exactly ascarve_out_target_allowed(repo_law.rs:307-314vsauthority.rs:578-590); absolute targets dual-strip raw+canonical (:252-256,:294-297). - Bypass spellings re-adjudicated: overshoot clamped and held (pins
:892-939), symlink-alias root held (:792-822), interior symlink held via resolved strip (:826-855), ancestor-root relative landing held (:962-989); trailing separators and//are harmless (component-based strip); case-aliased spellings fail closed on both sides — exec refuses withPathEscaperather than admitting past the law. - Mutation check: reverting the delegation to identity turns the overshoot pin, the clamp unit pin, and the sibling
..-spelling pin red (3 red) — the guard is real and pinned.
B18-1 is closed. This was the hard one.
Still open — round-18 blocker B18-2 → B19-1 (merge blocker): padded session-id bypasses the live-session 409
Unchanged at this head. The registry stores trimmed, the read/write paths trim, but the guard compares raw:
set_live_sessionstoressession_id.map(str::trim)(crates/tui/src/session_manager.rs:314-321) andvalidated_session_idtrims on every load/save (:986), whileis_live_sessiondoes an exactcontainson the raw string (:344).- PUT chain: client sends
session_id: " abc-uuid "→runtime_api/sessions.rs:749-750checks the raw string (no trim; the request struct at:107-117does no serde normalization) → guard false → 409 skipped →load_session(existing_id)(:781) trims to the same file and overwrites the live session's document, roots included. - PATCH chain: raw
Path(id)(sessions.rs:196-231) reachesrename_session/set_session_archivedviaSessionMutator::External(session_manager.rs:1633,:1700) — same bypass. - Secondary (P3): the guard fails open on a poisoned lock (
is_ok_and,:344-348);set_live_sessionno-ops on a poisoned write lock (:314).
Severity is debatable (bearer-token-gated local API; the guard arbitrates rather than contains), but this was a round-18 blocker, it is still unfixed, and the fix is small: normalize the id once at the API boundary (trim + reject empty) before the guard — or make is_live_session compare trimmed — plus a pin that a padded-id PUT /v1/sessions against a live session still answers 409.
New B19-2 (P2): attaching a super-root is an unvalidated sandbox-widening decision
Nothing in intake prevents workspace_roots: ["/"] — or /.. (which normalizes to / in every comparison), $HOME, or the primary's parent:
- Intake (
crates/core/src/lib.rs:90-111) accepts any absolute root;workspace_write_policycopies the set verbatim into per-turnWorkspaceWrite.writable_roots(crates/tui/src/core/authority.rs:401-414);get_writable_rootscanonicalizes and hands them to the sandbox profile generators (crates/tui/src/sandbox/policy.rs:302-320). Net effect: one attached/makes the sandboxed exec lane filesystem-writable in Ask posture, modal-free. - Verified limits that keep this P2 rather than worse: the write carve-out does not follow a super-root unless it is itself a git work tree (
root/.gitmarker,authority.rs:547-562; attaching a repo's.gititself fails the marker check); repo-law constitutions still judge every landing path; execpolicy scoped Allow rules can never fire on a super-root cwd (normalize_workspace_scope("/")→None,execpolicy/src/lib.rs:1090-1120). - The body's known-scope section discloses "not canonicalized at intake" but not the sandbox consequence. This deserves an explicit decision rather than a default: reject super-roots (at minimum the filesystem root and the primary's ancestors) at intake, or warn and document the sandbox semantics — either way, with a pin.
New B19-3 (P2): behavior breaks are not recorded — CHANGELOG untouched, the 404 break is in no doc
CHANGELOG.md maintains an active ## [Unreleased] section and this PR does not touch it, while the diff ships several single-root-visible behavior breaks. The PUT 409 is documented in RUNTIME_API.md, but the missing-thread resume/fork break (was 200 + status:"missing", now HTTP 404 / stdio -32004) is recorded nowhere in the repo. Please add Unreleased entries for the observable breaks (404/-32004 on missing-thread resume/fork, PUT 409, relative --workspace absolutization, string_field trim removal, worktree exec-lane confinement) and a RUNTIME_API.md paragraph for the 404.
New B19-4 (P3, body): the ~ disclosure is false
The known-scope section says "~ spellings and non-existent roots still pass untouched." They don't: normalize_workspace_roots drops non-absolute entries, so ~/shared is silently discarded at intake — the state reader warns only for already-persisted rows (crates/state/src/lib.rs:2165-2196); API intake is silent, and the caller's declared set narrows with zero feedback. Fail-closed, but the disclosure must be corrected — and ideally intake should warn (or error) on dropped entries instead of silently shrinking the set.
Verified clean this round (so later rounds don't redo it)
- Test record reproduced digit-for-digit (macOS arm64): fmt + the strict clippy gate clean; protocol 92 (70 lib + 22 parity), core 102, state 27+6 parity, execpolicy 123 + 3 Linux-cfg, app-server 104, command-contract 43; TUI workspace_roots 29 / repo_law 29 / carve_out 15 / session_manager 73 / runtime_api 188; wider union runs green.
- Mutation red-greens 4/4: intake guards → intake pins red; repo-law delegation → overshoot/clamp pins red;
is_live_session→ the 409 pin red;rederive_sandbox_policy_rootspassthrough → both worktree-resume pins red (plus its unit pin). - Merge state: cleanly mergeable with
pinvou3-clean@61cb769be(18 overlapping files auto-merge clean); the eventual merge should still get a real CI run on the combined tree. - Smuggling: 0 unrelated lines in the whole diff; the clippy side-rides are 8 lines and disclosed. Commit hygiene: 49/49 commits author=committer with a matching DCO sign-off; no WIP, no leftovers, zero TODO/dbg! added.
- Reuse check: upstream
dcd4c200fcontains no multi-root mechanism — nothing reinvented against upstream; zero dependency-edge changes. - Disclosure accuracy: 13+ disclosures sampled against code, all matched — B19-4 is the only inaccuracy found.
Non-blocking notes for the record (P3; post-merge fine)
- Perf (multi-root only): repo-law re-derives per root — the patch payload is re-parsed, law rules re-loaded and globset recompiled, and the identical candidate re-canonicalized per root (
repo_law.rs:80-110); carve-out lost its hoisted setup (per path × root). Hoist per-root invariants when this lane is next touched. Single-root is cost-neutral. - Intake polish cluster: relative
workspaceon POST/PATCH answers 200-OK for a thread whose boundary set collapses to empty (dead on arrival — reject where empty is rejected); explicitnullroots hard-fails deserialization on the Vec-typed lanes while resume/fork/PATCH accept it; no size cap on the root set (the authority envelope caps writable_roots at 32); trailing-slash spellings defeat dedup so the primary can be advertised as its own attached root; root strings render unescaped into theAccessible folders:meta line (trusted-client injection only). - Windows: ~20-25 new test fns carry ungated POSIX-absolute literals and would fail on a Windows test leg; fork-ci runs ubuntu-only so this is dormant debt, but the PR widens it.
- Elegance follow-ups:
repo_law::resolve_deepest_existingduplicatescore::authority's private helper in the same crate (make itpub(crate));normalize_workspace_rootscannot be the chokepoint for execpolicy by dependency direction — the pure-std helper belongs one level down (protocol); session-side roots pairing is writer-by-writer with three coexisting empty-set semantics (one save/merge chokepoint would retire the warn-only backstop); ~13 comments still cite review-round numbers (strip at squash).
What closes this round
- B19-1: normalize/validate the session id at the API boundary + a padded-id 409 pin.
- B19-2: super-root intake decision (reject or warn + document) + a pin.
- B19-3: CHANGELOG Unreleased entries + a RUNTIME_API.md paragraph for the 404.
- B19-4: correct the body's
~sentence (an intake warn would make it moot).
Requesting changes on the strength of B19-1 alone; the rest is fix-in-passing.
… set Review Pinvou#54 round-9 should-fix minimum close-out: the storage convention makes workspace a member of the declared root set, but a writer that moves one without the other used to persist silently. The guard lives in save_thread — the single choke point all persistence callers share — so the failure mode becomes a searchable signal instead of silent persistence damage. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
d872c66 to
5b37c58
Compare
Round-19 addressed (head
|
|
Follow-up on the round-19 close (head |
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 20 — fresh full re-review of 18c50a3f6
Method. From-scratch re-derivation, not a carry-over: 21 parallel review shards over the full range 61cb769be..18c50a3f6 (96 files, +8,785/−467) — closure verification (B19-1..B19-4 and the head commit), two adversarial security hunts (live-session faces, root-set intake), full-depth dives (core/state, repo_law, exec/approval/sandbox, runtime HTTP/threads, protocol/stdio/ACP, session/TUI layer, subagent/worktree), the four rubric axes re-derived independently, a hunk-by-hunk smuggling census, a body-claims audit, a test-quality census, rebase-interaction adjudication, and commit hygiene — plus my own line-level re-read of every P0/P1 below and a local test run (Linux arm64). The rebase was verified mechanically (git range-diff against the round-12 line: 41/41 commits all-equal; the 8 commits between the round-12 and round-18/19 heads were reviewed at final-tree level) and semantically (a dedicated shard adjudicated all priority pairs against the 7 new base commits — #63/#65/#68/#69/#70/#71/#76/#77 — every pair composes, no stale dual-touched pins).
Verdict: Request changes. The round-19 closures are real and well-pinned, the rebase is clean, and the test record reproduces. But this round re-derives one round-18 P0 that silently dropped off the fix list — through a numbering error in my own round-19 review, which I account for below — plus three P1s and a body whose three strongest safety sentences are contradicted by the tree it ships.
Blockers
B20-1 (P0, re-raise of round-18 B18-2 — never adjudicated, still open): the exec judged-cwd is lexical; execution canonicalizes
First, the lineage accounting, because it is my error to own: round-18's B18-2 was the exec judged-cwd split and B18-3 was the padded session-id. My round-19 heading said "round-18 blocker B18-2 → B19-1: padded session-id" — that mapped B18-3's content onto B18-2's number. You closed exactly what round-19 asked (the session id), and the exec-lane P0 silently left the fix list. No reviewer re-adjudicated it; no disclosure covers it. This round it was re-derived independently from the current tree before the lineage was reconstructed.
The code, crates/tui/src/core/engine.rs:8146-8165 — the comment concedes the defect in its own words:
// The join mirrors execution ... and stays lexical,
// matching `normalize_workspace_roots`.
let effective_cwd = ["cwd", "working_dir"].iter()
.find_map(|name| tool_input.get(name).and_then(Value::as_str))
.map(|dir| { ... crate::tools::spec::normalize_path(&joined) });Execution, crates/tui/src/tools/shell.rs:~5085: context.resolve_path(dir) — canonicalizes existing components (symlinks included) and admits against canonical attached roots. workspace_scope_matches (execpolicy/src/lib.rs:1124-1131) is exact equality. The trace from round 18 stands verbatim at this head: primary /ws, exact-scoped Allow rule for git push scoped to /ws, /ws/link → /attached/repo, call {"command":"git push","cwd":"link/.."} → judged cwd = normalize_path("/ws/link/..") = /ws → exact match → Allow fires, no modal → execution resolves through the symlink → runs git push in the attached repository. Amplified by the "always allow" capture (approval/ask_rules.rs:164-186), which pins the rule to the session workspace regardless of the operand that triggered it. TOCTOU between the turn-loop check and BashTool::execute also remains.
This contradicts the PR body's strongest sentence — "nothing auto-approves execution under an attached root, redirected or not" (and "a scoped allow no longer auto-approves execution redirected into an attached root") — which the tree's own comment refutes. The engine-side lexical tests (engine/tests.rs:9629/9642/9665) all use ./absolute spellings; no test exercises the symlink+.. class.
Fix: judge the operand through the same roots-aware resolution execution uses (ToolContext::resolve_path, or re-validate at execute time), which closes the TOCTOU too; add a symlink-interior and symlink+.. pin; correct the body sentences and the engine comment.
B20-2 (P1, new): the round-16 grant re-key is bypassed by cwd: null + working_dir
shell_cwd_operand (approval_cache.rs:141-147):
input.get("cwd").or_else(|| input.get("working_dir")).and_then(Value::as_str)get("cwd") returns Some(Value::Null), which blocks the or_else; as_str on Null is None → the grant is keyed to the no-operand family shell:{prefix}. The check side (engine.rs, as_str inside find_map) and execution (first_present_field skips Null) both treat the same call as redirected to working_dir. Chain: the user approves plain git status once for the session; later {"command":"git status","cwd":null,"working_dir":"/attached/repo"} hits the no-operand key → session-approved → runs in the attached root unprompted. This defeats the invariant the PR's own pin (shell_grouping_key_rekeys_on_the_cwd_operand) asserts, and the comment above the key ("the same value exec rule matching sees") is false for this spelling. Fix: mirror the check-side field semantics (skip Null, try working_dir), and hash or length-prefix the key components — the raw prefix@cwd: concatenation is non-injective (model-craftable collisions).
B20-3 (P1, re-raise of round-18 B18-3's third defect): the live-session 409 guard never fires in any shipped topology
Verified by census: set_live_session's only production caller is tui/ui/frame.rs:988 (TUI snapshot path); run_http_server's only caller is the Serve subcommand (lib.rs:2562), mutually exclusive with the TUI; no axum listener exists under tui/; the static registry is process-local. So in every shipped process either the registry is empty (serve) or there is no HTTP server (TUI). The PUT-409 "breaking change" — headlined in the body, CHANGELOG, and RUNTIME_API.md — is unobservable; B19-1's fix (verified correct: trim + fail-closed + recovery, all pins non-vacuous) hardens a path that cannot engage. RUNTIME_API.md's "a standalone codewhale web … is never blocked" frames as the edge case the only existing case. Decide the guard's fate: either a cross-process mechanism (or an embedded server in the TUI process) that makes the protection real — in which case also fix the two residual holes below — or remove the guard and retract the breaking-change claim from body/CHANGELOG/RUNTIME_API.md. Residual holes if kept: (a) on case-insensitive filesystems (macOS/Windows defaults) an uppercase-spelled id folds onto the live owner's file while missing the case-sensitive registry — the padding bug's class, reopened by case; (b) DELETE /v1/sessions/{id} has no live check and destroys the live session's goal sidecars and runtime dir (the retention sweep cleanup_old_sessions can delete a live session too — its sibling pruner has exactly the live-awareness keep that this path lacks); (c) stale claims are never cleared on /new//fork//resume` (false 409s until restart).
B20-4 (P1): the round-19 intake rejection is lexical and defeatable by ordinary symlink spellings
validate_workspace_roots (core/src/lib.rs:143-169) judges normalize_lexical_components output; enforcement (sandbox/policy.rs:320-323, spec.rs boundary_roots, carve-out) canonicalizes per root. Two working defeats of the fs-root and ancestor rejections: (a) realpath ancestor — macOS, primary /tmp/proj, attach /private (or /var): lexically a sibling, canonically the primary's parent → accepted, and the sandbox grants it writable; (b) child-spelled link — any symlink to / inside the workspace passes as a lexical child. Round 19 chose reject-over-warn on the premise of fail-closed intake; the rejection is advisory for any root whose spelling differs from its canonical form — which on macOS is the system tmpdir. Minimum acceptable: qualify every description (body, RUNTIME_API.md intake paragraph, CHANGELOG) as lexical-only; the real fix is canonicalize-at-intake or re-running the ancestor/fs-root checks where roots are materialized (get_writable_roots) — the body already schedules canonicalize-at-intake, so this is a decision to promote, not new work.
B20-5 (P1, body): the three strongest safety claims are contradicted by the tree, and the quantitative record is stale
- "nothing auto-approves execution under an attached root, redirected or not" — false (B20-1).
- The re-key trigger "when the call carries
cwd:/working_dir:" — false (B20-2). - "typed error" ×3 for intake rejections — they are
anyhow!strings (core/src/lib.rs:150-162); the body's own post-fix audit concedes stdio maps them tointernal. - "only
POST /v1/threadscreates a roots-bearing thread" — false; stdiothread/startcarriesworkspace_roots(app-server/src/lib.rs:1377) — the body's own Summary and CHANGELOG say so. - Stale after the rebase the body itself discloses: the Test record cites head
d872c6663, "49 commits, 95 files, +8,169/−460"; at this head the range is 51 commits, 96 files, +8,785/−467 over61cb769be. The fork-policy figure "16.1×" was measured pre-rebase:git diff --shortstat dcd4c200f..HEADat this head is 286 files, +39,430/−7,192 → net 32,238 ≈ 21.5×. Understating the fork surface is the wrong direction for the fork-policy duty, and the parent re-pin must use the fresh figure. - CHANGELOG
[Unreleased]still lacks six single-root-visible breaks round 18 named: the approval-grant re-key (invalidates stored grants), the PUT workspace-overwrite pairing, the/cdreceipt severity change + guard widening, the cached-resumeupdated_atbump, theisolated_worktreefalse→true flip, and thestring_fielddeny-rule narrowing (its bullet frames the trim as diagnostics-only).
Should-fix
/cdand/fork <id>mint the exact poisoned rows the runtime lanes reject — and persist them (three shards converged).switch_workspace(session_state.rs:755-767) filters only the old primary and calls the tolerant normalizer, thensave_sessionpersists — while its own comment claims alignment with "the runtime PATCH workspace-only branch andresolve_resume_roots", both of which now reject an entry that becomes an ancestor of the new primary./fork <id>(session.rs:160-178) re-anchors a cross-workspace source tolerantly. Consequence is the one 18c50a3 itself names: the widened row strands a later bare fork/resume-move with a hard error. No escalation (the ancestor was already writable pre-switch), operator-triggered, self-heals on the next/cd— but it is a durable invariant violation one lane over from the round you just closed. Run the rebased set throughvalidate_workspace_roots(or drop-and-disclose ancestors) before persisting.- Resume-lane worktree children keep the parent's plugin registry. Fresh spawn re-scopes (
mod.rs:9495-9498, with a comment calling the leak an isolation boundary); the resume clear site (:5937-5957) does not — under a comment claiming "Same isolation rule as a fresh worktree spawn". Workspace-scoped plugins and their authority receipts (registry.rs:236-240computes authority from the parent workspace) cross into the isolated child. Pre-existing shape, but the PR codified the parity claim; one line at the existing clear site closes it. - The disclosed worktree fail-open is wider than stated and tests the wrong leg.
is_terminalincludesInterruptedandprune_worker_recordsevicts terminal records, soInterruptedOnly— the policy both end-to-end pins drive — is equally exposed, while the disclosure names onlyInterruptedOrCompleted, the untested policy. Correct the disclosure; ideally add the manifest-persisting fix shape. POST /v1/snapshots/{id}/restoreis the one rollback face without the boundary clause (runtime_api.rs:5875-5877— every other face carriesATTACHED_ROOTS_NOT_REVERTED_NOTE), and the "one pin per face" claim over-counts: the runtime patch-undo note has no test.- A relative primary silently discards the declared set (
validate_workspace_rootsearly-returnsOk(Vec::new()); POST answers 200) — contradicting the layer's own "never silently reshape" contract and the create-lane comment "validation also requires it to be absolute" (it does not). Fail-closed, so a decision: reject relative primaries where empty is rejected, or document the collapse. - The primary slot admits the filesystem root (
workspace: "/"→ sandbox/) one field away from the roots-slot rejection the same round shipped. Disclosed as P3; two shards rate P2 — the asymmetry is stark and the fix is the same predicate. - The engine rebuild path silently re-arms forbidden persisted rows (
ensure_engine_loadedclones verbatim; normalize-only sinks; the pairing warn cannot see super-roots). A warn (or loud failure) at engine-build for fs-root/ancestor entries matches the theme's own threat model. - ACP
session/loadof a legacy poisoned row never fails loud — no roots override exists on that lane, so the "re-declare" remediation the body describes is unreachable there. Undisclosed nuance; one sentence fixes it. - The archived_at preserving-upsert cluster (~305 lines incl. tests) is dead-defensive at HEAD — both
persist_threadcallers forceRunning, the writeback uses the targeted UPDATE, so theCASE WHEN archivedpreserve arm is unreachable and its disclosed user-visible effect no longer exists on any path. Split it out or reduce to the topic-minimal form; carrying an unreachable SQL arm with trivially-passing tests is the one smuggling-adjacent residue this PR has left (disclosed and self-declared, to its credit).
Axis verdicts
Value / root cause — FAIL on completeness (unchanged; the round-18 ultimatum stands un-taken)
The persistence/materialization machinery attacks the right root cause at the right chokepoints, and the enforcement half is independently valuable — I re-verified against base that a worktree child's sandboxed exec lane inherited the parent's writable_roots (subagent/mod.rs clear site reset only workspace; the cloned policy carried vec![parent_workspace]), which rederive_sandbox_policy_roots closes even single-root. But completeness is unchanged: zero interactive producers (TUI /workspace swaps the primary only; no /roots or attach arm; CLI is a single --workspace; ACP session/new hardcodes empty; config has no key) and the first default-path action of an activated session silently betrays it — grep_files/list_dir/file_search default to the primary (search.rs:116, file.rs:2699, file_search.rs:109; resolve_path joins relative paths onto the primary at spec.rs:1091) while turn_meta advertises Accessible folders:. The body does not claim an interactive capability in so many words, but "end to end" in the Summary frames a programmatically-declarable sandbox property as a shipped feature. Minimum completion scope is small against 8.8k lines — the /cd pipeline already has receipts, busy-guards, and 15-locale localization, so an attach arm is incremental; and the default-path tools either iterate boundary_roots or fail with guidance naming the roots. Either land that, or retitle/reframe as infrastructure — the third round in a row this decision has been asked for.
Elegance — PASS-WITH-CONCERNS (leaning FAIL)
Measured, not vibed: 23 production signatures gained the parameter; 15 new pub workspace_roots fields; 28 hand-normalization call sites; 6 normalizer families encoding 3 ..-handling semantics — including two same-named normalize_lexical_components with opposite .. semantics (core drops unpoppable ..; repo_law's delegates to spec::normalize_path which keeps them); a byte-twin resolve_deepest_existing whose two callers feed different arguments; 3 capped-list renderers (arithmetic currently agrees; one latent len−1 divergence); five coexisting empty-set semantics spelled in prose; and crates/paths — dependency-free, already consumed by 7 crates, 0 diff lines — sitting exactly where a WorkspaceRoots value object (explicit Inherit/Clear/Set, validate-constructor) would delete the execpolicy String-dedup, the call-site ambiguity, and most of the sprawl. Three contract comments contradict their code ("typed", "never silently reshape", "stays lexical … mirrors execution"). Highest-leverage: the crates/paths newtype; make authority::resolve_deepest_existing pub(crate) and delete the twin; extract one format_root_list.
Smuggling — PASS-WITH-CONCERNS
Hunk-by-hunk across all 96 files: zero undisclosed-and-independent lines. Class C is ~311 lines (3.5%), 100% disclosed: the clippy-1.98 trio (6 lines, fine) and the archived_at cluster (~305 lines — should-fix 9). Round-19's two commits map 1:1 to the requested closures with no ride-alongs. All 51 commits: linear, author=qiuYliangM, one matching DCO sign-off each; the 49 rebased commits are committer=asto18089 — an expected rebase artifact, not a violation.
Defects — FAIL (one P0, four P1 above; no new P0 in the shipped multi-root mechanism itself)
Test record (reproduced locally, Linux arm64, this machine)
fmt clean; cargo check --workspace --all-targets --locked green; suite counts reproduce digit-for-digit: core 106, runtime_api 192, session_manager 75, workspace_roots 29, repo_law 29, carve_out 15, sandbox::policy 15, approval_cache 18, session_state 18, command-contract 43, parity 22, app-server 104, acp_server 59. The strict clippy gate fails on this machine's stable-1.98.1 with 3 pre-existing eprintln! hits — base fails identically, so it is toolchain drift, not this PR; the body's clean-gate claim holds for its CI. Test-quality census: 12 pins sampled adversarially, none vacuous (most carry positive controls and persisted-row-untouched asserts — above-average pin quality); gaps worth one line of disclosure each: the runtime patch-undo note unpinned, the PATCH workspace-only rejection unpinned, the /cd set-transform untested, the approval-side effective-cwd tested only in the lexical regime it already handles (B20-1), and ~38 new test fns carry ungated POSIX-absolute literals (disclosed debt).
What closes this round
- B20-1: judge the exec operand through the same resolution execution uses (or re-validate at execute) + a symlink+
..pin; fix the engine comment and the body sentence it refutes. - B20-2: align
shell_cwd_operandwith the check/exec field semantics (skip Null) and harden the key (hash or length-prefix). - B20-3: decide the guard — make it real (cross-process/embedded; then also fix case-folding, DELETE/retention live-awareness, claim clearing) or remove it and retract the breaking-change framing everywhere.
- B20-4: canonicalize at intake or at
get_writable_rootsmaterialization — or qualify every rejection-class description as lexical-only, explicitly. - B20-5: body corrections (the three safety sentences, "typed", the producer claim, fork figure ≈21.5×, Test record refreshed to this head) + the six missing CHANGELOG entries.
- Should-fix 1 (the
/cd//fork` mint) — the same intake rule the head commit just shipped, one lane over.
Happy to re-review on the next push.
Round-20 addressed (head
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 21 — re-review of the round-20 remediation (faa1190b8, one commit on the reviewed 18c50a3f6)
Method. The tree delta since the round-20 review is exactly one commit, verified additively (git rev-list --count 18c50a3f6..faa1190b8 = 1, merge-base --is-ancestor holds; base unchanged at 61cb769be). I ran 9 verification shards over the remediation: per-blocker closure checks for B20-1..B20-5 and should-fix 1, a full body-claims re-audit, a residual-minter census over every workspace_roots writer, hunk-by-hunk smuggling and commit-hygiene audits, and fresh value/elegance re-derivations — plus my own line-level read of every finding below and a local run (Linux arm64). The round-20 P0 is genuinely closed on the default path, B20-2/B20-4/should-fix-1 are closed, and the code core is sound. What remains is smaller than round-20 but real: one code residual that recreates the closed bypass behind an opt-in setting, two disclosure-integrity failures in the body, and a registration record measured at the wrong head.
Verdict: still request changes — 3 P1 + 3 P2 (round-20: 1 P0 + 4 P1).
What closed (verified, with credit)
- B20-1, default path: the judged-cwd canonicalization closes the
link/..exploit; the new unix-gated pin is non-vacuous (leg 2 fails red against the old lexical code; the control leg proves the harness is live). - B20-2:
shell_cwd_operandnow skips Null/non-strings exactly like the check and execution sides (all three adjudicated spelling-by-spelling, including thecwd:""corner); the pin is red in both directions against the old code. No other site in the tree reproduces the Null-blocking class on an authorization operand (full census). - B20-3 removal: complete and correct in code — no dangling callers, no unused imports, the retained registry consumer is real and load-bearing (
reclaim_orphaned_session_dirs, session_manager.rs:1835, runs on every save; the TUI session has no other protection), last-write-wins holds at the store layer, and exactly four route pins went with the guard. The case-fold and claim-clearing residuals are genuinely moot at this head. - B20-4: all four surfaces (body, RUNTIME_API.md, CHANGELOG, validator doc) qualify the checks as lexical, both defeats are named, and the scheduled promotion is honestly absent from code.
- B20-5: the three safety sentences, the producer claim, and the error-mapping wording now match the tree precisely; 5–6 of the 6 CHANGELOG entries are code-backed.
- Should-fix 1:
/cdand/fork <id>both validate the re-based set against the correct primary before any swap/persist, refuse cleanly (no partial state, no half-persisted fork), and match the lanes' reporting conventions.
Test record reproduces exactly on my machine: cargo fmt --check clean; core 106, runtime_api 188, session_manager 75, workspace_roots 29, repo_law 29, judged-cwd pin 1/1, approval re-key pin green. The full tui lib run shows 2 failures in this environment (model_inventory::ollama_default_prefers_live_local_tags_over_the_unresolved_marker, remote_control::separate_predispatch_crashes_on_one_run_get_distinct_recovery_turn_ids) — both modules are untouched by the diff and the failing set shuffles between runs (a base rerun failed two different remote_control tests), i.e. environment-flaky, not a PR regression. Note: the body's "two session_state locale-dependent tests fail on this machine" did not reproduce here — session_state filters 18/18 green on an EN-locale machine — so that disclosure should name the tests or be dropped.
Blockers
B21-1 (P1, body): the removed 409 breaking change is still promised — by the same body that claims it retracted
/tmp/cw-pr54-r21-body.md line 48 (Known scope) still opens: "Breaking change, disclosed: PUT /v1/sessions now answers 409 (was 200) when the write targets a live session's document…" — the tree removed the guard (correctly, per the sanctioned option b), and line 97 says "The breaking-change framing is retracted from the body, CHANGELOG, and RUNTIME_API.md." CHANGELOG and RUNTIME_API.md did it; the body's own disclosure bullet was never edited. Two sections of one body now contradict each other and the tree — this is the B20-5 class on the exact finding where it was raised. Fix: delete or rewrite the line-48 bullet.
B21-2 (P1, body): "Should-fix 2–9 are recorded as post-merge work, not silently dropped" is false for 5 of 8
Checked each against the body text: recorded — SF-5 (relative primary collapse, line 56/91) and SF-6 (fs-root primary, line 89); half-recorded — SF-9 (archived_at: admitted in a CHANGELOG parenthetical, absent from the body's post-merge lists). Absent entirely — SF-2 (resume-lane plugin-registry re-scope: "plugin" appears nowhere in the body), SF-7 (engine rebuild silently re-arms forbidden persisted rows), SF-8 (ACP session/load of a legacy poisoned row never fails loud — and line 89's "remediable" is false on this lane; there is no re-declare path on it). Still wrong, not just unrecorded — SF-3: line 50 still names only InterruptedOrCompleted, while is_terminal (subagent/mod.rs:587-592) also includes Interrupted; SF-4: the restore endpoint (runtime_api.rs:1260, handler :5865) still returns {"restored": id} with no boundary clause and zero pins, while line 52 claims "every rollback surface … names that boundary" (true only because the enumeration omits that face). The review history has treated body-vs-record contradictions as blockers since round 19; this sentence is one.
B21-3 (P1, code+comment): the judged-cwd fix's canonicalize-failure path repeats the round-20 defect one layer deeper
The fix comment (engine.rs:8156-8157) says "A nonexistent operand keeps the lexical join, matching resolve_nonexistent_path's behavior", and the body (line 97) says the judgment "canonicalizes the cwd:/working_dir: operand the way execution resolves it". Both are false on the fallback path: resolve_nonexistent_path (tools/spec.rs:1189-1232) canonicalizes the deepest existing ancestor and re-appends the suffix; it does not keep the lexical join. The divergence is reachable and recreates the exact B20-1 scenario under the supported, documented workspace_follow_symlinks: true (settings.rs:476, default false at :595): call {"cwd": "link/<absent>/../.."} — judgment: canonicalize fails, lexical normalize pops newdir/.. and link/.., lands on the primary → primary-scoped Allow, no modal; execution: resolve_nonexistent_path canonicalizes /ws/link → /attached/repo, and the follow_symlinks early return (spec.rs:1236-1241) skips the containment re-check → the command runs in the attached root. No race, model-craftable spelling. Under the default setting the same spelling is fail-safe (PathEscape), which is why this is P1, not a re-raise of the P0. Fix is mechanical and the right primitive already exists in-tree: judge via resolve_deepest_existing(...) + lexical normalize (see also the elegance note — the function was byte-identical-copied into repo_law.rs:337 instead of shared), correct the comment and the body sentence, and extend the pin with an absent-interior leg.
B21-4 (P2): two undisclosed fail-closed regressions from canonicalize-at-judgment
(a) Windows: std::fs::canonicalize returns \\?\-prefixed verbatim paths, which normalize_workspace_scope cannot match (is_windows_absolute_path, execpolicy/src/lib.rs:1198-1202, requires X:/) — so at this head every exact-scoped allow rule is ineligible for every existing operand carrying a cwd:, and all such exec calls silently degrade to modals on Windows. (b) Declared-vs-canonical spellings: captured ("always allow") and persisted rule scopes hold the declared spelling while judgments are now canonical, so on any system where the two differ (macOS /tmp → /private/tmp, /var → /private/var) those scoped allows silently stop firing for operand-carrying calls — including the feature's own captures. The new test dodges exactly this by canonicalizing its fixture spellings (tests.rs:26836-26838, its own comment). Neither regression is in the body or CHANGELOG. Minimum: disclose both (the Windows one needs a scope-normalization fix or a dunce-style trim; the macOS one needs the capture to store canonical spellings).
B21-5 (P2, body): the registration record is one commit stale and the parent re-pin would carry it
The body's fork triple "at this head" (286 files, +39,430/−7,192 ≈ 21.5×) is verbatim the diffstat at 18c50a3; at faa1190b8 it is 287 files, +38,268/−6,077 (net 32,191 ≈ 21.46× — the multiplier survives, the numbers don't). The rebase-fresh figures line 102 anoints for the parent re-pin ("96 files, +8,785/−467") are likewise the pre-remediation range; actual is 97 files, +8,824/−553 (GitHub-confirmed). Line 67 says the re-pin "advances it to d872c6663" — an object that does not exist (six cited SHAs fail git cat-file: d872c66, 8ba62fd, ad46698, e01e3a7, 0c366b9, 83bf482). Also false: "author = committer = qiuYliangM" (committer is asto18089 on 49/52 — rebase artifact, DCO clean, but the claim is wrong); "core 99" vs "106" and "runtime_api 192" vs "188" stand side-by-side in one Test record; "~13 review-round citations to strip at squash" is actually 37 lines / ~47 tokens, and the plan is mechanically unsound — these citations live in file contents (comments, RUNTIME_API.md prose), which squash does not touch; only the 7 in the commit message vanish. They need a real code edit before merge, or an explicit decision that they stay.
B21-6 (P2, code): exec --resume with a moved primary still mints a re-based row tolerantly
The census of every workspace_roots writer confirms should-fix 1 closed the interactive lanes — but the headless lane has the same defect one lane over: exec_agent.rs:209 reads the persisted set verbatim, :366-371 handles a moved primary with an eprintln warning ("Resuming anyway."), the engine build re-anchors normalize-only (engine.rs:1847-1849 — no ancestor/fs-root check), and persist_exec_session durably stamps the re-based workspace+workspace_roots pair (exec_agent.rs:917-925 → lib.rs:12415/12458). Result: a durably persisted ancestor-of-new-primary row plus a silently re-armed per-turn sandbox — the exact stranding/widening class B21/should-fix 1 refuses elsewhere. This falsifies body line 100 ("the runtime lanes' rule now holds on every lane that mints rows"). Either validate here too, or narrow the sentence and disclose the lane.
Should-fix (P3 batch, none blocking individually, most are one-liners)
- Botched doc comments (runtime_api/tests.rs:5451, 5453): the guard-test deletion fused surviving comment lines (
/// written behind its back./// \PUT /v1/sessions`…`) onto the unrelated peek test — and the fused text still promises the removed 409 twice. Sweep the ~8 stale guard-era comments that still describe the refusal as operative (session_manager.rs:205-216, 309-313, 344-355, 295-306; runtime_api/sessions.rs:201-204, 734-738; the guard-era pin docs :5011-5033). SessionMutatoris a dead parameter with lying docs ("External — refused while the session is claimed" is now false): deletion is mechanical (~2 signatures, ~7 in-crate sites, crate-private type). Thelet _ = mutator;transitional shape conserves nothing.- Phantom citation: "(architecture-guard allow-target-cfg)" (engine/tests.rs:26824) names a mechanism that exists only in the parent pinjou3 repo, scans only
pinvou3-app/src-tauri/src, and wouldn't match this format anyway. Delete it. - Share the resolution primitive: make
core::authority::resolve_deepest_existingpub(crate), delete the byte-identical copy at repo_law.rs:337, and have the judged-cwd call it (this also fixes B21-3's comment by construction). The two same-namednormalize_lexical_components(core, clamp-at-root; repo_law, alias tonormalize_path— different relative-path semantics) should move tocrates/pathsper the standing PWC note;crates/pathsis still zero-change in this diff. - Pin the retained consumer: nothing fails red if
is_live_session(id)is deleted from the reclaim keep-chain (session_manager.rs:1835). One test (live claim set →cleanup_old_sessionskeeps the dir) is the keystone for the whole "registry stays" decision. Also restore the round-19 trim-once/400-empty-id record — its only route pins and CHANGELOG line were deleted with the guard tests, so that fix now ships unpinned and unchronicled; and theResourceBusy → 409mapping arm (runtime_api/sessions.rs:1023-1029) is now dead on Unix with a stale rationale comment. - Approval key: the length-prefix covers only the cwd leg; the prefix leg stays raw and forgeable (
shell:git@cwd:9:/tmp/evilcollides across no-operand and redirected spellings;command_prefixadmits@/:verbatim via command_safety.rs:301). Exploitation requires the user to have approved a visibly garbage literal, so P3 — but the body's "prefix/cwd spellings cannot collide" overclaims, and the same file'shash_json_valueidiom would be both simpler and complete. - Disclosure/CHANGELOG tail: ACP
session/newaccepts no roots at all (acp_server.rs:1526-1530 passes&[]) — fail-closed, but absent from the known-scope ledger; three single-root-visible behavior changes lack CHANGELOG entries (non-cached-resume keeps the persisted cwd; bare fork anchors at the parent cwd; cached-resume writeback bumpsupdated_at— the body's "six entries" list citesupdated_atwhile the actual bullet coversarchived_at); the new/cd//forkrefusal messages are hardcoded English in lanes this PR deliberately localized (WorkspaceSwitch* ×15 packs); the 409-removal entry sits under### ChangedwhereRemoved` is conventional.
Four axes (re-derived at this head)
- Value/root cause: real problem, real mechanism, fail-closed and pinned end-to-end on every lane that can produce it (protocol/HTTP/stdio/
/threadhint). Under a user-facing multi-root framing the value verdict stays FAIL — unchanged from round 18: zero interactive entry (census: the only workspace command is/cd; it swaps the primary and can only carry or refuse, never attach; no config key; no CLI flag; the code's own comment says "no multi-root UI"), and the unqualifiedAccessible folders:disclosure still sits over primary-only AGENTS.md discovery and primary-defaultlist_dir/grep_files/file_search. Under an infrastructure/runtime-lane framing it passes. The round-18 ultimatum therefore reduces to a framing decision the author has now explicitly deferred (line 102): either add the/cd-attach arm (~100–200 LOC; every hard part exists and is pinned) plus the one-line disclosure qualifier, or retitle/re-scope the body to infrastructure — but the current hybrid (summary trades on "end to end", deferral hides the producer gap) is not a stable place to merge. - Elegance: concerns, not fail. The chokepoint is genuinely adopted (~20
normalize_workspace_rootscall sites,validate_workspace_rootsgates every row-minting lane, zero reimplementations of the rejections outside core). Against it: mirror-by-comment instead of mirror-by-construction (a third resolution idiom added beside a byte-identical copy of the right primitive), a dead parameter with false docs, a phantom guard citation, a hand-rolled injectivity-partial key beside the module's own hash idiom, and two glued doc comments. - Smuggling: clean-to-minor. All 97 files are in
crates/+docs/+CHANGELOG; zero dependency/manifest churn; all 52 commits on-theme; the remediation commit is 100% B20-scoped. The only off-theme lines are three behavior-neutral clippy test-lane fixes (ce8e926), disclosed verbatim — optionally split, defensibly kept. - Defects: the six blockers above plus the P3 batch; no new P0. History hygiene is clean (52/52 Signed-off-by matching author, conforming subjects, zero file add/delete churn, additive remediation).
The round-20 P0's closure is real and the remaining list is a fraction of last round's. The fastest path to merge: fix B21-1/B21-2/B21-5 (body edits), B21-3+B21-4 (one small commit around the judged-cwd: share resolve_deepest_existing, correct comment/body, disclose or fix the two platform regressions, extend the pin), and decide the scope-framing question (attach arm or infrastructure retitle). B21-6 is one validate_workspace_roots call on the exec resume path or a narrowed sentence.
Round-21 addressed (head
|
qiuYliangM
changed the title
|
Scope decision taken (closing the round-18/20/21 Value-axis ruling): infrastructure framing adopted. Per the reviewer's two options, the maintainer chose the framing path: this PR is retitled to "feat(fork): multi-root workspace_roots authorization infrastructure (runtime lanes) on v0.9.12" and the Summary now states explicitly what is and is not in scope —
Under the infrastructure framing the Value axis passes; the follow-up feature PR restores the end-to-end story when it lands. The parent PR (Hmbown#484) supplies the desktop lane's interactive producer today (its picker/project channels declare root sets through the create commands), and its base-layer section now states the framing and the default-path limitation explicitly. Happy to re-review under the framing — the push is |
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 22 — fresh full re-review of the PR (270b503a4, 54 commits over the r3 closure 61cb769be)
Method. Per the fresh-eyes instruction this pass ignored all 21 prior rounds and re-derived the PR from zero: 13 adversarial domain shards covering every one of the 97 changed files (core intake/lifecycle + protocol; per-turn sandbox materialization + path resolution; execpolicy; SQLite v5; runtime/API lanes; ACP; repo law + command contract; approval/authority; tools layer; session management + engine; TUI/commands/i18n across all 15 packs; a scope-and-honesty audit over the full diff + docs + CHANGELOG + DCO; and a dedicated performance pass) — then I personally re-verified every finding below against 270b503a4 by reading the cited code. cargo check --workspace --all-targets green locally (macOS arm64). No prior-round conclusion was reused; earlier SHAs are cited only where the finding is a diff-vs-base claim.
Verdict: changes requested — 5 must-fix blockers (two new/inherited code bypasses, two false closure statements, one unvalidated judgment lane), 8 should-fixes, and a nit tail. The feature's core is real, the intake chokepoint is genuinely wired on every minting lane I could find, and the pin discipline is strong — see the credit list. But the round-21 closure contains two false statements of exactly the class B21-1 was raised for, one PR-introduced hold bypass, one spelling that silently defeats the PR's headline law layer, and a shipped API lane that judges exec policy on the raw operand.
What holds (verified fresh, with credit)
- Intake chokepoint: sweeping every
ThreadState/row writer, every minting lane validates or fails closed — spawn (core/lib.rs:759, covering every fork viaspawn_thread_with_history), resume replacement + cwd-move arms (core/lib.rs:231-268, the18c50a3f6fix included), runtime create / PATCH-roots / PATCH-workspace-move (runtime_threads.rs:5464/5873/5897),/cd(session_state.rs:771), interactive/fork(session.rs:175), ACPsession/new/session/loadempty guards, exec--resume(see SF22-3 for its premise problem). The..-overshoot clamp is real (PathBuf::popcannot pop past/; verified with an isolated rustc probe) — the fs-root and ancestor checks cannot be defeated by overshoot spellings. - Empty ≡
[cwd]holds by construction at every materialization point (normalize_workspace_rootsprepends cwd;workspace_write_policy, engine seeding,boundary_roots, ACP registry all route through it). - SQLite v5: presence check inside
BEGIN IMMEDIATE, both in-transaction failure paths ROLLBACK and propagate, two-process v4→v5 race genuinely fixed; the tolerant-reader/strict-intake split holds — no tolerant lane can mint a widened row (every write path is either strict-validated or a same-primary round-trip);update_thread_root_setis a correct targeted UPDATE; v5 DB opened by base code is benign (stale, not lost). - Protocol: the absent=
[]/[]=clear/non-empty=replace wire contract is pinned on the wire itself and mutation-sensitive; the read-DTO asymmetry is disclosed. - Execpolicy hygiene: no prefix bug anywhere (all comparisons component-wise); deny > ask > allow precedence untouched; Allow rules never consult the root set (
candidate_idx = vec![0]); the B20-1/B21-3 judged-cwd walk mirrorsresolve_nonexistent_pathstep-for-step on the TUI engine lane, and its two unix-gated pins are mutation-honest; the absolute-path-fallback widening is bounded to exact raw spellings. - Worktree isolation: exactly two clear sites (fresh spawn + resume),
rederive_sandbox_policy_rootsrewrites onlywritable_rootswith flags preserved, no third clone path exists (spawn/resume/fleet-preflight enumerated), the launch manifest fallback is real and durably written; the 256-entry prune evicts only terminal records and breaks otherwise — the hunted extra fail-open does not exist. - Single-root perf is neutral as claimed (syscall-shape traced lane by lane vs
61cb769be); realistic N (≤10) is acceptable everywhere except the multiplicative repo-law/carve-out shape noted in N22-6. - No smuggling: every file classifies as multi-root-required plumbing, a body-disclosed behavior fix, or a gate-driven, behavior-neutral clippy fix (verified per-file, including the 16 locale packs' identical 9 keys and the
RepoLawRulere-export narrowing). DCO verified 54/54 (one matching sign-off each, author/committer split matches the disclosed rebase artifact). - i18n: all 15 shipped packs carry all 9 new keys with exact placeholder parity (scripted check); the placeholder gate self-asserts and has a synthetic-pack mutation pin.
Must fix
B22-1 (P1, new multi-root hold bypass) — repo law normalizes before its symlink leg, so a symlink+.. hop lands inside an attached root whose constitution never fires
repo_law.rs:292 computes candidate = normalize_lexical_components(...) before the symlink-reality leg at :306 (resolve_deepest_existing(&candidate)), so every judged tail is derived from the lexically-collapsed path. Vector: primary /p; attached root /shared with a block-class constitution (e.g. {"paths":["secret"],"action":"block"}); symlink /p/l → /shared/x; existing target /p/l/../secret. The kernel applies .. after symlink expansion, so execution (ToolContext::resolve_path) canonicalizes to /shared/secret, matches the attached root, and admits the write; repo law judges spelling leg /p/secret (under the primary), landing leg /p/secret, resolved leg resolve_deepest_existing("/p/secret") = /p — the attached root's constitution never sees the target. At base 61cb769be this exact call was PathEscape-blocked (primary-only resolve_path), so this is a PR-introduced silent bypass: under Full Access the write lands in a constitution-protected path with no hold; under default Ask the carve-out still opens a modal (its symlink leg keeps the raw spelling — authority.rs:577-598 — and correctly refuses), i.e. a hard hold degrades to a prompt. This is the same link/.. class B20-1/B21-3 removed on the exec lane, one lane over. Fix: also judge resolve_deepest_existing on the pre-normalization raw candidate, mirroring the exec lane's walk; the existing interior-symlink pin has no .. after the hop, so it stays green today.
B22-2 (P1, inherited but headline-defeating) — the filePath camelCase alias is invisible to every plan-time scanner; a filePath-spelled write gets zero law judgment
Execution accepts file_path and filePath via PATH_ALIASES (tools/file.rs:162-163), folded onto path only at execute time (:1496/:2054); the default ToolSpec::prepare passes input through unchanged (spec.rs:1476). Plan-time repo law scans ["path","target","destination","file_path"] (repo_law.rs:137 — no camelCase), and the approval-side consumers are narrower still: auto_review.rs::file_write_target_paths reads only path, and the engine's string_field(input,"path") likewise. So {"tool":"write","filePath":"<protected>","content":...} yields zero repo-law targets → repo_law_plan_decision returns None → no per-root constitution fires, no path-scoped ask/deny matches, and Auto-Review sees an unbounded write. Base had the identical gap, so this is not a diff regression — but this PR's headline feature is per-root constitutions, and the module doc's "Missing any shape the tool honors is a hold bypass, so this deliberately over-collects candidate paths" (repo_law.rs:124-125) is false for this shape. Fix: add filePath to the scan keys (and to the two approval-side collectors), or fold the aliases at prepare time so every plan-time consumer sees canonical keys.
B22-3 (P1, disclosure integrity — the B21-1 class) — B21-4's claimed disclosures do not exist
The body's round-21 disposition says the two canonicalize-at-judgment regressions (Windows \\?\ verbatim spellings never matching scopes; declared-vs-canonical scope spellings silencing persisted scoped allows) "are disclosed in the body and CHANGELOG per the review's minimum". They are disclosed nowhere: the Known-scope section has no bullet on either regression (\\?\/verbatim/scope-spelling appear nowhere in it), the [Unreleased] CHANGELOG has no entry, and the round-21 commits (e5ed6efb1, 270b503a4) touch six code files — neither CHANGELOG nor docs among them. Worse, the disposition bullet itself understates the mechanism: the lexical scope match silences persisted scoped ask and deny rules in both spelling directions (a deny scoped to the canonical spelling never fires for executions judged at the symlink spelling, and vice versa — the macOS /var ↔ /private/var pair included). Deny-silencing is fail-open and undisclosed anywhere. In this PR's own review regime a body bullet still claiming a thing that does not exist was exactly B21-1; same class here.
B22-4 (P1, closure integrity) — SF-4's restore-face boundary clause was never added; the handler is byte-identical to base
Round-20 SF-4 says "POST /v1/snapshots/{id}/restore is the one rollback face whose response carried no boundary clause — the clause is added with the round-21 commit", and round-21 B21-2 asserts "SF-4's restore-face boundary clause added in code with the 'every rollback surface' claim now true". Verified false at 270b503a4: runtime_api.rs:5870-5878 returns {"restored": id} with no clause — byte-identical to base — and RuntimeApiState carries no workspace_roots field, so the face cannot even compute the condition. Grepping the full round-20+21 deltas finds zero additions of ATTACHED_ROOTS_NOT_REVERTED_NOTE/restore_covers_primary_only on that face. A multi-root thread's snapshot restore over HTTP still reports success with no indication that attached-root writes persist, while the merge queue is told it was fixed.
B22-5 (P1, lane security) — the headless /tool + stdio tool-call lane judges exec policy on the raw cwd operand and takes workspace_roots unvalidated
Runtime::invoke_tool builds ExecPolicyContext with cwd from call.execution_subject(&fallback_cwd) (core/lib.rs:1605, tools/lib.rs:420-432) — the raw params.cwd verbatim: no workspace join, no canonicalize(), no B21-3 walk — while the same call's execution resolves the operand roots-aware and canonically (shell.rs:5084 → spec.rs:1088). This lane ships (POST /tool route, app-server/src/lib.rs:756, and the stdio tool-call equivalent), and its own field doc says attached-root ask/deny rules fire when the set is declared. Consequences: (a) a deny scoped to the canonical spelling of an attached root is evaded by a symlink-spelled operand (cwd:"/link" where /link → /real — judgment compares lexical components, execution canonicalizes into /real); (b) any operand containing .. or being relative makes normalize_workspace_scope return None, so all scoped rules — allow and deny — are silently inert for that call. No allow-bypass is possible (the same .. rejection blocks lexical-into-primary spellings), which is why this is P1, not P0. Compounding it: ToolCallRequest.workspace_roots (app-server/src/lib.rs:141-149) is #[serde(default)] and flows raw into ExecPolicyContext with no validation — the only roots intake in the PR with no validator at all. The body presents B20-1 as "the approval side judges that effective cwd — canonically since round 20" without this lane qualification. Fix: resolve the operand through the same roots-aware canonical resolution execution uses (the engine lane already has the exact walk to reuse), and validate or at least normalize the declared set.
Should fix
SF22-1 — a fake .git marker qualifies a root for the modal-free write carve-out, and the model can mint it itself
authority.rs:554 checks only root.join(".git").symlink_metadata().is_err() — an empty file or a dangling symlink named .git qualifies; no pointer validation (contrast the sandbox side, sandbox/policy.rs:404-435, which parses gitdir: and requires canonicalize success). touch/mkdir are WorkspaceSafe, so under the sandbox the model can run touch /shared/.git modal-free, and from then on every write under /shared is carve-out modal-free in the default Ask posture — with none of the git reviewability the carve-out exists to guarantee, and (per the disclosed primary-only SnapshotRepo) no undo evidence. The same predicate gates Auto-Review's autonomy (auto_review.rs:212-221). The body says "attached git root"; that git-ness is unchecked and self-mintable is undisclosed. Fix: require a directory or parse the gitdir: pointer like resolve_gitdir_pointer.
SF22-2 — one approval of any cd-led compound grants every cd-led compound in every attached root
cd is not in COMMAND_ARITY, so every cd-led compound classifies to the single prefix "cd" (command_safety.rs:270-303), and the grouping key is shell:cd@cwd:<len>:<cwd> (approval_cache.rs:96-108). Approving one benign-looking cd deploy && make covers cd /att && <anything non-Dangerous> in any attached root — multi-root widens the family's writable reach from the primary to N roots; neither the ask-rule judgment nor execpolicy re-judges segments after an internal cd. Execution-time Dangerous classification caps the worst case. Base inherited the classification; the multi-root reach is new and undisclosed.
SF22-3 — the B21-6 exec guard validates against a workspace the lane never adopts
exec_agent.rs:372-394 validates the carried set against the CLI --workspace, then :411 sends Op::SyncSession with workspace: saved.metadata.workspace — the engine runs the saved pair re-normalized against itself, so the "re-based … and persisted back on save" the comment describes never happens. Net: a legitimate resume is hard-blocked (session saved at /w with attached /shared, resumed from /shared/sub → loud error although the engine would run at /w exactly as declared); the error text and the "Resuming in {workspace}" warning both misdescribe the behavior; the hint names a --workspace-roots flag that does not exist (no such arg anywhere in ExecArgs); and exec_agent.rs contains zero test functions, so the round-21 "failing loudly" fix is unpinned. "Every lane that mints rows" was already true without this guard (the lane never minted moved rows); the guard as written is both over- and under-scoped.
SF22-4 — the HTTP /thread face answers 500, not the claimed 400, on intake-validation errors
app-server/src/lib.rs:698-715: the Err arm unconditionally returns StatusCode::INTERNAL_SERVER_ERROR with the error text (including absolute paths) in a status field. The body's intake bullet promises "HTTP 400 on HTTP lanes"; stdio's -32603 is disclosed, this HTTP face is not, and a client error is misclassified as a server error on one of the lanes the body counts for "HTTP /thread face delivers it".
SF22-5 — Commands::Exec and Commands::Serve bypass resolve_workspace; the "relative --workspace absolutized at intake" claim does not hold on the headless lanes
tui/src/lib.rs:2314-2316 (Exec) and :2529-2531 (Serve) clone cli.workspace raw. Outcomes, none matching the disclosure: (a) fresh codewhale exec --workspace . collapses to an empty boundary set — every tool resolve_path PathEscapes, a write-crippled session (a third behavior, absent from the single-root-visible list); (b) serve --http persists a relative primary with an empty writable set for default-created threads; (c) serve --mcp passes the empty-only guard and runs fail-closed for existing files but fail-open for nonexistent paths under the process cwd via the canonicalize-failure fallback (spec.rs:1163-1167). The Serve lane is absent from the body's intake-surface list entirely.
SF22-6 — ACP: relative cwd silently accepted into a dead session; session/load is the only resume face that seeds roots with no normalizer; the mode-change registry rebuild is unpinned
(a) session/new rejects only Some("") (acp_server.rs:1509-1517) and session/load only an empty path (:1599); cwd: "." from an editor produces a fully initialized session where every tool call PathEscapes (the chokepoint collapses relative cwd to an empty set) — every other intake lane rejects or absolutizes this class. (b) load_session clones saved.metadata.workspace_roots raw (:1607) — the only resume face with zero normalization at seed (contained today because every boundary consumer normalizes, but it contradicts the "persisted/legacy sets keep the tolerant shape normalizer" invariant, and a legacy row holding a now-forbidden absolute entry re-arms on every load with no re-declare path — the disclosed SF-8, verified). (c) the "mode" arm rebuilds the registry with &session.workspace_roots (:1724-1729); the two production siblings are pinned, this site is not — a &[] mutation passes the whole suite and silently narrows a loaded multi-root session to single-root after any agent↔plan round-trip.
SF22-7 — CLI codewhale sessions fork mints a row from the source set verbatim
tui/src/lib.rs:8428 stamps saved.metadata.workspace_roots with no validate and no normalize (the interactive /fork validates at session.rs:175; the runtime bare fork validates via spawn). A legacy poisoned set (ancestor-of-primary, empty/relative entries) propagates into a freshly minted row — propagation, not widening, but it contradicts both the round-19 audit's "a bare fork … fails loud" and B21-6's "'every lane that mints rows' is true again".
SF22-8 — no root-set size cap: a hostile or buggy declaration converts per-turn work into a permanent stall
Nothing bounds workspace_roots length (protocol, app-server, ACP, PATCH all accept unbounded Vec<PathBuf>). At 100k roots: normalize_workspace_roots' contains dedup (core/lib.rs:108) is ~5×10⁹ comparisons and re-runs on every turn and tool call; boundary_roots() canonicalizes 100k chains per operand call (spec.rs:1078-1086); repo law does O(roots × targets) canonicalize chains plus 100k uncached constitution loads per write call; state stores ~5 MB of roots JSON per row. Only the model-facing line and the UI notice are capped. The deferral is recorded, but the exposure is a UI/manager freeze from one malicious or buggy protocol client — a fixed intake cap (e.g. 64) is cheap and belongs before merge.
Nits / recorded for the post-merge pass
- N22-1 (honesty): CHANGELOG bookkeeping vs body claims — the claimed
### Removedheading does not exist under[Unreleased](the 409-removal entry sits under### Changed); the "trimmed/400-empty session-id record restored to CHANGELOG" is not there; of round-20's "six missing single-root-visible CHANGELOG entries", the cached-resumeupdated_atand the PUT-pairing entries are absent; and thestring_fieldentry's "(upstream v0.9.12 hardening)" attribution is wrong (base is v0.9.12 and carries the trim; the change is round 13 of this PR). - N22-2 (stale comments): the guard-era comment cluster survived round-21's sweep —
session_manager.rs:210-211/304-306/353-355,frame.rs:981-987(still promising the removed typed 409),runtime_api/sessions.rs:203-205; plus a mangled, glued doc comment inruntime_api/tests.rs(~5448) that still promises the retracted PUT 409. - N22-3 (wrong note):
ATTACHED_ROOTS_NOT_REVERTED_NOTE/restore_covers_primary_onlyfires for an attached root nested under the primary (validate_workspace_rootsexplicitly permits those), although the primary-rooted SnapshotRepo does revert such writes — every rollback surface then tells the user the opposite of what happened. - N22-4 (disclosure drift): the Known-scope fail-open bullet still names only
InterruptedOrCompleted; the SF-3 "both policies" correction was recorded in the ledger but never applied to the bullet text (is_terminalincludesInterrupted). - N22-5: turn_meta
Accessible folders:is unescaped (a root containing,/newline injects text into the model-facing line — disclosed, deferred);/cdfailure receipts never reach the durable transcript (the cell unconditionally says "Switched workspace to X"; the typed receipt lives only in a TTL-capped toast); the new/forkrefusal copy is hardcoded English and outside the disclosed/cdlocalization deferral;/clearsilently drops an inherited root set (new session starts single-root, undisclosed); the Degraded receipt's "next autosave re-persists it" promise points at the very actor that is unavailable in that arm. - N22-6 (perf, deferred but quantified): repo law re-derives the full target list per root and re-loads/re-compiles each constitution per call (
repo_law.rs:84-89— rules checked after the syscalls);boundary_roots()canonicalizes every root per operand call with no memoization; per-exec seatbelt materialization canonicalizes each root 2-3×. All fine at N≤10, all hoistable (targets once per call, rules cached with mtime, canonical roots per turn). - N22-7 (misc):
protocol/src/lib.rs:94-96claims "both spawn paths persist at least the cwd" — false for the degenerate-cwd lane (pins an empty persisted set); dedup is byte-exact on raw spellings so/repo/lib/and/repo/libcoexist (disclosed); case-folded spellings dodge the lexical ancestor check on case-insensitive volumes; scope matching never folds POSIX case while path matching folds on macOS (pre-existing; the PR's canonical-at-judgment widens its reach);auto_review.rs::file_write_target_pathsdropped base's.map(str::trim)— a single-root-visible change missing from the disclosed list; the relative-path write-clobber across roots (model writes attached-root content, relative path lands in the primary's same-named file) deserves an explicit write-side framing in the disclosures, not just the read-side one;codewhale sandbox(debug CLI) cannot reproduce multi-root decisions (alwaysVec::new()).
Scope, hygiene, perf verdicts (fresh)
- Smuggling: none found. The closest call is B20-3's guard removal — note for the record that the store-level External-live 409 existed at base (
61cb769be,session_manager.rs:1627/1684), so vs base this PR removes a shipped behavior (external writers can now rename/archive a live-claimed session; last-write-wins). It was review-sanctioned and disclosed, but "retraction" understates it; its acceptance-matrix rewrite and RUNTIME_API.md paragraph are honest. - DCO/commits: 54/54 commits authored by qiuYliangM with exactly one matching sign-off; committer asto18089 on 49 (the disclosed rebase artifact); subjects conform; the body's counts at
faa1190b8are exact. - Perf: single-root neutral vs base as claimed (traced, not benchmarked); N-root scaling linear on most lanes, multiplicative on repo-law/carve-out (N22-6), quadratic on the intake dedup, and unbounded without the SF22-8 cap.
- Docs: RUNTIME_API.md's 404/
-32004, intake rules, and PATCHSome([])semantics verify against code (the v1 lanes); the two claim/code mismatches are B22-4 and the ThreadRecord "only pre-field rows omit the key" note (a relative-primary row serializes with the key omitted today).
Closing
The trunk of this PR — intake validation, the empty-set contract, the v5 migration, the worktree isolation, the pin discipline — survived a from-scratch adversarial pass with real credit. What must change before merge: B22-1 and B22-2 are silent write-to-protected-path bypasses in the law layer this PR's headline is built on; B22-3/B22-4 are false closure statements of the class this PR's own review regime treats as blockers; B22-5 leaves a shipped API lane judging policy on data execution never uses. The should-fixes are small, mechanical, and mostly pin-shaped.
Round-22 addressed (head
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 23 — verification of the round-22 remediation (head 25ffd5d3a)
Method. 15 review shards: nine item-verification shards (one per round-22 blocker/should-fix/nit cluster, each re-deriving the fix from head code with static mutation analysis of every new pin) plus six fresh-eyes shards over the full 3-commit delta (faa1190b8..25ffd5d3a, 17 files, +1,066/−185) — per-hunk adversarial reads of repo_law, core, runtime_api, the approval side, session/snapshot/engine, and a cross-lane caller sweep of every consolidated primitive. I then personally re-verified the three new must-fixes below against head code. Central test run on a clean target dir (no shared CARGO_TARGET_DIR): core 112, repo_law 31, runtime_api 189, session_manager 76, approval_cache 18, judged-cwd pins 2/2, app-server 105, snapshot 224, engine::tests 409, cargo fmt --check clean — matches the commit message exactly.
Verdict: changes requested — but this is the closest round yet. All five round-22 blockers are genuinely closed; every closure claim in the round-22 disposition matches the delta. What remains are three narrow must-fixes introduced or left open by the remediation itself, plus a should-fix tail. The trunk — intake chokepoint, execpolicy judged-cwd, repo law, worktree isolation, SQLite v5 — survived this pass untouched.
What holds (verified, with credit)
- B22-1 CLOSED. The pre-normalization raw-candidate leg exists (
repo_law.rs:319-335), runs for every judged target in every posture, and feeds root attribution. The B22-1 vector hand-traced through head code now yieldsBlockfrom the attached root's constitution (execution canonicalizes through the link to/shared/secret; the raw leg lands the same tail). Pinsymlink_dotdot_hop_into_an_attached_root_is_heldis provably red against270b503a4(old three-legpush_normalizedcannot produce the attached tail; fixture ensures no other leg can) and non-vacuous (assertsBlock+ constitution-only reason text). Adversarial shapes (dangling hop, 2+ link chains,..before the hop, fs-root fallback) all terminate consistently with execution; no existing pin flips. The fix is uncoded-gated so it works on Windows too (only the pin is unix-gated). - B22-2 CLOSED.
filePathin the repo-law scan keys (repo_law.rs:142); both approval-side collectors now route through the sharedpath_param_value(tools/file.rs:252-262), whose alias set derives fromPATH_ALIASESso precedence matches the execute-time fold (divergent cases are ones execution rejects outright — judgment and write cannot disagree). All three pins mutation-honest. Residuals of the same class are listed under SF23-3/SF23-5. - B22-3 CLOSED, this time for real. The Known-scope bullet (body) and the
[Unreleased]CHANGELOG entry both exist and every mechanistic clause checks against code: declared-spelling scopes vs canonical operand judgments, both spelling directions, scoped deny silenced = fail-open, Windows\\?\verbatim degrading exact scoped allows to modals (fail-closed), nothing landed toward the scheduled remediation. The round-21 fake claim is explicitly owned in the disposition. - B22-4 CLOSED (core scenario) — but see B23-2 for a hole. The restore route joins the snapshot's
[sid=...]tag to the thread record and attaches the boundary clause; the route-level pin spawns a real axum server, and the single-root leg simultaneously kills "always attach" and "drop the sid filter" mutants. N22-3's predicate inversion is correct and all five rollback faces share the one fixed helper. The pin lacks the untagged-legacy leg the commit message claims — minor overstatement, noted below. - B22-5 CLOSED.
resolve_operand_cwdis the engine lane's walk moved byte-identically into core (single implementation;tools::spec::normalize_pathnow delegates to the identicalcore::normalize_path_lexically);invoke_tooljudges the resolved cwd and normalizes the declared set. Both pins end-to-end throughRuntime::invoke_tooland statically red against the raw-operand code. Full sweep ofExecPolicyContextproduction sites: no other lane judges on raw operands (ACP/exec_agent go through the engine helpers; the only other constructor is the debugsandbox checkface). - SF22-3 CLOSED (the round-21 guard retired with no residue; I re-verified round-22's premise independently:
Op::SyncSessioncarries the saved pair and the engine re-normalizes it against itself, so the lane never minted a moved row — base-identical net behavior, warning wording now accurate; moved-primary resumes fail loud atvalidate_shell_working_dir). SF22-4 CLOSED (typedIntakeValidationErrordowncast → 400; Display byte-identical so the stdio-32603classifier is untouched; no.context()in the propagation chain). Retention keystone pin real (three legs, hermetic under the same crate env lock, deletion ofis_live_sessionfrom the keep-chain provably flips it). - DCO 55/55 (one matching sign-off each; the three new commits authored and committed by qiuYliangM). Smuggling: clean — every hunk in the 17-file delta maps to a round-20/21/22 item; zero dependency or Cargo.lock changes; the two comment-only sessions.rs hunks are honest round-20 retirement alignment.
Must fix
B23-1 (P1) — restore_covers_primary_only is fail-unsafe for ..-spelled and inward-symlink roots, and its own doc comment asserts the opposite of the code
The new predicate (snapshot/mod.rs:81-85) is skip(1).any(|root| !root.starts_with(workspace)) over the raw persisted spellings. But validate_workspace_roots (core/lib.rs:287-321) judges /ws/../shared on its lexically folded form /shared (not fs-root, not a primary ancestor → accepted) and persists the raw spelling. Consumers (boundary roots, sandbox policy) canonicalize that entry to /shared — outside the primary — so writes there are admitted and not reverted by the primary-rooted restore. The predicate however sees Path::starts_with component-wise true (/ws/../shared.starts_with(/ws) ⇒ true) → judged "nested" → the note is withheld on every rollback face (HTTP restore, patch-undo, /undo, revert_turn, skills restore): the user is told the rollback was complete when attached-root writes persist.
Same for an inward symlink root /ws/link → /elsewhere (lexically nests → no note → writes persist). Worse, the doc comment (snapshot/mod.rs:74-76) claims the opposite: "A root that merely lexically nests through a symlink spelling still fires the note … over-disclosing the boundary is the safe side" — lexically-nested spellings do not fire the note; only the reverse class (outside spelling, nested target) over-discloses. The doc states the code's behavior backwards for exactly the dangerous direction.
This is also a disclosure regression vs the round-22-reviewed state: the old len() > 1 predicate honestly fired for both classes. Fix is one line — normalize both sides with normalize_lexical_components before starts_with (the function's own doc says it is comparison-local and intended for exactly this) — plus a doc correction. Pin should cover the ..-spelled and symlink-inward legs.
B23-2 (P1) — the B22-4 [sid=...] join breaks on the ordinary save/re-key sequence, reopening the exact blocker shape
The clause's only join key is snapshot tag ↔ the thread record's current session_id. But PUT /v1/sessions re-keys the thread to a brand-new session handle (sessions.rs:877-887, set_thread_session_id(&thread_id, &session_handle)), as does POST /v1/sessions/from-thread. Sequence: run a multi-root thread under session S1 → snapshot (tag [sid=S1]) → PUT /v1/sessions (thread now keyed S2) → POST /v1/snapshots/{id}/restore → no thread matches S1 → bare {"restored": id} while attached-root writes persist. That is precisely the B22-4 shape ("multi-root snapshot restore reports success with no indication that attached-root writes persist") through an unremarkable, disclosed-API sequence. The sibling patch-undo face reads the live thread's roots and does not have this hole; the HTTP face is strictly weaker. Fix direction: record the boundary fact (or the root set) in the snapshot itself at capture time — the join through a live, mutable key is the fragile part — or match against session history, and pin the re-key sequence.
B23-3 (P1) — the CHANGELOG's relative---workspace entry is false on the two shipped headless lanes
[Unreleased] states "Relative --workspace values are resolved against the process working directory at startup" (CHANGELOG.md:97). True for lanes through resolve_workspace; false for Commands::Exec (tui/src/lib.rs:2313-2316) and Commands::Serve (:2529-2532), which clone cli.workspace raw — codewhale exec --workspace . still enters boundary checks with a relative primary (the write-crippled collapse SF22-5 describes). SF22-5 is honestly recorded as deferred (body), but an unqualified CHANGELOG entry promising behavior the head does not deliver is the B21-1 class in the CHANGELOG. Fix: qualify the entry ("on lanes that route through resolve_workspace; the headless exec/serve lanes pass the value through — recorded") or land SF22-5.
Should fix
- SF23-1 (P2) — the restore route can now fail after mutating:
repo.restoreruns first, thenlist_threads(...).map_err(...)?(runtime_api.rs:5886-5896) — a thread-store read failure turns a completed rollback into a 500, breaking the "500 = not executed" invariant a retrying client relies on (and adding an O(threads) scan to every restore). Read the threads before mutating, or degrade to log-and-omit-boundary. - SF23-2 (P2) — SF22-8's stall survives on one wire-reachable lane:
POST /tool's declaredworkspace_rootsis only ever normalized (core/lib.rs:1777), never capped — a 100k-entry declaration costs an O(n²) dedup per tool call, client-repeatable.MAX_WORKSPACE_ROOTSguards onlyvalidate_workspace_rootslanes, and the CHANGELOG's "capped at 64 entries at every validating intake" is literally true but dodges the fact that the one lane with no validator at all is where the original stall lives (the core comment admits it). One-line cap at theinvoke_toolentry + honest wording. - SF23-3 (P2) — the apply_patch arm is the leftover of the B22-2 class: execution folds
PATH_ALIASESand honorspathas a touched-files override (apply_patch.rs:393/583-613), while plan-time preflight (auto_review:400-404, engine ask-rulesengine.rs:8319/8335-8339) judges the raw headers — afilePath-spelled override writes a path no approval-side scanner judged (persisted path-scoped deny silenced, carve-out/auto-review judge the header file). Repo law covers it only when the target root has a constitution (it scans both the alias and the headers). Base-inherited, not delta-introduced — buttools/file.rs:245-251's new doc claims the plan-time gates "each must consult this helper" while this arm doesn't, and the doc is the disclosure. Fold before preflight, or disclose precisely. - SF23-4 (P2, recurring B21-5 class, third round) — body records lag the head by one beat again, and once with a false "at this head": line 7 "This push (head
faa1190b8…)" (head is25ffd5d3a); line 67's fork-surface figure claims to be measured "at this head" — actual at head:287 files, +47,041/−13,969, net 33,072 ≈ 22.05×(net stable across diff algorithms), i.e. the 21.46× understates the fork surface in the direction the fork-policy obligation cares about; lines 46/110 carry52 commits / 97 files +8,824/−553vs actual 55 / 98 files +9,846/−694. Thefaa1190b8figures are themselves exact (verified) — this is a one-beat lag, not fabrication, but it has now recurred three rounds running. Refresh in one pass (or adopt "figures pinned to SHA X, deltas per section" as a fixed convention). - SF23-5 (P2, latent) — the headless lane's
permission_path_for_call(core/lib.rs:2267-2284) still reads onlypath, sofilePath-spelled file calls enterExecPolicyContextwithpath: None— path-scoped deny/ask blind on that lane (B22-2's other half, one lane over). Mitigated today only because the production app-server Runtime builds an empty ToolRegistry (dispatch dies atToolNotFoundbefore any write); the moment a real handler registers, this is a silent deny-evasion P1. Sink an alias-aware extractor into core (or fold at intake) before that lane grows tools, and record the dependency explicitly.
Nits / recorded for the post-merge pass
- Dead
ResourceBusy → StatusCode::CONFLICTarm + guard-era comment survived the N22-2 sweep (runtime_api/sessions.rs:1021-1030; its producer was deleted in round-20 — the sweep cleared the cited sites but missed this one). - The 409-removal entry is duplicated, not moved: identical text under
### Changed(CHANGELOG.md:80-84) and the new### Removed(:109-113) — commit and body say "moved". repo_law.rs:142keeps a hand-copied alias key list whilepath_param_value's doc names repo law as a consumer — second source of truth; a futurePATH_ALIASESaddition would silently re-open the repo-law half only. Promote the alias keys to a shared constant.- The two symlink-reality legs (
repo_law.rs:311-335) are eight-line copies differing only in input; foldable into one loop. - Retention pin hygiene: SAFETY comment cites
platform::paths::tests::ENV_LOCK, which does not exist (the actual lock is the shell_dispatcher env lock); thePINVOU3_HOMEset/restore has no reader anywhere in the crate. RUNTIME_API.md:817-821still documents the restore response as bare{"restored": ...}— the new optionalboundaryobject is undocumented (re-introduces a doc/code mismatch on a face round-22 verified aligned).- Body says the only workspace command is "
/cd" — no such command exists (real:/workspace [path|worktrees], alias/cwd); the substantive claim (primary-swap only, no attach arm) is accurate. - The B22-4 route pin lacks the untagged-legacy leg its commit message claims; the engine camelCase pin's name promises carve-out coverage it only gets transitively;
IntakeValidationError's 400 body is a bare string object, inconsistent with the handler's other error shapes; headless fallback leg judges the canonicalized session cwd while the engine lane judges the raw spelling (disclosed class, but the disclosure says "operand-carrying");subagent/worktree.rs:311retains a third, semantically differentnormalize_path_lexically(pre-existing);approval_cache'sshell_cwd_operandcomment still describes the pre-B22-5 judgment.
Scope, hygiene, verdicts
- Value/root-cause: PASS on the recorded framing. The scope decision (2026-09-29, infrastructure substrate; interactive attach arm + default-path tool sweep as the titled follow-up) is now in the body and matches reality — negative claims verified (no attach arm, no config key, ACP
session/newpasses no roots, CLI single--workspace, default-path tools resolve against the primary). Round-18/20/21's value ruling is thereby closed as agreed. - Elegance: light concerns only. The consolidation went the right way (one
resolve_operand_cwd, one lexical normalizer, one alias-aware path extractor); repo law's fix is additive-only inside the module's existing over-collection idiom. Blemishes: the hand-copied alias list, the duplicated legs, and — structurally — B23-2's join-through-a-live-mutable-key design, where a snapshot-side record would be simpler and robust. - Smuggling: clean (per-file mapping above; the CHANGELOG duplication is a bookkeeping blemish, not smuggled content).
- Defects: B23-1/2/3 + SF23-1..5 above. All three must-fixes are narrow and mechanically fixable; none touches the intake/execpolicy/repo-law trunk that rounds 12–22 hammered.
Closing
The round-22 remediation is real: five blockers closed with honest pins, disclosures actually written, the first round in this PR's history where every disposition bullet survived contact with the diff. What stands between this and mergeable is small: make the restore-boundary predicate safe-side and truthful (B23-1), stop deriving the clause from a mutable live key (B23-2), and stop the CHANGELOG promising what the headless lanes don't do (B23-3) — then the should-fix tail, fixed or explicitly recorded as the body does elsewhere. The pin discipline and the fresh-eyes survivability of the trunk are genuinely good work; keep that bar for the follow-up feature PR.
Round-23 addressed (CodeWhale
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Round 24 — fresh full re-review of the PR (5063be0e8, 56 commits over the r3 closure 61cb769be)
Method: a 13-domain parallel deep review (protocol/state, core intake, execpolicy, repo law + command contract, tools layer, subagent isolation, engine/turn-loop, app-server, thread store, runtime API + relay, session layer, ACP + CLI, TUI commands/UI) plus dedicated cross-cutting performance and honesty/smuggling passes, with every reported finding re-verified against the code at head (static reading; the highest-severity items re-verified independently by me, including a globset probe and direct reads of every cited line). Status note: GitHub has this PR as open, not merged, so the round-22/23 closes below are reviewed as merge-blocking remediation.
What holds (verified fresh, with credit)
- Round-22 blocker disposition is real, verified at the diff level. B22-1 closed: the L4 leg judges
resolve_deepest_existingon the pre-normalization raw candidate and the pin is mutation-red; a 10-spelling attack table (symlink+..hop, interior symlink, overshoot, trailing slashes, NUL, symlinked root, APFS case, symlink-out, symlink-out→in) traces to HELD or execution-refused on every leg, so law and gate agree in both directions. B22-3 closed: the declared-vs-canonical and Windows\\?\CHANGELOG entries now exist and match code. B22-4 + B23-2 closed: the restore face genuinely reworked — reads before the mutating restore, tagged snapshots name the boundary conservatively (match → predicate, no-match → conservative, store error → conservative, no 500 after rollback), and the re-key pin is real and non-vacuous. B22-5 closed on the judged-cwd half: the/toollane now resolves the operand throughresolve_operand_cwd, the same primitive the engine lane uses. - The intake chokepoint is real. Every caller-declared mint lane (spawn, resume replacement + cwd-move, fork via spawn, runtime create, both runtime PATCH arms,
/cd, interactive/fork) frontsvalidate_workspace_roots; I swept all 146workspace_roots:construction sites in the repo and found no unvalidated writer from caller input (the exceptions are B24-3 and the tolerant load faces, both reported below). - The v5 migration genuinely improves on the inherited pattern: presence check inside
BEGIN IMMEDIATE, explicit ROLLBACK on both in-transaction failure paths, in-transaction re-check neutralizes theuser_versionrace; idempotency, concurrent-open, and read-only-DB behavior verified. - Empty set ≡
[cwd]holds at every consumer traced (execpolicy prepends cwd and dedups; engine unions cwd-first; spec boundary falls back to the primary). - Worktree-child isolation: spawn and both resume legs pinned end-to-end through the spawn seam (root set AND
writable_roots), and the re-derive helper's output is shape-identical to the engine's own policy build, so no widening is possible; the claim-less fail-open residual matches its disclosure exactly. - Execpolicy allow-narrowing is correct against its contract: allow candidates pinned to
vec![0], ask/deny span all roots with scope+path paired per index, single-root behavior byte-compatible with base, five real pins. - Parity pins are non-vacuous: exact encoded-JSON substring asserts, key-absence asserts, and the three-state
Some([])vsNonedistinction asserted on the wire in both directions. - Hygiene: no undisclosed smuggles. Every hunk in the 98-file diff maps to the roots theme or a disclosed ride-along (three clippy-1.98 fixes, the 16 MiB test stacks, mechanical
Vec::new()field additions) — full-diff sweep plus per-domain classification found no dependency/Cargo/CI/script/URL/telemetry content. 56/56 commits carry exactly one DCO sign-off matching the author; committerasto18089on exactly 49 is the disclosed rebase artifact; commit style uniform. Localization is mechanically clean (9 new MessageIds × 15 packs, placeholder parity, gate extended with a mutation test).
Must fix
B24-1 (P1, closure integrity — the B21-4/B22-3 class) — B23-1's symlink leg is unfixed, its claimed pin does not exist, and the shipped doc comment asserts the opposite of the code
restore_covers_primary_only normalizes both sides with normalize_path_lexically (snapshot/mod.rs:86-98), which cannot see through a symlink: for an inward-symlink root /ws/link -> /elsewhere it returns /ws/link unchanged, starts_with("/ws") still holds, the predicate returns false, and the boundary note stays withheld — while intake admits the spelling (the lexical validator's own documented defeat) and enforcement canonicalizes it, so writes through that root land outside the primary and persist through the primary-rooted restore. Exactly the fail-unsafe the round-23 change claims to close; only the ..-spelling leg is real.
The integrity half is what makes this a blocker under this PR's own regime:
- Commit
5063be0e8claims "Pins: '..'-spelled and symlink legs through the HTTP route" — no symlink pin exists.runtime_api/tests.rs:5255-5283creates only the..-spelled thread; no symlink is created anywhere in that file, while its own comment says "the clause must fire for both". - The shipped doc comment (
snapshot/mod.rs:76-85) says "Normalizing first fires the note for exactly the roots whose consumers see them outside the primary" — false for the symlink spelling; a future maintainer relying on it would stop hunting. - The same commit message says the restore-response boundary object is "documented in RUNTIME_API.md" — the commit does not touch that file and no doc mentions
boundary/attached_roots_not_reverted;docs/RUNTIME_API.md:817-821still documents the response as bare{"restored": "<id>"}.
Fix shape: canonicalize the root (raw fallback, the boundary_roots() idiom) before the containment comparison; add the real symlink pin through the HTTP route; correct the comment and the commit-record claims; document the boundary object. (Mitigating context for triage: the note is advisory and restore behavior is unchanged from base — but the false pin claim and the asserting comment are the exact class rounds 21–23 treated as merge-blocking.)
B24-2 (P2, approval bypass) — apply_patch's alias-spelled top-level path override is invisible to every plan-time approval gate
apply_patch::execute folds PATH_ALIASES and then reads the override (apply_patch.rs:394 → :583), while every plan-time collector runs on the raw input: apply_patch_permission_paths calls preflight_apply_patch with no fold (normalize_apply_patch_input does not fold — verified), feeds ask rules and the Hmbown#5185 carve-out (engine.rs:8327-8339), and auto_review has the same shape (auto_review.rs:401-405). Concretely, under the shipped default Ask posture:
apply_patch {"file_path": ".git/hooks/pre-commit", "patch": "--- a/README.md\n+++ b/README.md\n@@…"}
- plan time sees no
path, judges only the payload header (README.md — allowed), the carve-out qualifies, the modal is skipped; - execution folds the alias, the
pathoverride wins over the payload headers (a mismatch is only a"Note:"string, not an error), and the write lands in.git/hooks/pre-commit— the carve-out's own named exclusion (authority.rs:635-637).
Repo law still scans the alias keys and payload headers, so the constitution layer holds — this is a gap between two of the three gate layers, not a total blind spot. But it re-opens the exact bypass class B22-2 declared closed ("an alias-spelled write was otherwise invisible to every one of them at once"), for one tool. Fix shape: fold aliases inside preflight_apply_patch (or route the override through path_param_value before collection); pin that an alias-spelled override reaches both the ask-rule and carve-out judgments.
B24-3 (P2, intake doctrine) — the /tool face silently truncates, admits //ancestor roots, and leaves the cwd slot unvalidated
core/src/lib.rs:1780-1788 comments itself as "capped like a validating intake" — but a validating intake rejects over-cap declarations (validate_workspace_roots errors at >MAX_WORKSPACE_ROOTS); this lane's take(64) silently truncates, dropping entries 65+ without telling the caller. Beyond the cap there is no validation at all on this face: a caller-declared / or an ancestor of the primary — rejected on every thread lane since round 19 — is admitted into ExecPolicyContext.workspace_roots here, and the cwd slot falls back req.cwd → current_dir() → "." with no empty/relative rejection. This is the only roots/cwd intake in the PR that fronts neither the validator nor an error path, it violates the chokepoint doctrine this PR itself wrote (core/src/lib.rs:250-251: "admit it whole or reject it with an error, never silently reshape it"), and the face has no test at all. Impact is bounded (loopback+auth by default; the served Runtime builds an empty ToolRegistry, so nothing executes after the judgment) — that is why P2, not P1. Fix shape: route through validate_workspace_roots (or at minimum error on truncate + reject //ancestors) and pin the intake behaviors; also drop the needless .cloned().collect::<Vec<_>>() (a slice prefix does the same without the allocation).
B24-4 (P2, unbounded lane) — the 64-root cap is not enforced on any load face; a wide legacy/hand-edited row becomes a per-call stall
MAX_WORKSPACE_ROOTS is enforced only at the validating intakes (reject) and the /tool lane (truncate, B24-3). The loaded set is consumed untruncated by five tolerant faces: the state reader → pure-load resolve_resume_roots (core/src/lib.rs:408), engine init (engine.rs:1847-1849) and Op::SyncSession (:3503-3504), exec --resume (exec_agent.rs:209), and ACP session/load (acp_server.rs:1607). A row with 100k entries (hand-edited, pre-B19-2, or a future buggy writer — exactly the inputs normalize_workspace_roots' own doc says it is "the last line" for) turns every write-tool call into ~5×10⁹ string compares in the O(N²) dedup here (core/src/lib.rs:112) plus N per-root canonicalize/walk cycles in repo law, boundary_roots, sandbox enumeration and execpolicy — a permanent per-call stall, defeating the cap's stated purpose verbatim (core/src/lib.rs:242-247 names these consumers). No shipped writer can mint such a row today — hence P2, not P1. Fix shape: cap inside normalize_workspace_roots itself (keep primary + first MAX-1 absolute entries, consistent with its documented tolerance) — one edit covers every consumer — or truncate-and-warn at the five load faces.
B24-5 (P2, hot path) — repo law recomputes root-independent work per root per write call, and re-reads/re-parses/re-compiles every constitution per root per call
Inside repo_law_plan_decision's root loop: the two resolve_deepest_existing calls (repo_law.rs:323, :340) take candidates that depend only on workspace and the raw target spelling — byte-identical across all N+1 roots — yet are recomputed per root, each an O(depth) canonicalize chain; load_repo_law_rules(&root) (:89) re-walks to the git root, re-walks upward for the constitution, and re-reads + re-parses + re-compiles the glob set per root per call with zero caching; normalize_apply_patch_input(input) (:159) re-parses per root; the key Vecs rebuild per root. At N=64 that is ~190+ realpath syscalls plus 64 constitution read/parse/glob-compile cycles on every write-tool call (base was single-load, zero-fs-call lexical judging). The body's deferred "repo-law per-root perf hoists" names this; now that it is quantified at 4–6k syscalls/call it should land with (or before) merge: hoist the two walks and the patch-input parse above the loop, memoize constitution loads per root (session-lifetime or per-turn — constitutions are static within a turn).
B24-6 (P2, user-facing copy) — the localized busy receipt tells users to run /cd, a command that does not exist in this TUI (all 15 packs)
WorkspaceSwitchBusy — the entire recovery guidance for the blocked lane this PR added — reads "wait, then try /cd again" in all 15 locale packs. The shipped command is /workspace (alias /cwd; commands/groups/core/workspace.rs:9-12, byte-identical at base) and dispatch is exact-match; no /cd is registered anywhere, so the guidance is unactionable in every locale. The PR body repeats the misnomer ("the terminal TUI's only workspace command is /cd"). The placeholder-parity gate cannot catch this class — the strings are wrong, not missing. Fix the copy in all 15 packs and the body.
B24-7 (P2, disclosure accuracy) — CHANGELOG attributes the trim removal to the session-diagnostics classifier; that module is untouched and never trimmed
The entry reads "Session failure diagnostics collect candidate string fields verbatim: the classifier no longer trims…". crates/tui/src/session_diagnostics.rs is not in this PR's 98-file diff, is byte-identical to base, and its collect_string_fields never trimmed. The actual removal was engine.rs::string_field's .map(str::trim) plus the auto_review write-target collector (round-13 commit 305ff9551 states this). Round-22's N22-1 claimed to fix the "string_field attribution" but changed only the parenthetical — a half-done fix of a false record, the same class as B24-1 at smaller scale. Re-point the entry at the real symbols.
B24-8 (P2, doc stronger than the truth) — RUNTIME_API.md's ThreadRecord key-omission note
"only rows persisted before the field existed omit the key entirely" — workspace_roots is #[serde(skip_serializing_if = "Vec::is_empty")], so any empty-set row omits the key today, including a relative/empty-primary row (the validator's degenerate collapse keeps such rows loadable and re-serializable). The adjacent guarded claim (a thread created through POST /v1/threads serializes at least one element) is true. Reword to "rows whose normalized set is empty — which includes every pre-field row — omit the key".
B24-9 (P2, fingerprint) — the fork-surface figure no longer reproduces at the head the body points at
The body claims "at this head (git diff --shortstat dcd4c200f..HEAD) is 287 files, +38,268/−6,077 (net 32,191 ≈ 21.46×)", naming faa1190b8 as the basis. Measured: the figure is exact at faa1190b8, but at the actual head 5063be0e8 the same command yields +47,190/−13,976 (net 33,214 ≈ 22.1×) — Myers alignment thrash in engine/tests.rs (the round-22 test insertion flips the diff alignment; --histogram measures +5,386/−280 for that file). The surface is now understated — the wrong direction for the fork-policy duty, and the parent register's registration figures will drift. Re-measure at the merge head and pin an algorithm (--histogram) or restate the basis.
Should fix / recorded P3s
Performance (bounded, but real hot-path waste): execpolicy matching_ask_rule allocates a candidate_idx Vec per rule, re-normalizes the same rule pattern per root per call, and the owned Vec<PathBuf> context is cloned per deny-scan segment (file lane: once per permission path) — prefilter rules by tool, normalize each distinct pattern once per root, borrow the roots; boundary_roots() canonicalizes every root on every resolve_path and re-normalizes each root up to 3 more times inside the same call — memoize per ToolContext (roots are turn-constant); sandbox get_writable_roots gained a per-root canonicalize on the per-exec-command path (base: clone()) plus a full Vec clone and per-root git-pointer walks — canonicalize once at policy materialization; the restore face lists the snapshot store twice per request and adds a full list_threads scan for tagged snapshots (thread the fetched Snapshot through; short-circuit the join on the in-process registry); /tool's cloned().collect (B24-3); canonical_readonly_roots O(N²) contains (use a set); carve-out re-canonicalizes each root per absolute target (hoist per-root canonical spellings out of the per-target loop).
Disclosure/comment accuracy: the execpolicy workspace_roots field doc says "Allow rules keep matching the primary root only", but under a cwd: redirect the unscoped allow matches at the redirect target (deliberate "judged where it runs", yet the opposite policy from the session-grant re-key — undisclosed); the declared-vs-canonical disclosure says "operand-carrying calls" but the headless lane canonicalizes unconditionally (on a /tmp-spelled macOS session every captured scoped allow is inert lane-wide, not just redirected calls); the body says "the file gates … read the operand's canonical spelling" — the file lane judges the declared workspace + raw path and canonicalizes nothing; POST /v1/sessions/{id}/resume-thread intake rejections answer 500, not the body's "HTTP 400 on HTTP lanes" (map_resume_thread_create_err sends every non-provider reason to internal); runtime_threads.rs:5457-5463 claims the primary must be absolute while the validator collapses a relative primary to an empty set (and the PATCH arm's "keeps the additional roots" comment is false for that input); the repo_law ..-marker comment is false for **-leading globs (fail-closed today, but the stated invariant is load-bearing — say "anchored glob"); state::update_thread_root_set's doc overclaims column ownership on the roots-only leg (it writes the stale cached cwd unconditionally).
Behavior residuals (all fail-closed or narrow): update_thread_root_set ignores rows_affected — a concurrently deleted row makes the cached-resume writeback a silent no-op success; runtime POST /v1/threads/{id}/fork clones the row without validation, so the body's "a bare fork … fails loud" over-generalizes (core fork and TUI /fork do validate); the exec lane can durably mint a relative-workspace row (exec --workspace . --output-format stream-json persists it; exec --resume re-stamps) — the SF22-5 ledger says only "pass the value through"; ..-spelled path operands keep absolute-path deny rules inert while execution canonicalizes (pre-existing at base, unpinned and undisclosed); the documented absolute-path-fallback exception has no pin bounding it; ACP session/new rejects only Some("") — a relative cwd builds a crippled session that fails per-call (disclosed, but the guard comment's own rationale argues against the non-string fallthrough that lands next to it).
Pin gaps: the interactive /cd and /fork <id> validate-refusal legs (no test asserts they fire); manager-level combined PATCH (workspace + roots together, exercising the validates-against-new-primary ordering); the ACP ask-rule/auto-review admission call sites (a &[] regression passes the ACP suite); the mode-switch roots-preserving rebuild (SF22-6 leg 3); session/load applies no MAX cap (wide rows reach the same consumers as B24-4); the new 400 arms (PATCH path-id trim/empty, PUT empty session_id) lost their pins with the 409 removal and were not replaced.
Display/small: a successful /cd that re-based surviving roots emits no WorkspaceRootsNotice while /resume///load disclose the set; the /cd refusal guidance ("re-declare the roots after switching") is unactionable inside the TUI (no producer exists there); WorkspaceSwitchReceipt::Failure's doc says "not durably recorded" but the (save-failed, actor-queued) arm is queued and converges; /branch persists without stamping the live set (harmless — disk round-trip — but the "every remaining writer stamps" claim should enumerate it); stale guard-era docstrings survive in the same PR that retired the guard (session_manager.rs:5035 inside a new pin, and the :217 coexistence sentence contradicting the B20-3 rationale); exec-resume's workspace-mismatch warning is Text-format-gated (silent under JSON output).
Body staleness: no round-23 disposition anywhere in the body; the "This push (head faa1190b8)" header and its counts are four commits stale (56 commits, 98 files, +9,996/−702 at head); B21-6's "every lane that mints rows is validated" stands uncorrected inline although SF22-3 reverted that guard; the body still cites engine.rs::string_field (deleted by round-22 in favor of path_param_value).
Verdicts on the four review axes
- Utility/completeness: the infrastructure is real and mostly root-caused (intake chokepoint, v5 migration, empty≡[cwd], worktree isolation, judged-cwd unification), but not complete: B24-1 is an open false closure, B24-2 re-opens the headline bypass through one tool, B24-3/B24-4 leave the intake/cap doctrine unenforced on two faces.
- Elegance: good structure overall (single predicate, typed receipts, shared
PATH_ALIASES, chokepoints, non-vacuous pins); warts: silent-truncate vs reject inconsistency, two near-identical lexical normalizers with different relative-..semantics, a second textual copy of the alias list inpermission_path_for_call, the/cdnaming confusion propagated into 15 locale packs and the body. - Smuggling: none found — every hunk maps to the theme or a disclosed ride-along; commit hygiene matches its disclosure exactly.
- Correctness/performance: no fail-open found on any shipped lane beyond B24-2; the P1-class issue is closure integrity, not a new bypass; performance is bounded at validated N≤64 on all validating intakes, with the unbounded face (B24-4) and the hot-path multipliers (B24-5 + list above) quantified.
Closing
Rounds 22–23 genuinely closed most of the round-22 blockers — verified at the diff level, with credit. What must change before merge: B24-1 is the same false-closure class this PR's own review regime treats as a blocker — a commit message and a shipped doc comment asserting a fix and a pin that do not exist; B24-2 re-opens the headline bypass through one remaining tool; B24-3 breaks the intake doctrine every other lane follows; B24-4 leaves the cap's stated purpose unenforced on the load side; B24-6 ships wrong recovery guidance in 15 languages. The remainder are small, mechanical, and mostly comment/doc/pin-shaped. The review worktree remains at .worktrees/cw-pr54-fresh (head 5063be0e8) for reproduction.
| /// Normalizing first fires the note for exactly the roots whose consumers | ||
| /// see them outside the primary; over-disclosing the boundary is the safe | ||
| /// side. | ||
| pub fn restore_covers_primary_only( |
There was a problem hiding this comment.
B24-1 (P1, closure integrity). normalize_path_lexically cannot see through a symlink: for an inward-symlink root /ws/link -> /elsewhere it returns /ws/link unchanged, starts_with("/ws") still holds, the predicate returns false, and the boundary note stays withheld — exactly the fail-unsafe this round-23 change claims to close. Intake admits the spelling (the lexical validator's own documented defeat), enforcement canonicalizes it (writes land outside the primary and persist through the primary-rooted restore), so the hole is reachable end to end; only the ..-spelling leg is real.
The commit message further claims "Pins: '..'-spelled and symlink legs through the HTTP route" — no symlink pin exists anywhere in runtime_api/tests.rs (the B23-1 test creates only the ..-spelled thread), and the doc comment above (lines 76-85: "Normalizing first fires the note for exactly the roots whose consumers see them outside the primary") asserts the opposite of the code for the symlink case. A false pin claim plus an asserting comment is the exact class rounds 21-23 treated as merge-blocking (B21-4/B22-3 precedent).
Fix shape: canonicalize the root (raw fallback, the boundary_roots() idiom) before the containment comparison; add the real symlink pin through the HTTP route; correct this comment and the commit-record claims.
| "the re-keyed owner must not drop the boundary clause: {body}" | ||
| ); | ||
|
|
||
| // Round-23 B23-1 legs: a `..`-spelled root and an inward-symlink root |
There was a problem hiding this comment.
This comment says "a ..-spelled root and an inward-symlink root … the clause must fire for both", but the test creates only the ..-spelled thread (ws/../shared2); no symlink is ever created in this file. A claimed pin that does not exist is a false verification record — under this PR's own review regime that is blocker-class. Please add the symlink leg (thread with a symlink-spelled root, restore through the HTTP route, assert the boundary clause fires) or correct the claim here and in the commit record.
| /// `path` alone: a `file_path`- or `filePath`-spelled write was otherwise | ||
| /// invisible to every one of them at once (review #484/CodeWhale round-22 | ||
| /// B22-2). | ||
| pub(crate) fn path_param_value(input: &Value) -> Option<String> { |
There was a problem hiding this comment.
B24-2 (P2, approval bypass). This helper closed B22-2 for the single-path tools, but apply_patch's top-level path override never routes through it: apply_patch::execute folds PATH_ALIASES first and then reads optional_str(input, "path") (apply_patch.rs:394/583), while every plan-time collector runs on the raw input — apply_patch_permission_paths calls preflight_apply_patch with no fold (normalize_apply_patch_input does not fold — verified), file_write_tool_target_paths feeds it to the ask rules and the Hmbown#5185 carve-out (engine.rs:8327-8339), and auto_review has the same shape (auto_review.rs:401-405).
So under the shipped default Ask posture: apply_patch {"file_path": ".git/hooks/pre-commit", "patch": "--- a/README.md…"} — plan time sees no path, judges only the payload header (README.md, allowed), the carve-out qualifies, the modal is skipped; execution folds the alias, the override wins over the payload headers (a mismatch is only a "Note:", not an error), and the write lands in .git/hooks/pre-commit — the carve-out's own named exclusion (authority.rs:635-637). Repo law still scans the alias keys, so the constitution layer holds, but this re-opens the exact bypass class B22-2 declared closed, for one tool.
Fix shape: fold aliases inside preflight_apply_patch (or route the override through path_param_value before collection) + a pin asserting an alias-spelled override reaches both the ask-rule and carve-out judgments.
| cwd, | ||
| workspace_roots | ||
| .iter() | ||
| .take(MAX_WORKSPACE_ROOTS) |
There was a problem hiding this comment.
B24-3 (P2, intake doctrine). The comment says "capped like a validating intake", but a validating intake rejects over-cap declarations (validate_workspace_roots errors at >MAX_WORKSPACE_ROOTS) — this take(64) silently truncates: entries 65+ vanish without telling the caller. Beyond the cap there is no validation at all on this face: a caller-declared / or an ancestor of the primary (rejected on every thread lane since round 19) is admitted into ExecPolicyContext.workspace_roots here, and the cwd slot falls back req.cwd -> current_dir() -> "." with no empty/relative rejection. This is the only roots/cwd intake in the PR fronting neither the validator nor an error path; it violates the chokepoint doctrine this PR itself wrote (core/src/lib.rs:250: "admit it whole or reject it with an error, never silently reshape it"), and the face has no test. Bounded today (loopback+auth, empty ToolRegistry), hence P2 not P1.
Fix shape: route through validate_workspace_roots (or at minimum error on truncate + reject //ancestors) and pin the intake behaviors; also drop the needless .cloned().collect::<Vec<_>>() — a slice prefix does the same without the allocation.
| normalized.push(root.clone()); | ||
| } | ||
| } | ||
| normalized |
There was a problem hiding this comment.
B24-4 (P2, unbounded lane). The O(N^2) contains dedup here is fine at the validated N<=64, but nothing caps the loaded set: all five tolerant load faces (state reader -> pure-load resolve_resume_roots :408, engine init :1847 and Op::SyncSession :3503, exec --resume :209, ACP session/load :1607) consume rows verbatim with no truncation. A row with 100k entries (hand-edited, pre-B19-2, or a future buggy writer — exactly the inputs this function's doc says it is "the last line" for) turns every write-tool call into ~5x10^9 string compares here plus N per-root canonicalize/walk cycles in repo law, boundary_roots, sandbox enumeration and execpolicy — a permanent per-call stall, defeating the cap's stated purpose verbatim (core/src/lib.rs:242-247 names these consumers). No shipped writer can mint such a row today — hence P2, not P1.
Fix shape: cap inside this function (keep primary + first MAX-1 absolute entries, consistent with its documented tolerance) — one edit covers every consumer — or truncate-and-warn at the five load faces.
| - A worktree child session's exec lane no longer inherits the parent | ||
| session's writable roots: the lane is re-derived at spawn and at resume | ||
| from the child's own workspace. | ||
| - Session failure diagnostics collect candidate string fields verbatim: the |
There was a problem hiding this comment.
B24-7 (P2, disclosure accuracy). This entry attributes the trim removal to the session-diagnostics classifier: crates/tui/src/session_diagnostics.rs is not in this PR's 98-file diff, is byte-identical to base, and its collect_string_fields never trimmed. The actual removal was engine.rs::string_field's .map(str::trim) plus the auto_review write-target collector (round-13 commit 305ff95 states this). Round-22's N22-1 claimed to fix the string_field attribution but changed only the parenthetical, leaving the module wrong — a half-done fix of a false record. Re-point the entry at the real symbols.
| primary first; normalization always prepends the workspace, so a thread | ||
| created through `POST /v1/threads` serializes at least one element — a | ||
| single-root thread reads `"workspace_roots": ["<workspace>"]` — and only | ||
| rows persisted before the field existed omit the key entirely), `mode`, |
There was a problem hiding this comment.
B24-8 (P2, doc stronger than the truth). "only rows persisted before the field existed omit the key entirely" — workspace_roots is #[serde(skip_serializing_if = "Vec::is_empty")] (runtime_threads.rs:644-646), so any empty-set row omits the key today, including a relative/empty-primary row: validate_workspace_roots returns Ok(vec![]) for a degenerate primary and the tolerant pure-load branch keeps such rows loadable and re-serializable. The adjacent guarded claim (a thread created through POST /v1/threads serializes at least one element) is true. Reword: "rows whose normalized set is empty — which includes every pre-field row — omit the key".
| pub ask_for_approval: AskForApproval, | ||
| /// The sandbox mode in effect, if any (e.g. `"workspace-write"`). | ||
| pub sandbox_mode: Option<&'a str>, | ||
| /// Additional workspace roots for ask/deny path and scope matching; |
There was a problem hiding this comment.
P3 (doc misstates the allow contract). This field doc says "Allow rules keep matching the primary root only" — true for scoped allows, but at head callers feed the judged effective cwd as ctx.cwd (resolve_operand_cwd at the exec seams), so under a cwd:/working_dir: redirect an unscoped allow (command-only rule, or relative-path rule) auto-approves execution inside an attached root: candidate_idx = vec![0] pins to the redirect target. That is the deliberate "judged where it runs" design, but it is the opposite policy from the session-grant layer, which re-keys on the operand precisely so approve-at-the-workspace does not cover redirected calls — and neither this doc nor the body records the unscoped-allow side. Fix the doc (and consider one disclosure sentence next to the grant re-key item).
| // super-root (`/`, `/..`), an ancestor of the primary, or a | ||
| // non-absolute entry (`~/shared`) each widens — or silently shrinks — | ||
| // the sandbox the caller thinks it declared. The primary slot was | ||
| // already guarded non-empty above; validation also requires it to be |
There was a problem hiding this comment.
P3 (comment asserts the opposite of the validator). "validation also requires it to be absolute" is false: validate_workspace_roots returns Ok(vec![]) for a non-absolute non-empty primary before any rejection (core/src/lib.rs:288-290, pinned as the degenerate collapse). So this lane accepts a relative workspace with a full declared set and silently collapses the set to empty — fail-closed and body-disclosed, but this comment, in the one place a future reader will trust, promises a rejection that does not exist; the PATCH arm's "a workspace-only change keeps the additional roots" comment (:5884) is false for the same input (the re-declaration collapses to []). Fix: correct both comments (or bail on a non-absolute primary here — one is_absolute check).
| // glob, and the ordinary approval/sandbox gates still govern it. | ||
| // an explicit marker so this spelling tail can never match a | ||
| // workspace-relative glob. Where the write actually lands is | ||
| // judged separately below from the clamped execution candidate. |
There was a problem hiding this comment.
P3 (wrong invariant, fail-closed direction today). The claim that this marker "can never match a workspace-relative glob" is false for **-leading globs: probed with globset 0.4, **/secret matches ../secret, so an out-of-tree overshoot spelling judged against a constitution carrying a **/name glob produces the marker tail and DOES match — an extra hold on a write landing outside every root (fail-closed), but the stated invariant is the load-bearing reason this leg exists, and a future maintainer "simplifying" the marker on the strength of this comment would re-open the overshoot bypass. Fix the comment: "never matches an anchored (root-relative-prefix) glob".
History compression (2026-09-30): the 56 reviewed topic commits (the round-8 through round-23 closes) are squashed into this single commit. The tree is byte-identical to the reviewed head 5063be0; the pre-squash line stays publicly reachable on the fork backup branch backup/pre-squash-54-20260930 and in this PR's timeline. The topic: threads carry cwd (primary root) + workspace_roots (the full accessible root set) across protocol, SQLite v5, TUI JSON persistence, per-turn sandbox materialization, cross-root carve-out/resolve_path/execpolicy judging, primary-root-only instructions, and the Accessible folders turn_meta disclosure. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
5063be0 to
4095834
Compare
History compressed: branch squashed to a single commit (2026-09-30)Per the author's decision,
Round-24's findings are untouched by this push and remain the next work item. |
3 participants
Summary
Scope framing (decision recorded 2026-09-29, closing the round-18/20/21 Value-axis ruling): this PR is multi-root sandbox/authorization INFRASTRUCTURE for the runtime lanes — not an end-to-end user-facing feature. Threads carry cwd (primary root) + workspace_roots (full accessible set) across protocol, SQLite v5, TUI JSON persistence, per-turn sandbox materialization, cross-root carve-out/resolve_path/execpolicy, primary-root-only instructions, and
Accessible folders:turn_meta disclosure. Runtime API (POST/PATCH /v1/threads), stdio, and the HTTP/threadface all deliver the set; empty set ≡[cwd]byte-for-byte.What this PR deliberately does not include (the round-18/20/21 producer gap, now the titled follow-up): there is no interactive producer — the terminal TUI's only workspace command is
/cd(primary swap; no attach arm), the CLI takes a single--workspace, there is no config key, and ACPsession/newpasses no roots — and the default-path tools (grep_files/list_dir/file_search) resolve against the primary, so a model that reads theAccessible folders:disclosure must address attached roots by explicit path. Everything else in this Summary is delivered and pinned. The interactive attach arm plus the default-path multi-root tool sweep are the scheduled feature follow-up; until then this PR is the authorization/persistence substrate its title names.This push (head
4095834e, the 2026-09-30 compression of the reviewed topic line — tree byte-identical to the round-23-reviewed5063be0e8; all pre-squash SHAs cited below (faa1190b8,25ffd5d3a,d872c6663, …) resolve on the backup branchbackup/pre-squash-54-20260930)Round-17 remediation (
d872c6663) — three new blockers and both/cdshould-fixes:normalize_workspace_rootsreturns an empty set for an empty/relative cwd (the old[""]passthrough was the same vacuous-containment primitive as the round-15 roots fix; a relative cwd is too, sincenormalize_path(".")is the empty path). Empty cwd/workspace is rejected atcreate_thread, app-server thread/start+resume+fork (resolve_resume_rootsisResultnow), ACPsession/new+session/load, the TUI session-restore funnel, CLIresolve_workspace, and standalonerun_mcp_server. Behavior fix that fell out: relative--workspacevalues (e.g..) are now absolutized at intake — they previously disabled containment silently. Pins on every lane, red without the guards. (Premise correction: clap already rejects--workspace ""at parse; the guard is defense-in-depth and a pin locks the parse contract.)normalize_lexical_componentsnow delegates totools::spec::normalize_path(execution's own normalizer), so an overshoot spelling like/w/x/../../../att/vendor/lib.rsjudges the clamped landing path and is held by the attached root'svendor/**block in every posture; law and gate cannot drift apart again. Two pins, both red under the old strict-collapse bail (got None— the exact bypass).WorkspaceWrite.writable_rootsfrom the cleared set (rederive_sandbox_policy_roots, same idiom asworkspace_write_policy); flags preserved, non-worktree children unchanged. Both resume variants are pinned end-to-end through the spawn seam (red without the rebuild: the parent's attached root stayed writable); the fresh-spawn call site shares the unit-pinned helper but has no harness. Behavior change, disclosed below: the confinement now holds on the sandboxed exec face even single-root./cdpersistence receipt is typed (Degraded→ Warning toast,Failure→ sticky Error — the "persisted, actor unavailable" arm no longer screams failure); the/cdblocked message is localized (WorkspaceSwitchBusy× 15 packs, placeholder-free) and names no request, mirroring/clear.Round-16 remediation (
ad466982a). The round-16 pass reviewed9f5f4d34a, so its three must-fixes were already closed by the round-15 push below — re-verified per item in the round-16 reply. The remainder landed here:shell:<prefix>@cwd:<operand>when the call carriescwd:/working_dir:, so a remembered session grant approved at the workspace no longer covers the same command family redirected into an attached root. No-operand keys are unchanged (base grant behavior byte-identical). Pin red without the re-key (left == right == shell:git status).relative_target_landing_under_an_ancestor_root_is_held): session cwd inside an attached root, a relative target lands inside that root, whosesub/**glob must hold the landing path. Red when the candidate judgment is re-narrowed to absolute spellings.workspace_roots(the real contract: an empty set is omitted; threads created by this build carry at least[cwd]), and the execpolicy Allow-narrowing comment now says exec is judged where it runs (session cwd, or the resolved redirect operand) instead of "exec always runs in the session cwd".Round-15 remediation (
e01e3a7b4+8ba62fd7e, the latter adding three pre-existing clippy-1.98 lint fixes the CI gate would have flagged; behavior-neutral, in lanes this PR does not touch):repo_law_plan_decisioncanonicalizes each root once with raw fallback (theboundary_roots()idiom — a non-resolving root keeps its constitution);push_normalizeddual-strips absolute targets against raw + canonical spellings and re-judges the execution candidate per root unconditionally (the relative-only leading-..re-judge is subsumed; its pin still passes);resolve_deepest_existingstrips the resolved path against the canonical root exactly ascarve_out_target_alloweddoes. A symlink-alias spelling, an interior symlink, or an absolute..spelling can no longer skip an attached root's constitution while execution lands the write. Three pins (vectors A/B/C), all red on the pre-fix legs.metadata.workspacefrom the same snapshot beside the roots and re-normalizes (the/forkidiom), so a save → PATCH move → re-save can no longer persist a stale primary for resume-thread to resurrect as a writable root. Pin red without the stamp (left: W1, right: W2).cwd:/working_dir:operand is resolved exactly as the execution lane joins it and fed asExecPolicyContext.cwd; the full normalized session set keeps ask/deny scope matching spanning every declared root. A scoped allow no longer auto-approves execution redirected into an attached root (base refused the call withPathEscape); a grant scoped to the attached root fires exactly where execution lands. Disclosed narrowing: the round-15 judgment was lexical — a symlinkedlink/..spelling could still normalize back into the primary, closed canonically in round 20 (B20-1). Disclosed narrowing: an exact-scoped allow no longer fires with acwd:operand pointing at a subdirectory of the primary (fail-closed; no-operand path byte-identical). Pin red without the fix (Some(Allow)vsNone).normalize_workspace_rootsdrops empty/relative roots entries: an empty-string root madePath::starts_with("")true for every path, nullingresolve_pathcontainment for reads under approvalAutoin every posture. The state reader warns through its existing channel when a persisted row holds such an entry. Three pins, red without the filter.revert_turn's tool result and tool description,/undo's summary and transcript cell,/restore's message, and the runtime patch-undo face now state that only the primary workspace was reverted when attached roots exist (shared wording:snapshot::ATTACHED_ROOTS_NOT_REVERTED_NOTE). Single-root output is byte-identical; one pin per face, red without the gate.update_thread_root_set(cwd + roots + updated_at) instead of the full preserving upsert (cross-process clobber, pinned); the three resume faces normalize the persisted set at seed; the placeholder-parity gate coversWorkspace*MessageIds (synthetic-pack pin; all 15 packs clean); the/cdfailure receipt promotes as a typed sticky Error in any locale;/cdguards onsession_transition_blocked(aligned with/clear); elevation retry policies carry the session's attached roots; pins for the named test gaps (engine exec-policy glue, auto-review bounded-write context, relative-path execution resolution, turn-loop and ACP repo-law call sites, runtime PATCHSome([])clear/evict leg); RUNTIME_API.md corrections (sessions endpoints listed, PUT named beside PATCH in the 409 paragraph, server-injected primary-slot substitution qualified, ThreadRecord key-omission scope corrected to pre-field rows).0c366b9edand83bf482e3are siblings both parented atbbc90540a, so the compare API's three-dot form renders the merge-base diff (the whole round-12 commit, 30 files); the direct tree diff between the two commits is the state test leg alone (1 file, +7/−3), exactly as the 07:19 correction stated.Default-posture carve-out reach (disclosure)
Under the default Ask posture, a write inside any attached git root is carve-out modal-free:
paths_within_workspace_write_carve_outjudges every root independently, so this PR widens the silent-auto-approve surface from one root to N attached git roots. Ask/deny rules and the sandbox keep their scopes, and Allow-scoped execpolicy rules stay primary-root-only (with the base-inherited absolute-path-fallback exception now documented in AUTHORIZATION_ORDER.md). The normal exec lane'scwd:operand resolves through the roots-aware path, and the approval side judges that effective cwd — canonically since round 20 (B20-1: the earlier lexical join let a symlinkedlink/..spelling normalize back into the primary and fire a primary-scoped allow while execution landed in an attached root). A scoped allow matches only where execution actually runs; a same-process TOCTOU between judgment and execution remains inherent to path-based operands. Remembered session grants re-key on the cwd operand (round 16;cwd: nullspellings included since round 20 B20-2, with the operand length-prefixed in the key), so an approve-for-session at the workspace does not cover the redirected family either. The approval modal itself is still skipped for carve-out-qualifying writes under every attached git root./status, the resume notice, and the turn_meta line disclose the set; the reach is the pinned Hmbown#5185 carve-out behavior extended to N roots.Test record (refreshed per review round; round-20 basis below)
cargo check --workspace --all-targetsgreen;cargo fmt --all --checkclean; the CI-equivalent clippy gate (--workspace --all-targets --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or) clean.RUST_MIN_STACK=16777216, and four pre-existing locale-sensitive tests assert English copy while the app takes the locale fromLANG=zh_CN.utf8— all pass underLC_ALL=C, and the lanes they cover are untouched by this PR. Linux CI remains the green authority.faa1190b8: 52 commits over the r3 closure61cb769be; drift vs the closure: 97 files, +8,824/−553; every commit authored by qiuYliangM with exactly one matching DCO sign-off (committer isasto18089on the 49 rebased commits — a rebase artifact, disclosed).Known scope / disclosures (updated)
Guard retired (round-20 B20-3, retraction): the
PUT/PATCHlive-session409 Conflictpromised here by an earlier revision does not exist — the process-local registry cannot coexist with the runtime HTTP server in any shipped topology, so the conflict could never engage and the guard was removed (round-21 B21-1: this bullet's earlier revision still promised it; caught by review). Same-process writers converge by last-write-wins at the store layer; the registry itself remains for same-process retention pruning.Queued/scheduled tasks run single-root:
NewTaskRequestcarries noworkspace_roots, so a task spawned from a multi-root session executes with no attached roots — fail-closed and silent; inheriting is the natural expectation and needs aNewTaskRequestshape change, so it is scheduled rather than smuggled in.Narrow-window worktree-resume fail-open: if a worktree child's claim is stale-released, its worker record has been pruned (256-entry cap), and it is revived via
InterruptedOrCompleted, the resumed child inherits the caller's root set (claim-less and manifest-less). Low probability, widening direction — disclosed; retaining the isolation fact when pruning worker records is the fix shape, scheduled.Subagent
cwd:swaps are non-isolating: an explicitcwd:subagent call without a worktree keeps the parent's root set (the cwd is an explicit, non-isolating swap); worktree children clear the set — on spawn, and on resume via the claim or the launch manifest — and since round 17 the child's sandboxed exec policy is re-derived from the cleared set as well (previously the cloned parent policy kept the parent's roots writable on the exec lane, silently breaking the promised isolation; this also confines the single-root worktree child's exec lane to the worktree, which is the isolation the clear sites already claimed). The resume leg is pinned end-to-end (root set AND exec policy, claim-present and claim-less); the fresh-spawn call site has no harness — its clear+rebuild shares the unit-pinned helper but the call site itself is unexercised. Disclosed here; the code comment cites this section.Attached-root writes have no snapshot/undo evidence:
pre_tool_snapshotopens the SnapshotRepo on the primary root only; carve-out-approved writes under a git attached root leave nothing app-level to restore and/undowill not list them. Since round 15 every rollback surface (revert_turn,/undo,/restore, runtime patch-undo) names that boundary instead of claiming full rollback; base's trusted-external/yolo lanes were equally unsnapshotted — not extended rather than cut.Cross-process archive-flag flap (narrow): the conditional upsert's stamp arm is payload-blind, but a stale in-process cache that still believes a row is archived can re-set the archived flag (never the stamp) under a cross-process unarchive race — a cross-process status-flap cost, self-healing on the next archive/unarchive. Since round 15 the cached-resume override writeback no longer routes through this upsert (targeted cwd/roots/updated_at UPDATE), so the flap window no longer covers roots/cwd.
v0/v4 migration blocks keep the base shape: their presence checks still run outside their transactions (inherited, not this topic's columns); the v5 block this topic adds checks inside
BEGIN IMMEDIATEand rolls back on any in-transaction failure.import_foreignsilently drops roots carried in export files;/resume <export>gives no hint the set did not survive — follow-up.Root-set intake validates fail-closed and is not canonicalized: the round-19 close rejects caller-declared entries that would silently reshape or widen the declared set — a non-absolute root (
~/sharedis refused with an error string — HTTP 400 on HTTP lanes, a JSON-RPCinternalerror on stdio — not silently dropped), a root normalizing to the filesystem root (/,/..), and a root that is a proper ancestor of the primary — on thread create/resume/fork and both runtime PATCH lanes (persisted/legacy sets keep the tolerant shape normalizer, so old rows stay loadable). An empty/relative primary still collapses to the empty set (round 17), so a poisoned record fails closed. A non-existent root is accepted and never matches execpolicy, keeping its constitution fail-closed. Canonicalize-at-intake + a canonicalize-failure warn remain the scheduled follow-up.Canonical-judgment scope spelling (disclosed; remediation scheduled post-merge): exec policy — and the file gates that share its judgment — now read the operand's canonical spelling, while captured ("always allow") and persisted rule scopes hold their declared spelling. On any system where the two differ — macOS
/tmp↔/private/tmp,/var↔/private/var, or a root reached through a symlink — a scoped rule spelled the other way silently stops firing for operand-carrying calls, in both spelling directions. That includes scoped deny rules: a silenced deny is fail-open (the denied command runs). Windows is harder hit and fails closed:std::fs::canonicalizereturns\\?\-prefixed verbatim paths that scope matching cannot produce, so at this head everycwd:-carrying operand degrades every exact scoped allow to the ordinary approval modal on Windows. Scheduled: capture canonical scope spellings at grant time, and trim Windows verbatim prefixes at judgment."Always allow" cannot save for attached-root writes (fail-closed annoyance): the approval overlay builds the ApprovalRequest against the primary, so an attached-root absolute path yields no workspace-relative form and no ask-rule set — and one attached-root path in a multi-file patch discards the rule set for the whole patch. Every attached-root write re-prompts under Ask. The fix is a rule-shape decision, scheduled.
Bridge hint-change PATCH fails the next bridged turn: a
RuntimeThreadHintroots change against an active runtime thread errors the whole next bridged turn (opaque error) instead of queueing until the turn settles. Fail-closed is the right direction; queueing is the scheduled follow-up.No interactive producer for multi-root: roots-bearing threads are created by the runtime surfaces only — HTTP
POST /v1/threadsand the stdiothread/startequivalent (exec/lane/fleet have no flags; the TUI swaps the primary only and there is no config key). The body's "roots enter only via Runtime API/headless" is phrased precisely.Primary-root-only consumers under multi-root (fail-closed, recorded from the round-14/15 consumer hunts):
image_analyzeresolves against the primary only and rejects absolute paths (a shipped tool that cannot see attached-root images while turn_meta advertises them);read_lintsandproject_mapare primary-rooted; the default-rooted search/read tools (grep_files/list_dirdefaultpath=".",file_searchdefaults to the primary, relative paths join the primary — the first gap a user hits in an attached repo); memory/knowledge are keyed to the primary (native_memory.rs,remember.rs) —NativeMemoryStore::workspace_idis the SHA of the primary's git origin, so memories about an attached root land under the wrong key (scheduled follow-up, with project config); skill discovery, slash-command discovery (.codewhale/commands), and the whole LSP subsystem (the pool is rooted at the primary) are primary-only; project config reads only the primary's.codewhale/config.toml, so an attached root's approval-tightening config is silently ignored (scheduled follow-up, with memory keying); review/verify diff evidence resolves against the primary (resolve_diff_target— an attached-root change under review presents the primary's diff as evidence); project-MCP cwd confinement, hooks execution cwd, the PTY start dir,test_runner,js_execution, the verifier and diagnostics lanes, and the git-coupled family (git_status/diff/log/show/blame, review, verify, run_verifiers, run_tests, task gate records) all judge the primary; the cwd-derived PTY/sandbox materialization's writable set diverges from the roots-aware turn policy; the workflowsource_pathgate, subagentresident_file, and subagentcwd:validation are primary-only;@-mention/completion resolution never traverses attached roots. ACP sessions enforce the full set but never disclose it (no turn_meta line;session_configurationomits it) — the "turn_meta discloses the set" promise does not hold on the ACP face for loaded multi-root sessions. All fail-closed or display-only; widening each is a separate decision, tracked for the post-merge pass.Single-root-visible behavior changes (recorded): non-cached resume keeps the persisted cwd where base overwrote it with the process cwd; a bare fork anchors at the parent's cwd; the archive-stamp repair riding the roots persist rewrite means a cached resume of an archived thread no longer nulls
archived_at;engine.rs::string_fieldno longer trims its extracted value (round 13) — load-bearing for the raw-spelling alignment, acwd: " /repo "operand now fails resolution instead of being silently trimmed; an exact-scoped allow rule no longer fires when an execcwd:operand points at a subdirectory of the primary (round 15, fail-closed); the cached-resume cwd override now persists the row and bumpsupdated_at(base did neither); a missing-thread resume/fork answers HTTP 404 (was 200 +status:"missing") and stdio-32004(was success);/cdpersists synchronously and composes persistence receipts into the closing status; a claim-less worktree-child resume flipsisolated_worktreefalse→true via the launch-manifest fallback; thestring_fieldtrim removal also narrows whitespace-padded relative deny-rule matching (mitigated: the actual landing path is the padded junk name in both versions); and the session-grant grouping key re-keys on the exec cwd operand (round 16 — a grant approved at the workspace no longer covers a redirected call; no-operand grants unchanged). Round 17 adds: a worktree child's sandboxed exec lane is confined to the worktree even single-root (the confinement the clear sites always claimed); relative CLI--workspacevalues are absolutized instead of silently disabling containment; the intake chokepoint returns an empty set (fail closed) for an empty/relative cwd where it previously produced[""]; and the/cdblocked message moved from a hardcoded English string to the localizedWorkspaceSwitchBusy, with the degraded-success receipt downgraded from sticky Error to Warning. All in the right direction; listed because they are observable outside multi-root sessions.N3 bundle remainder, listed: envelope-fork hint test (the both-faces hint test drives envelope-resume only), repo-law hold reasons not naming the owning root's constitution, exec saving unnormalized sets, no manager/HTTP-level empty-set or combined PATCH tests, deleted root directories still disclosed as accessible after resume, the conservative carve-out modal gaps (nested attached git root under a relative spelling;
..-spelled target into an attached root always modals; a symlink from attached git root A into attached git root B modals), allow-rule asymmetry under multi-root (relative-path allows only match the primary), and the notice cap-branch pin (display-only; the arithmetic is saturating since round 14). All fail-closed or display-only; tracked for the post-merge pass.Fork-policy: with this theme the fork surface measured against upstream
dcd4c200fat this head (git diff --shortstat dcd4c200f..HEAD) is 287 files, +47,190/−13,976 (net 33,214 ≈ 22.14× the 1,500-line soft limit; the net is diff-algorithm-independent — Myers +47,190/−13,976,--histogram+36,041/−2,827; measured at the compressed head4095834e, tree-identical to the round-23-reviewed5063be0e8, so this is the round-24 measurement basis too — the round-21 figure 32,191 ≈ 21.46× predated the round-22/23 content, the round-19 figure of 16.1× predated the rebase and understated the surface; the round-14 head's figure was 240 files, +25,538/−3,522, net 22,016 ≈ 14.7×). The retention reason and reduction order land with the parent re-pin PR (v0.8.9 macOS: native workbench integration contract for DeepSeek-TUI engine Hmbown/Codewhale#484) — the parent register has no workspace-roots entry yet, andfork-guard.shstill pins the r2 closure, so the parent-side batch is part of the merge sequence; retirement condition: delete when upstream absorbs (upstream openai/codex ships a parallel experimental mechanism, so absorption is bounded adaptation).Parent-repo sequencing
Convergent order per the round-11 review: #54 squash-merges first → parent PR Pinvou/pinvou-agent#484 rewrites its registration against the squash SHA (recomputed figures) → re-pin Hmbown#549. (Hmbown#553 is resolved: it merged 2026-09-22 with its gitlink at the r2 tip, so the feared rewind cannot happen.) Until the squash SHA exists, Hmbown#484's gitlink keeps pinning this branch's head (the re-pin commit riding Hmbown#484 advances it to this branch's live head (
faa1190b8at the round-21 re-review; since advanced by the round-21 close), with EXPECTED_COMMITS and the behavior count recomputed against that head — the parent register currently reads gitlinkfaa1190b8,EXPECTED_COMMITS101, drift 97 files +8,824/−553);verify-public-submodulestays the disclosed transition red in the meantime.No-Issue: foundation port of the pinned single-entry workspace design; the parent-repo registration rides with the stacked pinvou-agent PR.
Signed-off-by: qiuYliangM 185303122+qiuYliangM@users.noreply.github.com
Round-19 disposition (head
841190215)One commit closes all four findings; the round-18 blocker B18-1 was verified closed by the reviewer and the merge-state note (cleanly mergeable with
pinvou3-clean@61cb769be) is satisfied by the rebase that preceded it.is_live_sessiontrims the query and fails closed on a poisoned lock (ownership unknown = live = conflict),set_live_sessionrecovers a poisoned write lock instead of dropping the claim, and the API boundary normalizes once: thePUT /v1/sessionsbody id and thePATCH /v1/sessions/{id}path id are trimmed with an explicit-empty rejection (400). A padded id can no longer read as a stranger to the guard and as the owner to the store on either lane. Pins: a padded-id PUT and a padded-path PATCH against the live session both answer 409, plus registry-level trim and poisoned-lock tests.codewhale-core::validate_workspace_rootsgives intake an explicit fail-closed decision — a non-absolute root, a root normalizing to the filesystem root (/,/..), and a proper ancestor of the primary are rejected (error strings, not typed codes) instead of silently widening the per-turn sandbox; a root under the primary stays fine, a root equal to the primary dedups, and an explicit empty set still clears. Threaded through thread create, resume (explicit replacement sets), fork, and the runtime create/PATCH lanes. The reviewer's alternative (warn + document) was rejected in favor of the fail-closed posture this topic uses everywhere else.[Unreleased]entries for the missing-thread resume/fork 404 /-32004, the live-session 409, intake validation, relative--workspaceabsolutization, worktree exec-lane confinement, and thestring_fieldtrim removal; a RUNTIME_API.md paragraph documents the 404 and the intake rules.~disclosure): superseded by B19-2 — intake refuses a non-absolute spelling with an error string instead of silently shrinking the declared set; the known-scope bullet above is rewritten to the implemented behavior.Local record (Linux arm64; TUI suites on a 16 MiB test stack per the in-repo convention): runtime_api 192 (was 188), session_manager 75 (was 73), core 106 (was 102), workspace_roots 29, repo_law 29 — all green;
cargo fmt --checkclean.Independent post-fix audit (head
18c50a3f6)A fresh-eyes subagent re-verified all four round-19 findings against the code — closure confirmed, mutation checks hold (deleting the guard trim reds the registry-level pin; deleting the ancestor check reds the core and HTTP intake pins), and the measured suite counts matched the claims digit-for-digit. Its findings, acted on or disclosed:
cwdwithoutworkspace_roots) re-anchored the persisted additional roots with the tolerant normalizer, so a persisted entry that becomes an ancestor of (or a super-root for) the new primary was durably minted into the row — the same widening class B19-2 refuses, one lane over from the PATCH workspace-only move that already rejects, and a widened row would additionally strand a later bare fork.18c50a3f6validates the re-based set with the same intake rules; the pure-load branch keeps the tolerant normalizer (legacy rows stay loadable); pin covers the rejection and that the persisted row is untouched by the failed attempt; core suite 106.workspace: "/") is still accepted — the round closed the roots slot, not the primary slot (pre-existing); legacy rows carrying now-forbidden entries load but a bare fork or PATCH-workspace move fails loud until the set is re-declared (remediable, in slight tension with the "loadable" framing — loadable holds literally); stdio intake validation errors map toJsonRpcError::internalrather than a typedinvalid_params;DELETE /v1/sessions/{id}has no live-session guard at all (pre-existing, same bypass class as B19-1 but nothing is bypassed — a candidate for the same 409 treatment); the validate-level duplicate-of-primary acceptance has only indirect coverage via the normalize dedup pin.Deferred from the round-19 non-blocking tail, recorded for post-merge: the repo-law per-root perf hoists, the intake polish cluster (relative-
workspace200-OK, explicit-nulldeserialization asymmetry, root-set size cap, trailing-slash dedup spellings, meta-line escaping), the Windows POSIX-literal test debt, the same-crate helper copy/pub(crate)and execpolicy chokepoint elegance follow-ups, and the review-round citations: measured at round-21 as 37 lines / ~47 tokens across file contents (comments and RUNTIME_API.md prose), so a squash does not remove them — decision recorded: they stay until the post-merge doc sweep, explicitly.Round-20 disposition (head = this push)
cwd:/working_diroperand the way execution resolves it (canonicalize()on the joined path, lexical fallback for nonexistent operands matchingresolve_nonexistent_path) — thelink/..symlink spelling can no longer normalize back into the primary and fire a primary-scoped allow while execution lands in an attached root. Pins: a unix-gated fixture with a real symlink covers symlink+..(not-allow) and symlink-interior (not-allow) with the canonical-spelling in-root control. The engine comment and the body's safety sentence are corrected; the same-process TOCTOU is disclosed as inherent to path-based operands.shell_cwd_operandskipsnull/non-string values like the check and execution sides (cwd: null+working_dirnow keys as the redirected operand), and the grouping key length-prefixes the operand so prefix/cwd spellings cannot collide. Pin: the null spelling keys with the redirected family and not the no-operand family. Stored grants re-key (CHANGELOG entry added).internal), the producer claim corrected (stdiothread/startincluded), the fork figure refreshed to ≈21.5× (the round-19 16.1× predated the rebase and understated the surface — wrong direction for the fork-policy duty, agreed), the Test record refreshed, and the six missing single-root-visible CHANGELOG entries added (approval re-key, PUT workspace-overwrite pairing retraction-adjacent,/cdreceipt severity + guard widening, cached-resumeupdated_at,isolated_worktreeflip,string_fieldnarrowing)./cdvalidates the re-based set throughvalidate_workspace_rootsbefore the swap (refusing the move with guidance) and/fork <id>validates before persisting — the rule now holds on the interactive lanes; round-21 B21-6 narrows the claim: the headlessexec --resumelane still re-anchors a moved primary tolerantly and persists the re-based pair (see the round-21 section), so "every lane" was false until that lane is validated.InterruptedOrCompleted, butis_terminalalso includesInterrupted— both policies are equally exposed; the correct disclosure is "the policies both end-to-end pins drive (InterruptedOnlyandInterruptedOrCompleted)".POST /v1/snapshots/{id}/restoreis the one rollback face whose response carried no boundary clause — the clause is added with the round-21 commit; the runtime patch-undo note remains unpinned (disclosed).session/loadof a legacy poisoned row has no re-declare path — the "remediable" framing does not hold on that lane (8); the archived_at dead-defensive cluster (~305 lines, unreachable preserve arm) split or reduced (9).Scope decision taken (2026-09-29, closing the round-18/20/21 ruling): infrastructure framing adopted — this PR is the runtime-lane authorization/persistence substrate; the interactive attach arm plus the default-path multi-root tool sweep are the titled follow-up feature PR. Figures the parent re-pin carries:
97 files, +8,824/−553over the r3 closure (fork surface ≈21.46× atfaa1190b8).Local record (Linux arm64, 16 MiB test stack): core 106, runtime_api 188 (the four dead-guard route pins removed with the guard), session_manager 75 (registry pins retained), exec judged-cwd pins 1/1, approval grouping pins green, workspace_roots 29, repo_law 29;
cargo fmt --checkclean; twosession_statelocale-dependent tests fail on this machine at the review head identically (pre-existing, EN-locale expectation vs this machine's locale — not a round-20 regression, disclosed).Round-21 disposition (head
270b503a4, two commits on the reviewedfaa1190b8)session/loadlegacy-row re-declare unreachable) are now recorded; SF-3's disclosure corrected to both policies (is_terminalincludesInterrupted); SF-4's restore-face boundary clause added in code with the "every rollback surface" claim now true; SF-9 recorded including the CHANGELOG parenthetical...included), canonicalizes it through any symlink, re-appends the tail and normalizes, mirroringresolve_nonexistent_path(the first cut usedresolve_deepest_existing, whoseOptionreturns None on trailing-..operands — the pin caught it and the walk replaced it). Pin extended with the absent-interior leg (link/<absent>/../..→ not-allow). Comment and body sentence corrected.\\?\verbatim spellings never matching scopes; declared-vs-canonical scope spellings silencing persisted scoped allows) are disclosed in the body and CHANGELOG per the review's minimum — neither is fixed blind from a Linux-only checkout (the Windows trim needs a cfg-gated change that cannot be tested here; the macOS capture-spelling change is a product decision).faa1190b8(287 files, +38,268/−6,077 ≈ 21.46×; 97 files, +8,824/−553 over the r3 closure), the six dead pre-rebase SHAs marked as no longer fetchable, author/committer reconciled, the side-by-side suite counts unified, and the review-round citation decision recorded explicitly (they stay until the post-merge doc sweep — measured 37 lines across file contents, which squash does not touch).exec --resumemoved-primary lane now validates the re-based carried set with the same intake rules (failing loudly with a re-declare hint) — "every lane that mints rows" is true again.resolve_deepest_existingshared (repo_law's byte-identical copy deleted), the retention keystone pin added (a live-claimed orphan dir survives the reclaim and is reclaimed on release — the consumer the B20-3 registry-retention decision stands on is no longer unpinned), the trimmed/400-empty session-id record restored to CHANGELOG, the grouping-key overclaim requalified (the prefix leg stays raw; collision requires a user-approved garbage literal), the ACPsession/newno-roots ledger line added, the 409-removal entry moved under a### Removedheading, and three missing single-root-visible CHANGELOG entries added. Deferred:SessionMutatordead-parameter removal and the localized/cdrefusal copy (rides the same locale surface as the pending attach-arm decision).Local record (Linux arm64, 16 MiB test stack): repo_law 29, exec judged-cwd pins 2/2, approval_cache 18, runtime_api 188, session_manager 76 (+the keystone pin), core 106, fmt clean; the full tui lib on this machine carries the pre-existing locale-env failure band (61–65, sets shuffle between runs; 62 at the review head) — verified against a stash-baseline diff rather than absolute counts, and the three head-only names in the last comparison were investigated (one fixed by the acceptance rewrite, two pass solo).
Round-22 disposition (head
25ffd5d3a, one commit on the reviewed270b503a4)resolve_deepest_existing— the symlink+..hop (/p/l/../secretthrough/p/l → /shared/x) now gets the attached root's constitution applied. Pin: the exact review vector, unix-gated; mutation red on the disabled leg.filePathjoins the plan-time scan keys, and one shared helper (tools::file::path_param_value) feeds both approval-side collectors — pins cover repo-law hold, ask-rule/carve-out judgment, and auto-review reach; mutation red on key removal.{"restored": id}— "every rollback surface names that boundary" is true including this face. Pin: multi-root carries the clause, single-root stays byte-identical.resolve_operand_cwd) used by BOTH the engine lane andRuntime::invoke_tool; the headless lane also normalizes its declared set. Pins: symlink-spelled and relative/..operands now judge like execution (both mutation-red); nuance recorded — execpolicy ask/deny rules span the declared root set, so review vector (a) required the denied scope to sit outside the declared set for the judged-cwd leg to be load-bearing, which is how the pins are scoped.MAX_WORKSPACE_ROOTS = 64intake cap with a pin. SF22-4: the HTTP/threadface answers 400 on intake-validation errors (typedIntakeValidationError; stdio-32603unchanged) with a pin. N22-1/2/3: CHANGELOG corrections (### Removed heading, trim/400 entry, cached-resume updated_at + PUT-pairing entries, string_field attribution), the guard-era comment sweep, and the boundary note corrected to not fire for attached roots nested under the primary (they ARE reverted).Deferred (recorded, not improvised): SF22-1 (.git marker validation), SF22-2 (cd-led compound grouping), SF22-5 (Exec/Serve resolve_workspace bypass), SF22-6 (ACP relative-cwd/load normalizer/mode-rebuild pin), SF22-7 (CLI fork validation), N22-4/5/6/7.
Local record (Linux arm64, 16 MiB test stack): core 112 (+6), runtime_api 189 (+1), repo_law 31 (+2), judged-cwd pins 2/2, app-server 105 (+1), fmt clean. The reviewer's baseline counts reproduce.