Repository navigation
feat(fork): multi-root workspace_roots authorization infrastructure (runtime lanes) on v0.9.12 - #54
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
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
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
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
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
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
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
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
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
Round-24 B24-6/B24-7/B24-8: the localized busy receipt in all 15
language packs told users to retry /cd, a command that does not
exist (the TUI command is /workspace, alias /cwd); CHANGELOG
attributed the deny-rule trim removal to session_diagnostics, a
module this branch never touches, instead of the plan-time operand
collector (path_param_value) its consumers actually read; and
RUNTIME_API.md overstated the ThreadRecord key-omission condition
(serde skips the key for every empty normalized set, not only
pre-field rows) and documented the snapshot restore response as
bare {"restored": id} without the conditional boundary object.
The stale /cd line in the CHANGELOG behavior list is corrected by
the same commit. Rust doc comments keep the /cd shorthand this
branch's review history uses for the workspace-switch lane.
Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
Accuracy group: the execpolicy workspace_roots field doc now states the judged-cwd redirect caveat instead of the false primary-only claim; the headless lane canonicalizes ONLY operand-carrying calls (a no-operand call judges the declared spelling, matching the engine lane — pin: a declared-spelling scoped deny fires for a no-operand symlinked session); the resume-thread lane answers 400 for IntakeValidationError like the /thread and /tool lanes (pin); the create/PATCH primary-absoluteness comments and the PATCH keeps-roots comment tell the collapse truth; the repo_law .. marker says anchored glob; update_thread_root_set's doc drops the column-ownership overclaim and its vanished-row writeback now fails loud (pin). Behavior residuals: the runtime fork validates the cloned set at intake (a poisoned source row is rejected, not duplicated); ACP session/new rejects a relative cwd like the empty string; the exec persist absolutizes a legacy relative workspace row instead of re-stamping it; the /cd refusal copy names actionable exits (no TUI root producer exists); a re-based /cd emits the WorkspaceRootsNotice like /resume; the exec-resume mismatch warning is no longer Text-gated (stderr never touches the JSON stream); /branch documents its deliberate non-stamp; the guard-era registry docstrings stop describing the retired conflict guard. Pins: manager-level combined PATCH (workspace + roots validate against the NEW primary, rejection moves nothing), the PUT empty-session_id and session-PATCH invalid-id 400 arms, and the absolute-path fallback outside every root. Also carries (round-40 M2 names them here — the perf commit's earlier reword pointed at a pre-squash SHA that does not resolve): the execpolicy matching_ask_rule static-candidate-slice hoist (a borrowed static candidate slice for the dominant shapes instead of a Vec per rule per call) and the typed_ask_absolute_path_rule_matches_a_call_outside_every_root pin. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
Round-24 P3 performance group: ToolContext::boundary_roots memoizes the normalized set plus canonical spellings per context lifetime (roots are lifetime-immutable; the builder installs a fresh cache) instead of canonicalizing every root on every resolve_path; the carve-out stats .git and canonicalizes each root once per judgment instead of once per absolute target; canonical_readonly_roots dedups through a set (was O(N^2) Vec contains); get_writable_roots drops the full root-vec clone on the git-pointer walk; and the restore face threads the fetched snapshot through instead of listing the store twice per request. The matching_ask_rule static-candidate-slice hoist named in this message originally landed here but actually ships in the round-24 P3 residue wave commit that immediately follows on this branch (locate it with `git log -S 'static candidate slice'`; round-37 M37-5 pointed at a pre-squash SHA that does not resolve — round-40 M2 corrected the pointer); no judgment semantics change, and the existing suites pass unchanged. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
B26-1 (isolation regression, introduced by the round-25 memo): the boundary memo's no-in-place-mutation invariant was false at three production sites. ToolContext::rebase_roots now owns the (workspace, roots) pair swap AND the memo Arc swap together; the worktree spawn and resume sites and the per-turn live context go through it, and the field invariant names the setter as the only mutation path. Pin: rebased_roots_drop_the_parent_filled_boundary_memo fills the PARENT memo first (the falsely-covered shape) and proves the rebased child denies the parent workspace and its attached root while resolving inside the worktree. M26-1: fork_at_user_message (the live /undo, /patch-undo, /retry routes) runs the same validating intake fork_thread got in round-25 — a poisoned source row is rejected, not duplicated. Pin covers BOTH fork faces with a hand-poisoned store row; the round-25 ACP relative-cwd rejection gets its missing pin too (m26-6). M26-2: the four TUI load/resume faces move onto a new primary yet seeded the tolerant normalizer, durably re-minting a widening carried entry every intake refuses. seed_loaded_workspace_roots applies the core resume lane's rule — pure load tolerates (legacy rows stay loadable), a move validates and refuses before any app mutation. Pin: validates_moves_and_tolerates_pure_loads. M26-3: the apply_patch session-grant grouping key hashed only the payload headers, so the same body under two different top-level overrides shared one grant family — a session-approved patch auto-approved a redirect to an arbitrary target (the B20-2 class, patch arm). hash_patch_paths folds the aliases and keys an override exactly as execution's PathOverride-wins semantics do. Pin: grouping_key_rekeys_on_the_top_level_path_override. M26-4d/f: the revert_turn tool description's boundary clause is an unconditional literal, so single-root schemas changed — the description is now qualified, recorded, and pinned; the /fork <id> refusal copy no longer names the producerless re-declare step. Verification: the fork-ci clippy gate command clean; full codewhale-tui --lib 12,029 passed with 5 remote_control/model_inventory parallel flakes that pass single-threaded (unchanged class); the targeted suites for every touched area green. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
B27-1/B27-2 (both from the round-26 M26-2 shape): the TUI load faces re-home the app onto the RECORD primary during restore, so the seed is a pure load under that primary — the 'moved primary validates' arm seeded against the pre-restore app.workspace instead, materializing the launch directory as an undeclared writable root (persisted by the next autosave) and falsely rejecting primary-led rows from subdirectories (the record primary was never stripped). seed_loaded_workspace_roots now anchors at the record primary with the tolerant normalizer; the three faces seed at their original post-restore spots. Pin: seeds_under_the_record_primary_and_never_the_launch_directory. B27-3 + A28-3: the apply_patch grouping key is built from the REAL parser's plan (preflight_apply_patch) — the top-level override keys only in the patch form (replace-form top levels are decoys execution never reads), every accepted +++ spelling normalizes into the key, and deletions key under a separate delete marker. The old ad-hoc scan keyed an approved override grant onto arbitrary replace-writes and keyed all non-b/-spelled sections into one shared no_files family. Pins extended: replace-decoy equivalence, a/b-spelling collapse, delete-vs-write re-key. A28-1: resume/fork/start reject a RELATIVE cwd exactly like the empty spelling — the degenerate collapse used to destroy the declared root set and persist the broken primary, bricking later resumes. Pin: resume_with_a_relative_cwd_is_rejected_like_the_empty_spelling (row untouched). A28-2: ACP session/load validates the persisted root set before arming the registry — a poisoned row is refused with the intake reason instead of silently granting a fully-writable posture (REST resume 400s the same row). Pin: session_load_rejects_a_poisoned_persisted_root_set. A28-4/M27-1: the two macOS-deterministic fixture bugs fixed — the rebased-roots pin compares against the canonical attached spelling, and the snapshot nested-plain-root dir exists before the containment judgment (missing dirs canonicalize to the raw /var spelling). M27-2: both runtime fork faces clear the source session handle and task_id — ensure_engine_loaded prefers the handle and would replay the FULL source history, dropped tail included. Pin: both_fork_faces_clear_the_source_session_handle. M27-3: the CLI fork runs the validating intake on the source set before minting the copy. M27-4: the /tool empty-or-relative cwd rejection is typed as an IntakeValidationError so the app-server maps it to 400, per the lane's own comment. Verification: fork-ci clippy gate clean; core 117/0; full codewhale-tui --lib 12,032 passed with 4 parallel flakes (remote_control x3, clipboard wl_paste) that pass single-threaded. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
B1: the app-server intake pin asserts the stable 'cwd must be' fragment (production moved to 'a non-empty absolute path' three texts ago). B2: approval-cache grouping keys are domain-separated — patch markers vs touched-file names, and shell prefix vs cwd operand, are tagged and length-prefixed so neither class can impersonate the other (the round-29/30 dynamic collisions), with anti-collision pins. B3: the PATCH intake rejects a relative workspace with the start/resume/fork lanes' typed text (A28-1 doctrine) — the workspace-only leg used to silently drop the declared root set and persist the relative primary; red pin included. M31-1: a constitution READ failure is never cached (chmod-000 window no longer fail-opens repo law for the process lifetime). M31-2: the production ask-rule call sites' workspace_roots forwarding is source-pinned. Plus four CI-shape clippy fixes (print_stdout allows, from_ref idiom) so the gate runs clean at this head. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
B32-1: the update_thread pins migrate to the A28-1 text family (production moved to 'a non-empty absolute path'; create_thread's own lane keeps its original 'must not be empty' assertion). B32-2: the shell grouping key's operand-less (None-cwd) arm is length-prefixed like the operand arm — a forged single-token command containing the shell:<len>:<prefix>@cwd:<len>: shape no longer collides with a genuine operand-carrying key (the review's dynamic probe); anti-forgery pin added. M31-1 pin: the constitution read-failure no-cache fix gets its red-green leg (chmod-000 window self-heals on recovery; root-run environments skip the degraded assertion where 000 does not bite). M31-2 residual: the engine.rs composer-bang ask-rule call site's workspace_roots forwarding is source-pinned (all &self.config-bearing occurrences must forward). Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
exec --resume and the runtime resume_thread both consumed the persisted root set verbatim (normalize-only) while every sibling lane consuming a persisted set hard-refuses a corrupt/hand-edited row. Both lanes now run validate_workspace_roots before arming the engine config — a poisoned row fails the resume loudly instead of silently granting a writable sandbox in a headless run. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
B37-1: the exec resume lane validates the persisted row against its OWN
saved primary (the CLI --workspace/cwd is informational and never
re-anchors a resumed row); Op::SyncSession and the persistence stamp
carry the SAME validated, saved-primary-anchored set, so the lane no
longer mints moved rows (roots[0] = launch dir) that accumulate a
permanent writable root per cross-directory resume.
B37-2/B37-3: new stored-row primitive validate_stored_workspace_roots
strips the row's own cwd-first head (lexically, so ..-spelled copies
drop too) before the declared-count intake; wired across all six
re-declaring faces (exec resume, REST resume arming, ACP session/load,
CLI fork, POST /v1/sessions/{id}/resume-thread, and the stdio bridge's
create/PATCH wire), so a legal full-load row (64 additions + head)
round-trips whole instead of being bricked by its own gate. At-cap
round-trip pins cover every face.
M37-3: intake judges the path the filesystem resolves — an existing
declared root is canonicalized before the ancestor/filesystem-root
rejection, closing the symlink and case-fold widening spellings.
M37-7: get_writable_roots drops a root that canonicalizes to / for
every consumer — seatbelt used to emit it as a whole-filesystem write
allow while bwrap silently dropped it.
M37-10: workspace-scoped ask/deny rules own their whole scope subtree;
the exact-equality eligibility let a scoped deny be filtered out for a
call judged inside its scope (denied command ran).
M37-4: the in-app plain /fork and /save stamp the live set through the
same validating intake as every other fork face.
M37-6: session approval grants resolve the cwd operand against the
current primary and bind the patch family to it; the sets clear on a
workspace move and on fork/full-reset/session-load.
M37-9: the approval card's Dir preview reads [cwd, working_dir] like
the exec and key sides.
M37-1: the revert_turn tool description no longer carries review
meta-text onto the wire.
M37-8: the /cd and /fork widening refusals ship in all 15 locale packs;
a SyncSession that installs a different root set discloses it.
M37-11/M37-2: CHANGELOG hunks stripped per CONTRIBUTING (entries are
written on main at merge time); the true API behavior records in
docs/RUNTIME_API.md (cap 64, canonicalize-at-intake, stored-row strip
convention). Plus the round-37 resolve_resume_roots relative-primary
destruction guard.
Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
M38-1: the Allow eligibility arm returns to EXACT scope matching — the round-37 subtree matcher silently widened scoped allows to every cwd operand inside the scope's subtree, contradicting the standing fail-closed disclosures. Ask/deny keep the subtree (their fail-open was the bug); pin: a scoped allow fires at its root and NOT one directory down. M38-2: the load/persist lanes close at the root cause the /save fix scoped around. New tolerant companion sanitize_workspace_roots drops filesystem-root and primary-ancestor entries (same canonical+lexical judgment as intake) and reports the count; the four seed faces strip and warn explicitly instead of arming a live / that contained every path for the in-process file tools, and the autosave stamp runs through the same sanitize so a poisoned live set cannot launder onto disk. M38-3: new core primitive strip_workspace_roots_head (the lexical head strip, one implementation); validate_stored_workspace_roots delegates to it and the five raw-equality sites (resume-thread, bridge wire, the PATCH workspace-only move, /cd, /fork <id>) call it — a dot-dot-spelled copy of the OLD primary drops with the head on every move (pin). Records: the fork-policy figure refreshed at this head (309 files, +57678/-14710, net 42,968 ~ 28.65x), the two single-root-visible behavior entries added (apply_patch plan-time alias folding; the carve-out untrimmed-spelling branch), the canonicalize-at-intake sentence, the grant re-key disclosure, and the review-citation count corrected in the PR body. Minors: the resume-thread empty-workspace 400 mapping, the RUNTIME_API relative-workspace sentence, the intake rustdoc contract, the approval card cwd-null preview skip (pin), the two new refusal copies localized into all 15 packs, the /fork <id> refusal pin, the CLI fork at-cap pin, and the engine-arming poison-refusal half. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
B39-1: the sixth conversion face — core fork_thread's None-inherit arm — routes through strip_workspace_roots_head, so a dot-dot-spelled copy of the parent's primary drops with the head on bare-fork inheritance (pin). B39-2: the intake and sanitizer comparisons go through a spelling normalizer on both sides — on Windows, canonicalize's verbatim backslash-question-mark prefix never compares equal to the DOS-form lexical fallback, silently skipping the ancestor/fs-root guard whenever exactly one side canonicalized (an existing primary plus a not-yet-materialized ancestor was admitted). Mirrors the repo's normalize_windows_path_for_comparison precedent: strip the verbatim prefix, map UNC, case-fold (Windows only; Unix passthrough). Pins: the component-boundary helper test plus a cfg(windows) mixed-spelling ancestor rejection for the required windows-latest check. B39-4: the two RUNTIME_API contract sentences — a fresh relative-workspace row IS mintable through POST /v1/threads (the collapsed shape is not legacy-only), and the first-entry demotion applies whenever roots[0] differs from a present workspace field. Minors: the strip-and-warn receipt localized into all 15 packs with the count, the folded-space runs and the unactionable remedy gone, and the re-attach remedy named (pin); the sanitize drop count now covers the shape normalizer's drops and the cap truncation (pins); the PUT /v1/sessions arming poison refusal answers 400 like the resume face; the two display-only filters and the core move arm route through the shared strip; and the pin wave (sanitize canonical leg, empty-workspace 400, autosave-belt non-identity). Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
The round-39 B39-2 boundary heuristic ran on every platform, but ':' and '\' are legal Unix filename characters: a sibling named data: was misjudged as the ancestor of data:archive/ws (and data of a sibling literally named data\archive), so validate_workspace_roots REJECTED and sanitize_workspace_roots DROPPED legal attachments — three variants reviewer-reproduced with real Linux directories. The ancestor predicate is now platform-split: the native Path component comparison on Unix (canonicalize produces no verbatim prefix there; the mixed-spelling hazard does not exist), the spelling-normalized comparison confined to Windows where it is real. Pins: the colon/backslash sibling fixtures on Unix (both guards) and the boundary assertions moved under cfg(windows). Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
M1: the constitution repo-law globs compile case-insensitively on every host — the law's contract is that it may only ADD holds, and a wrong-cased create-shaped tail (VENDOR/new/x.rs against a vendor/** block) is resolved by the OS onto the protected directory on case-insensitive filesystems while the model's verbatim casing carried through all four target legs. Pinned; Unicode NFC/NND divergence stays a disclosed residual. Minors: create_session_from_thread validates the cloned set (the one minting lane that skipped intake); the thread SyncSession op ships the same validated, head-stripped set the engine armed with; the approval card skips empty-string operands like Null; the sub-agent write collector is alias-aware (path_param_value) like the session side; ACP session/load refuses a relative workspace like session/new and the REST lane; the placeholder-parity gate covers the Fork*/Save*/Sanitized* prefixes; zh-Hans/Hant full-width punctuation and the Catalan elision in the new keys; the model-facing Accessible-folders line escapes the list separator; and the SessionUpdated id-divergence path clears the session grant sets when the workspace is unchanged. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
M41-1: the REST resume-thread face refuses a RELATIVE session workspace like every sibling intake — forwarding it verbatim let create_thread's degenerate collapse destroy the declared root set and mint a relative-primary thread nothing else loads (the ACP guard's comment and the round-40 disposition claimed the REST lane already refused this row; both corrected). Pin: the relative spelling 400s on the HTTP harness beside the empty one. M41-2: the head strip gains a CANONICAL comparison leg (canonicalize with lexical fallback — the strip only runs on cold move/load faces): a symlink alias of the OLD primary (or a case-folded spelling on case-insensitive hosts) is admitted at intake because the ancestor rejection exempts equality, and survived the pure lexical strip, re-arming the abandoned directory after a move. Pin: the symlink alias drops through a real move face's re-validation. M41-6: the round-39 mixed-spelling Windows pin reworked onto the MIXED-EXISTENCE shapes the normalizer actually rescues (existing primary + not-yet-materialized ancestor, and the inverse); the old both-sides-existing fixture passed with the normalizer reverted. The sanitize leg gets its Windows shape too. M41-4 + locale grammar wave: the zh-Hans/Hant refusal keys punctuate full-width for real this time (the round-40 edit was a same-character no-op), the Catalan de+el contractions and elision residue fixed (del workspace), and the fr/uk/ru/hi/vi grammar defects in the new family repaired. M41-3/M41-5: the UNC-alias residual disclosed in RUNTIME_API (with the ASCII-only case-fold note), and AUTHORIZATION_ORDER's scoped-allow sentence qualified (an allow scoped to an attached root auto-approves calls judged there) plus the containment and unscoped-span precision fixes. Minors: the export mint degrades to the SANITIZED remainder with a warn (not a bare empty set); the PUT re-stamp runs the sanitize belt like the TUI autosave; the exec --resume mismatch warning regains its output-format gate; the Op::SyncSession doc says full cwd-first form; and the model-facing Accessible-folders line uses the shared strip. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
The first M41-1 edit died on an unmatched anchor before writing (its assertion fired before the file write), so the pushed head carried the corrected ACP comment claiming the REST refusal without the refusal — the review caught it. The guard now lands with an endpoint-level regression (the relative spelling 400s with the unified refusal message; the empty-spelling assertion follows the same producer). The M41-6 mixed-existence fixture drops the impossible first shape (an existing primary plus a not-yet-created SIBLING is a legal declaration, not an ancestor) and keeps the realizable nonexistent-primary shape for both the intake and sanitize legs. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
Round-41 zhuowp follow-up (native Windows probe): after the old primary directory disappears, both canonicalize calls fail and the strip's head comparison fell back to the raw lexical spellings — a stored case alias of the old primary compared unequal and rode the move, retaining the abandoned workspace slot. The fallback comparison now goes through the platform's comparison spelling (case-fold + verbatim strip on Windows, identity on Unix, where distinct spellings of a vanished directory remain distinct paths). Pin: a case alias of a vanished old primary drops with the head through a real move face (cfg(windows), runs on the required windows-latest gate). Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
M42-1 (the M41-4 residue, completed for real): the vi missing-word and hi garbled-tail defects fixed, the ru new-family terminology unified to рабочего пространства, and ja's four refusal keys punctuate full-width (codepoint-verified) — the ja item was silently dropped from the round-41 disposition and is accounted here. M42-2 (record correction, no third reword): the r37 reword carried a +12/-9 test-harness re-anchoring on the line tip — the body's byte-identical claim is corrected to name the delta; the r40 reword remains the genuinely byte-identical control. M42-4 (the preferred fix): the Unix ancestor leg canonicalizes both sides with lexical fallback before the native component comparison — the macOS /var-to-/private/var divergence for mixed-existence pairs dies, the deterministic red pin goes green, and the platform doc now states the fallback semantics. M42-3 + minors: the body re-measured at the pushed head; the remedy copy names the reload path (no /workspace producer exists) across all 15 packs with the pin updated; the constitution cache single-reads and inserts only on Ok (the probe/parse race that could cache empty rules); the strip's per-turn canonicalize cost ledgered with the cold-paths claims corrected; the TUI restore lane refuses relative primaries like its siblings (fixtures pinned to absolute workspaces); the export mint degrades to the sanitized remainder (pinned at the HTTP seam with store-level poison); the PUT mint arms run the sanitize belt; the M41-1 pin asserts nothing was persisted; and the stale comment sweep. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
…rage-derived disclosure, locale completeness) Windows fixtures (P2-3): every hand-written absolute-path fixture in core goes through a cross-platform abs_fixture helper — a POSIX /tmp spelling is not absolute on Windows and the (correct) intake guards refused it, regressing 28 core tests and both tool_success tests; the helper keeps Unix spellings byte-identical and maps to C:\ drive paths on Windows. Alias targets (P2-1): the nonexistent-target containment check accepts every declared root's canonical form, not only the workspace pair — a new file created through an alias of another declared root (primary/link -> attached-root/existing) resolves like the direct spelling instead of PathEscape. Pinned (alias vs direct spellings resolve to the same containment). Rollback disclosure (P2-2): restore_covers_primary_only derives from SNAPSHOT COVERAGE, not directory containment — a nested root under a built-in exclude (vendor/, node_modules/, ...) or inside a nested repository (mode-160000 gitlink) never reverts with the primary, so the boundary note fires for those shapes too; ordinary tracked nested roots stay covered. Pinned alongside the existing symlink cases. Locale completeness (P2-4 + P2-5): SanitizedLoadedRoots carries the full disclosure in every pack (count + runs-without + next-save persists + reload recovery — the dangling-semicolon and omitted-clause shapes gone, script-aware verification across all 15); the Save/Fork refusals carry the reload-or-repair remedy everywhere; and the rollback note is a real MessageId with translations (the TUI undo lane localizes; the portable /restore facet and the model/API lanes keep the documented English-fallback contract). Docs: AUTHORIZATION_ORDER's lead names exact-scope Allow vs subtree ask/deny; the revert_turn description states snapshot coverage (excluded dirs and nested repos keep changes) instead of the unconditional claim. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
Round-43 follow-up (zhuowp's refreshed probe): the abs_fixture sweep
missed the GENERATED root spellings — the cap tests build
format!("/repo/r{index}") and one /att{i} generator, which stay
POSIX-absolute (non-absolute on Windows) and collapsed the normalized
set to length 1 where the test expects 65. All generated fixtures now
route through the same helper (format! outputs coerced via as_str);
the checked-in cap input now normalizes to 1+64 on both platforms.
Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
R44-1 (the ghost-primary ancestor hole, probe-verified): a not-yet-materialized primary cannot canonicalize, so the Unix guard's canonicalize-with-fallback mixed the primary's LEXICAL spelling against the ancestor's CANONICAL one — on macOS-default /var-vs-/private/var shapes POST/PATCH admitted the primary's parent as writable. The primary now resolves through its deepest EXISTING ancestor before the comparison (the resolve walk); red/green-pinned on Linux with a symlink-alias ancestor under a ghost primary. The predicate doc and RUNTIME_API's canonicalization sentence corrected with it. R44-2 (the round-43-fix wave's own defects): the alias-target pin's expected prefix canonicalizes through the same walk (the macOS mixed-spelling class the review re-ran red); the doubled remedy sentences in hi/ja/zh-Hans/zh-Hant deduplicated with a segment-level no-duplication verification; the script-aware-verification claim is retired from the record (the localization delta was one MessageId — no structural gate exists, and the dedupe proves the point). R44-3 (NEW, end-to-end probe): a dot-dot-folded root declaration with a nonexistent mid-component silently disabled that root's constitution while execution admitted writes under the resolved root — discovery and all four target strips keyed the UNRESOLVED spelling. Repo law now keys discovery and the strips on the lexically RESOLVED spelling (normalize_path_lexically — execution's own containment identity); a genuinely nonexistent root still judges raw spellings (fail-closed), while ghost/../att lands on the resolved root's law. Pinned with the folded spelling through the real plan decision. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
R45-1 (the red required gate, three heads running): the rollback boundary pin asserted the SHORT English const while /undo renders the localized long form — red deterministically since the round-43 wave. The pin now asserts the localized note (locale-following), and both localization-heavy pins run on explicitly sized 8 MiB stacks so the CI RUST_MIN_STACK stop-gap is no longer load-bearing for them (P2-8). R45-2 (half-closed dedupe + the overclaim): hi.json still carried the doubled remedy in SaveLiveSetRefused/ForkLiveSetRefused (byte-level surgery, single occurrence asserted); the promised segment gate now EXISTS — `complete_packs_have_no_duplicated_segments` normalizes every complete-pack message to its word skeleton and fails on any 6-word window repeated inside one value, catching containment-shape duplicates the exact-repeat class misses. Developed red-first against the broken hi.json. R45-3 (i18n convention breaks): apply.rs's restore-refusal copy ships as SessionRestoreWorkspaceInvalid in all 15 packs (localized-locale pin included); /restore now rides the host-localized boundary note through a CommandSkillGroupContext default method (the English sentence moved to the contract crate as the single source, re-exported from crate::snapshot so the model/API lanes keep their spelling). R45-4 (four drifting copies of the ancestor walk): one core walk (`walk_deepest_existing`) now serves resolve_operand_cwd's Err branch, the R44-1 comparison, the carve-out resolver, and the tools' resolver — parameterized by fallback target (OriginalPath vs DeepestAncestor) and carrying the raw existing spelling for the tools' leniency gate. The operand lane's pinned dot-dot-skip semantics moved verbatim; the engine-parity pin caught and enforced this during consolidation. R45-5 (silently discarded sanitize receipts): all four sessions-API faces surface the drop count through warn_sanitized_dropped; a source pin keeps any future site from rebinding `_dropped`. P2-10 folded in (NotFound-gated operand walk: EACCES/ELOOP operands keep their own spelling, judging policy against the right directory); P2-14/15 folded in (the overclaiming repo-law containment comment corrected, the symlink+dotdot mirror and the nonexistent-root fail-closed direction both pinned). Also absorbs the maintenance-batch collisions (elevation helpers retired by Pinvou#102, toolbox eviction tests from Pinvou#104, SyncSession literal keychain field). Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
Round-46 P2 riders (the REQUIRED is a record fix, landing in the PR
body):
- P2-1: resolve_deepest_existing's always-compose flip documented
truthfully (a ghost-component + `..` spelling used to be DENIED
modally by the walk's file_name corner; it now composes onto the
parent and qualifies — a false-denial fix, single-root-visible) and
pinned with the ghost_dir/../src/main.rs carve-out shape.
- P2-2: validate_workspace_roots' doc no longer claims the ghost-primary
"existence-dependent residual" R44-1 closed three hundred lines up;
the closing pin is named in its place.
- P2-4: the round-44 dedupe had glued sentence boundaries in ru/vi
("({reason}).Перезагрузите" / "({reason}).Tải lại", SaveLiveSetRefused
and ForkLiveSetRefused); spacing restored, byte-verified from disk.
The R46-1 record itself (the compile-broken 25-commit window repaired
at the tip, the replay carrier's false provenance, and the re-measured
figures) is owned in the PR body — squash-merge discards the history,
so the record is the only durable home.
Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
Round-47 riders: - Item 3 (P2-1's replacement doc was wrong twice): for `<ws>/ghost_dir/../target` the shared walk composes `<canonical-ws>/ghost_dir/target` — the trailing `..` pops without canceling the ghost component, which re-appends into the tail — while the LEXICAL landing is `<ws>/target`; judging the execution- materialized composed spelling is exactly why the carve-out admits (both spellings in-tree). Both the resolver doc and the pin's comment now say this precisely. - Item 1 (P2-5): the PATCH active-turn fence's message now carries the "already has an active turn" substring `map_thread_err` keys 409 on, matching the four sibling turn-start guards — a mid-turn roots PATCH is no longer surfaced as a permanent 400. The R47-1 record (the compile-broken window, the replay carrier's provenance, the re-measured figures) lands in the PR body with this commit: the round-26 edit attempt failed silently (a GraphQL deprecation notice masked it) — disclosed in the body's process note. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
9e23403 to
be2cd01
Compare
|
Round-47 follow-up — head The base advanced (#107 route limits), forcing a rebase. I ran the per-commit The body's provenance disclosure is updated accordingly: the window/repair/provenance facts stand as the historical record of the Local gates at |
asto18089
left a comment
There was a problem hiding this comment.
Round-48 fresh review — APPROVE
Head reviewed: be2cd01ef (28 commits). Base pinvou3-clean@f27672935 (#107) == merge-base == current tip — rebase verified a no-op at this head. This round started at 9e234030e on 480bebf61; the author's mid-review push rebased onto the advanced maintenance head, and the replay is tree-faithful: git diff 9e234030e be2cd01ef is exactly #107's client.rs absorption (124/−5) and nothing else — no replay drift, no lost content (the round-45 damage class did not recur).
Method: a full from-scratch pass — 13 review lanes (core intake/chokepoint hunt; core carve-out+materialization with 11 bypass constructions; execpolicy scope matching incl. both disclosed fail-open directions; state/v5 migration; protocol+app-server wire; runtime HTTP face; ACP/stdio/CLI; repo-law/authority/constitution cache with 11 more bypass attempts; tools (spec/apply_patch/approval_cache/shell/revert_turn/subagent); engine+session lifecycle; TUI commands/UI/locales with a machine locale audit; smuggling/docs/claims-static incl. a per-commit damage-site scan; cross-cutting perf) plus orchestrator first-hand gates at the tip: cargo fmt --check clean; cargo check --workspace --all-targets --locked green; clippy green under fork-ci's real allow-list; core 117 / execpolicy 125 (Linux) / state 28 / protocol 70 / app-server 108 / command-contract 43 — all matching the body; targeted tui suites green (authority 22, runtime_threads 183, runtime_api 196, localization 50, repo_law 28, spec 47, approval 132, snapshot 74, session_manager 75, acp_server 62, engine 433).
Gates note: the body's quoted clippy command omits two allows fork-ci actually passes (fork-ci.yml:52-53: -A clippy::collapsible_if -A clippy::assertions_on_constants). Reproducing the body's command locally goes red on a collapsible_if at turn_loop.rs:5339 that is pre-existing at base — CI's real command is green at this tip. Worth correcting the quote so reviewers don't chase a phantom red.
Round-47 REQUIRED closure — verified CLOSED, and superseded by something better
R47-1 (the record) is written — and the round-47 rebase closed the compile-broken window at the source. Verified first-hand, not from claims:
- I machine-scanned both damage sites in all 28 trees: the unclosed
assert!(intoolbox_note_survives_a_batch_without_resultsand theworkspace_roots-lessOp::SyncSessionliteral insession_sync_drops_a_parked_toolbox_eviction_noteare repaired in every commit of the replayed line (zero failures), andcargo check --workspace --all-targetspasses at the two previously-uncompilable trees I probed (80a1b3021the carrier,e70ee0599the round-24 residue commit the body names). The per-commit "gate runs clean" claims in the window are now true at their own trees. - Figures re-measured: drift 100 files, +14,904/−945 exact; fork surface vs
dcd4c200f445 files, +67,187/−21,066 exact; locales 2,100 × 15 packs, 16 additive keys, zero removals, zero placeholder mismatches (machine-checked); DCO 28/28, author==committer, one matching sign-off each. - Rider closures verified: the PATCH active-turn fence now carries the
already has an active turnsubstring (runtime_threads.rs:5998) thatmap_thread_err(runtime_api.rs:7352) maps to 409, matching the four sibling guards — end-to-end traced; and P2-1's replacement doc now matches the walk exactly (<canonical-ws>/ghost_dir/targetcomposed vs<ws>/targetlexical — I tracedwalk_deepest_existingcomponent by component; both spellings in-tree is structurally true at every consumer).
Criterion verdicts (fresh, this round)
- Useful / complete / root-caused: PASS. The chokepoint claim survives a from-scratch cross-crate hunt: every thread-root mint/move/arm/re-persist lane (core spawn/resume/fork, runtime REST create/PATCH/fork/resume, app-server stdio bridge + hint remap, ACP new/load, exec resume, TUI /cd + load faces + autosave belt, CLI) crosses a validating face; the tolerant-load boundary cannot launder a rejected shape into an armed set. The disclosed producer gap is real and the infrastructure shipped is internally consistent — empty ≡
[cwd]holds on every face traced, wire included. - Elegance: PASS. The R45-4 one-walk consolidation, the v5
BEGIN IMMEDIATEmigration (presence check inside the transaction, explicit rollbacks, idempotence), the boundary memo and constitution cache are the right shapes; no dead code found; both perf commits do what their messages claim (verified against base's per-call re-read+recompile shape). - Smuggling: CLEAN. All 100 files classified topic/disclosure/test; CHANGELOG, Cargo.lock,
.github/untouched; the only non-topic lines are the disclosed clippy ride-alongs (mcp.rspub(crate)widening,shell_dispatcher.rslint fix); zero TODO/FIXME/dbg!/commented-out code introduced. - Bugs/perf: no new fail-open found. 22 bypass constructions across the carve-out and repo-law lanes all failed into the disclosed residual classes (prefix confusion, symlink hops, absolute/trailing
.., ghost+.., dangling tails, whitespace, case, unicode folds, alias spellings, EACCES fallbacks); the two fail-opens that exist are the body-disclosed spelling-silence residuals, verified accurate against the code. Persistence/migration/wire/downgrade sound. Perf is linear, 64-cap-bounded, and a net win vs base.
P2 ledger (non-blocking, fix-or-ledger; items 1–3 worth landing pre-merge if convenient)
- fs-root primary doctrine gap (new; pre-existing shape, but this PR wrote the doctrine):
POST /v1/threadsaccepts{"workspace": "/", "workspace_roots": []}—create_threadguards the workspace only for emptiness (runtime_threads.rs:5490-5493) andvalidate_workspace_rootschecks the cwd slot only for empty/relative (core/src/lib.rs:509-511) while the roots slot refuses/with "attaching it would make the whole filesystem writable" (:540-544).boundary_roots(tools/spec.rs:1120-1131) is built verbatim from the normalized set, so a/primary neuters the in-process containment for every resolve (reads under Auto; writes stay modal-gated since the carve-out needs a.gitmarker at/, and the OS-sandbox enumeration drops canonical/— M37-7). Same client trust level, same shape, opposite doctrine — and RUNTIME_API.md documents the relative-primary mint (B39-4) but not this. Guard it at the same chokepoint or disclose it beside B39-4. - The P2-14 "pinned" closure is hollow (new):
symlink_dotdot_root_declaration_keys_the_fold_not_the_kernel_truth(repo_law.rs:552-616) declareslink → <parent>/elsewhere— inside the parent — sorealpath(<parent>/link/../x) = <parent>/x= the fold. The geometry has no fold-vs-kernel divergence; the test's write is admitted via the primary's containment, not the declared root's, and the assertion would still pass under the scheduled kernel-truth re-keying. The residual itself is honestly disclosed (code comment + body, scheduled); the round-45 body sentence "the symlink+dotdot mirror … pinned" overclaims. Re-cut the fixture with an out-of-parent symlink target so the pin can actually fail. - Snapshot exclude-list drift (new):
SNAPSHOT_EXCLUDED_DIRS(snapshot/mod.rs:145-168) hand-copiesBUILTIN_EXCLUDES(snapshot/repo.rs:142) and omits six entries (.mypy_cache,.pytest_cache,.ruff_cache,.gradle,.m2,.local). A nested attached root under one of those is excluded from snapshots butrestore_covers_primary_onlyjudges it covered and withholds the boundary note —/restoreoverclaims coverage. Derive one list from the other or add a parity pin. - Case-variant scoped deny on macOS (round-46 P2-6 / round-47 ledger 2, still open, re-confirmed by trace):
normalize_workspace_scopepreserves POSIX case (execpolicy/src/lib.rs:1113-1119) while the path channel folds on case-insensitive platforms — a deny scoped/REPOnever fires for/repoon a default macOS volume (fail-open), and adding a scope makes a rule strictly weaker there. Covered by the disclosure's letter, unnamed as an axis; the macOS-aware fold or a disclosure clause still closes it cleanest. - Carried (round-47 ledger 7): constitution-cache
(mtime, len)key spoofable with exec access, and the process-wide Mutex spans stat→read→parse→compile (constitution.rs:116-173) — a stalled/FUSE constitution head-of-line-blocks every write-gate call. - Carried (round-47 ledger 5):
/renameand/titlestampapp.workspace_rootsverbatim (rename.rs:125/:180,title.rs:171) while/saveand/forkre-validate — one future App-writer away from a laundering lane. - Carried (round-47 ledger 8): the 8 MiB explicit-stack wrapper is hand-rolled triplicate, and
restore.rs:440-443still asserts CI'sRUST_MIN_STACK"is load-bearing" for a test that wraps its own stack. - RUNTIME_API.md gaps: the turn_meta
Accessible folders:disclosure (zero hits in docs/) is invisible to API readers; the restoreboundaryfiring condition (:852-857) is narrower than code (tagged-snapshot re-key, store-read failure, and nested-root escapes also fire it; untagged legacy never does). - AUTHORIZATION_ORDER.md overclaim: the new "an UNSCOPED rule spans the whole set" is true for ask/deny only — an unscoped Allow stays gated to the judged-cwd slot (
&[0],execpolicy/src/lib.rs:486-503; the crate's own doc comment says exactly this). - Record nits: the "This push" header says both "figures below re-measured at this head" and "measured at
9e234030e" (partial-edit self-contradiction); the single-root-visible entry "missing-thread resume/fork 404 (was 200)" describes the pre-rebase base — both legs already exist at480bebf61; the window rangeacdaf8d2c..59344db94is 24 commits (25 counts the carrier, which is also broken, so the substance holds). - PATCH-fence pin weaker than the mapping it protects:
update_thread_roots_rejects_active_turn(runtime_threads/tests.rs:6181) asserts onlycontains("active turn")while the 409 hinges on the exact substringalready has an active turn— a wording drift re-creates the retry-forever 400 under a green suite. Pin the exact substring. - Stored-row faces run the canonical strip before the intake cap (
core/src/lib.rs:569-594viavalidate_stored:624-626, resume:768-773, fork:1420): a hand-edited 100k-entry row costs one realpath walk per entry per resume/fork before the cap rejects. A one-line pre-count closes it. - Perf (linear, non-blocking): 64-root sessions pay ~64 realpath walks + ~800–1000 warm stats per write-gate call (the deliberately-uncached constitution discovery walk dominates; cold/network FS is the visible case); the no-law fast path regressed slightly (
collect_write_candidatesruns two deepest-existing walks before any constitution-existence check,repo_law.rs:84,252-254— base did zero FS work when no law existed). Candidates for the post-merge pass with item 5.
CI note
fork-ci is ubuntu-only and ci.yml's 3-OS matrix gates on master/main (verified in the workflow files); platform validation lands with the merge target's CI, as the body states. The be2cd01ef fork-ci run was still in progress at posting time; every gate it runs was re-earned locally at this tip (fmt, check, clippy under the workflow's exact allow-list, and the suites above), plus compile probes at two previously-broken mid-line trees.
Verdict
APPROVE. R47-1 is closed and superseded — the record exists and the compile-broken window is gone at the source (machine-verified across all 28 trees); both round-47 riders landed and verify; the rebase onto #107 is tree-faithful; all gates are green at the tip; the from-scratch adversarial sweep (22 bypass constructions, chokepoint hunt, wire/migration/isolation audits, locale machine check) found no new fail-open and no smuggling. The P2 ledger above is fix-or-ledger for the post-merge pass; none of it blocks — items 1–3 are the set worth folding before merge if convenient.
R1 (the MAJOR merge blocker): the two keychain writers were the last mutators bypassing the cross-process section lock. `set_session_workspace_roots_with_explicit_ancestors` (and through it the `set_session_workspace_roots` wrapper and the detached sweep) now runs section lock → fresh reload → mutate → persist, refusing a corrupt index instead of replacing it with the stale table — a CLI `projects rebind` / `sessions delete` committed from a second process can no longer be reverted by the GUI's whole-table persist, and a sibling's reload can no longer wipe an applied align from memory while it still reports applied. Both command callers (align kernel, rebind detached sweeps) join on blocking workers via `on_blocking_labeled` so the lock wait never stalls the async executor (the round-49 convention). Two section-truth pins pin the writer and the sweep; dropping the lock was mutation-verified to turn both red. Registration text (the second blocker, the r10 R2 class re-staled through rounds 37-44): the guard banner now derives its count from EXPECTED_COMMITS (94, r1 tag + 79 topic/maintenance per the measured range) instead of a frozen number; the layer-0 success message names the asserted MAINTENANCE_HEAD (480bebf61) instead of a head nothing asserts; both registers' §0/maintenance/drift rows carry the measured values at the new pin (94/79 registered, 53 maintenance squashes, topic drift 100 files +14863/-945 measured 480bebf61..4755bac88), and the guard's topic-lead check is a named constant (26). Should-fixes: SF2 — the native sidecar persists the align-granted ancestor provenance (additive serde-default field) and the index-loss rebuild carries it, closing the grant-evaporation window (pin covers survive-and-reset). SF6 — rebind_workspace_prefix resets the provenance with the replacement keychain, unifying the invariant across lanes (sidecar writers already reset). SF7 — same-shape repeated bind/set_acp_workspace resets the provenance in the same write. Also: the reviewer carry's subject shortened to pass the commit-message gate (description 52→26 chars; tree and authorship preserved via commit-tree surgery), main merged (0.10.10 #697 + gitlink #696), and the CodeWhale gitlink re-pinned to the round-45 head 4755bac88 (rebased onto the 480bebf61 maintenance batch; PR Pinvou/CodeWhale#54). SF3 (align live-push turn gate across probe→load→send), the SF4 batching half (one persist per sweep), SF5 (defer the compose-time provenance read + the composer-dropdown/chip frontend pair), and the four coverage-gap pins stay on the ledger with the standing #54 → re-pin → required-gate handoff. Signed-off-by: qiuYliangM <185303122+qiuYliangM@users.noreply.github.com>
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
/workspace(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
be2cd01ef— 28 topic commits over the public maintenance headf27672935(#107): the round-47 rebase repaired the replay damage at the source, closing the compile-broken window; figures below re-measured at this head; the round-45 close rebased onto the 2026-10-13/14 maintenance batch and closed its REQUIRED set, the round-46 commit carried the first riders, and the round-47 commit corrects the record's own errors and maps the PATCH fence 409; figures below are measured at9e234030e)Round-45/46 disposition (head
4755bac88, all five REQUIREDs verified closed by the round-46 review with its own mutations M-A/M-B/M-C)SessionRestoreWorkspaceInvalidin all 15 packs;/restorerides the host-localized note through a new contract facet whose default is the stable English const (single-sourced in command-contract, re-exported fromcrate::snapshot). R45-4 one core walk (walk_deepest_existing, two explicit fallback variants) with four delegations; the engine-parity pin caught and enforced the dot-dot-skip semantics during consolidation. R45-5 all four sessions-API faces surface the sanitize drop receipt; a source pin trips on any_droppedrebind. P2-10 (NotFound-gated operand walk), P2-14/15 (repo-law comment corrected; the symlink+dotdot mirror and the nonexistent-root fail-closed direction pinned) folded.Provenance disclosure — the 2026-10-14 rebase replay, its silent repair, and the round-47 source closure (owned here because squash-merge discards the history)
f27672935(fix(tui): keep host-declared route limits for same-model model-aware requests #107) repaired both damage sites in the carrier commit itself (plus one maintenance-signature interplay in the round-24 residue commit), and a per-commitcargo check --workspace --all-targetspass over the whole replayed line verified zero failures — every commit in the line now compiles at its own tree. The disclosure below describes the pushes at4755bac88/2b1704ca0/9e234030e, where the window was still live; it stands as the historical record of what shipped to review.480bebf61as the machine carrieracdaf8d2c; its replay of the maintenance batch mangledcrates/tui/src/core/engine/tests.rsin exactly two places — (1) thetoolbox_note_survives_a_batch_without_resultsassert!left unclosed, which also swallowed the Round-20 B20-1 test into the macro (parse error), and (2) theOp::SyncSessionliteral insession_sync_drops_a_parked_toolbox_eviction_notemissing this topic's ownworkspace_roots: Vec::new(),(E0063; the replay updated 14 of the file's 15 literals). Every commitacdaf8d2c..59344db94therefore failscargo check --workspace --all-targetsat its own tree; the tip4755bac88repairs both silently. Consequence, owned: the per-commit verification claims inside that window (193975cda"gate runs clean at this head",d361539e4,30e711837"Windows green",59344db94"probe-verified / red/green-pinned") are byte-identical replay claims inherited from the old compiling line, not re-earned at their own trees; only the core-crate pins in the window remain possible as stated (core does not depend on tui). The repaired state is fully verified at the tip (fork-cicheck+gategreen).acdaf8d2cstill tells the round-33 story ("re-derived against the public base0bc868ff0… moved 68 files (+7,765/−654)") while its actual tree is the old carrier plus the 155-file, +6,212/−10,969 maintenance replay of3118c36b8→480bebf61(the manufactured 2026-09-30 author date the message sits on pre-dates0bc868ff0). The replay itself is commit-for-commit faithful — the round-46 drift audit confirms nothing from the reviewed old line is missing at the tip (docs byte-identical; residue = the two-line syntax damage + benign EOF/align artifacts) — the false provenance is the only untruth in it, and this section retires it.be2cd01ef(prior body figures were stale at each earlier push): topic drift vs the maintenance head 100 files, +14,904/−945 (f27672935..be2cd01ef, 28 topic commits); fork surface vs upstreamdcd4c200f445 files, +67,187/−21,066, net ≈30.6× the 1,500-line soft limit; locale keys 2,100 per pack; suite counts moved with the base — core 117, execpolicy 124 (125 on Linux), state 28, protocol 70, app-server 108, command-contract 43.Round-46 riders
resolve_deepest_existing's always-compose flip is documented exactly (round-47 item 3 corrected the round-46 doc's own two errors: for<ws>/ghost_dir/../targetthe walk composes<canonical-ws>/ghost_dir/target— the ghost component re-appends into the tail — while the lexical landing is<ws>/target; judging the execution-materialized composed spelling is why the carve-out admits, both spellings in-tree) and pinned (carve_out_admits_a_ghost_dotdot_shape_composed_onto_the_parent) — a single-root-visible behavior change, added to the single-root-visible list below.validate_workspace_rootsdoc's stale ghost-primary "existence-dependent residual" claim corrected (R44-1 closed it; the pin is named)..Перезагрузите/.Tải lại, 4 keys); spacing restored byte-level./undo'sprimary_onlytranscript sentence ("/undo reverted the primary workspace to snapshot …") stays English beside the localized boundary note; the generic template it extends is the pre-existing EN surface.・/「」are outside the delimiter set, so a pasted CJK clause can still slip (P2-12).already has an active turnconflict substring, somap_thread_errmaps it 409 like the four sibling turn-start guards (an IDE client no longer sees a retryable-mid-turn PATCH as a permanent 400).gh pr editthat was supposed to land this record failed silently (a GraphQL deprecation notice masked the failure); the record physically reaches the body with THIS edit, byte-verified against the live PR before the round-47 reply. The round-46 commit's claim "owned in the PR body" was therefore untrue for six hours; this section is the correction.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/workspaceblocked 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 codewhale-hq#5185 carve-out behavior extended to N roots.Test record (refreshed per review round; round-46 basis at head
4755bac88)cargo check --workspace --all-targetsgreen;cargo fmt --all --checkclean; the fork-ci clippy gate command (--workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or) clean.cargo test -p <crate> --libat4755bac88, Linux arm64): codewhale-core 117, codewhale-execpolicy 124 (one pin is#[cfg(target_os = "linux")]; Linux measures 125), codewhale-state 28, codewhale-protocol 70, codewhale-app-server 108, codewhale-command-contract 43 — all green (the counts moved with the 2026-10-13/14 maintenance base: test: drop verified worthless tests across crates #102 dropped verified-worthless tests). (Round-37 overhauls this section from its round-28 basis per the review's records ask, adding the app-server and command-contract counts alongside tui.)codewhale-tui --libunderLC_ALL=en_US.UTF-8 RUST_MIN_STACK=16777216on Linux arm64: 12,164 passed at52f2de016(the remote_control trio under the parallel run passes 83/83 single-threaded; composition varies within the band — the pair vs trio shuffles withno_new_privs, see round-41 minor 10) — the two are the documented environmental band (remote_controlscheduling;no_new_privsfails at base too — whole suite 83/83 single-threaded) on this machine (the round-37 review measured the same class on its own box and ruled them environmental, CI stays the authority); the wholeremote_controlsuite passes single-threaded here (83/83), including both flakes./-canonical root drop, the scoped-deny subtree pair, the grouping-key root-binding trio, and the workspace-move/conversation-change clear.RUST_MIN_STACK=16777216; locale-sensitive lanes pass underLC_ALL=C. Linux CI remains the green authority.be2cd01ef: 28 topic commits over the public maintenance headf27672935; drift vs the maintenance head: 100 files, +14,904/−945; every commit authored by qiuYliangM with exactly one matching DCO sign-off (the replay carrieracdaf8d2ccarries the manufactured author date of the 2026-09-30 compression — see the provenance disclosure above).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 canonicalizes existing spellings: 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 IS implemented (round-37 M37-3): an existing declared root canonicalizes before the ancestor/filesystem-root comparisons, so symlink and case-fold spellings of the primary's ancestor are rejected; nonexistent declared roots keep the lexical judgment (materialization stays the delivery lane's job, same-disk TOCTOU disclosed inherent), and a nonexistent PRIMARY also keeps its lexical spelling — on hosts where the primary's canonical form differs from its spelling (macOS/tmp↔/private/tmp) the ancestor guard is existence-dependent on the primary (round-38 disclosed residual; the grant stays bounded by the declaration).Canonical-judgment scope spelling (disclosed; remediation scheduled post-merge): exec policy now reads the operand's canonical spelling (the file-tool lane judges the raw spelling — its own residual), 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 on the allow face; the SAME verbatim-prefix mechanism silences subtree-scoped DENIES on every existing-target operand call on Windows — a 100%-frequency fail-open disclosed here (the scheduled verbatim-prefix trim closes it):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 and, since round-41 M41-1,session/loadREFUSES poisoned/relative rows with the intake reason instead of tolerating-and-arming (acp_server.rs, pins alongside); the turn_meta line andsession_configurationstill omit the set — 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);/workspacepersists 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; round-37 M37-6 additionally resolves the operand against the current primary and binds the patch family to it, clearing the sets on workspace moves and conversation changes). Every persisted grant re-keys once on upgrade — including no-operand families, whose key format length-prefixed (round-20/32) — so a one-time re-prompt after upgrading is expected and fail-closed (round-38 correction: the CHANGELOG entry that disclosed this was stripped per the merge-time clause, so it is recorded here). 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 (theexec/serve --mcpsubcommands still consume the raw flag — ledgered SF22-5); the intake chokepoint returns an empty set (fail closed) for an empty/relative cwd where it previously produced[""]; and the/workspaceblocked message moved from a hardcoded English string to the localizedWorkspaceSwitchBusy, with the degraded-success receipt downgraded from sticky Error to Warning. Round-38 M38-5 additions: the apply_patch plan-time gates foldPATH_ALIASESexactly like execution (the base judged raw input at plan time while execute folded — every plan-time consumer in single-root sessions, ask_rules/auto_review/work_surface/engine prep/LSP hooks, now judges the aliased override target instead of decoy headers, so an alias-spelled write to a protected location moves from modal-free to modal/blocked and conflicting alias+canonical spellings error at plan time; fail-closed); and the carve-out branches on the untrimmed operand spelling (a leading-space spelling like" /etc/passwd"goes from modal to modal-free because the true landing path — a padded junk name inside the workspace — is what gets written and is not excluded; head judges what is actually written). Round-39 minor 6 additions: the fork faces clear the sourcesession_id/task_id(M27-2 — a forked thread no longer replays the source session file into the model context; wire-shape change),get_writable_rootscanonicalizes every configured writable root (base mounted verbatim — observable where spellings diverge), and ACPsession/newrejects an empty/relative cwd where base silently fell back to the default (fail-closed, pinned). 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: the fork surface measured against upstream
dcd4c200fat this headbe2cd01ef(git diff --shortstat dcd4c200f..HEAD, re-measured at every pushed head) is 445 files, +67,187/−21,066 (net 46,121 ≈ 30.6× the 1,500-line soft limit; the net is diff-algorithm-independent; the prior 350-file/≈33.57× figure was the pre-rebase head's).Round-37 disposition (head
08a972336)B37-1 closed. The exec resume lane validates the persisted row against its OWN saved primary (
validate_resume_workspace_roots);Op::SyncSessionand the persistence stamp carry the SAME validated, saved-primary-anchored set — the CLI--workspacenever re-anchors a resumed row, no moved row is minted, and the "silently degrades to single-root" leg (relative--workspace .) dies with the CLI anchor. Three pins inexec_agent.rs(cross-directory anchor, at-cap whole, poison refusal with the row-state wording).B37-2/B37-3 closed. New core primitive
validate_stored_workspace_roots(lexically strips the row's own cwd-first head —..-spelled copies drop too — then reusesvalidate_workspace_roots), wired at all six re-declaring faces: exec resume, REST resume arming, ACP session/load, CLI fork,POST /v1/sessions/{id}/resume-thread(strips before declaring), and the stdio bridge's create/PATCH wire (runtime_thread_roots_wire; both wire-level harness tests re-anchored to the declared form). The two fork faces switch to the same primitive. At-cap round-trip pins cover every face (64-declared create → 65 stored → resume whole).M37-3 closed by fix. Intake judges the path the filesystem resolves: an existing declared root is canonicalized before the ancestor/filesystem-root rejection (symlink pin included; the case-fold spelling is the same mechanism on case-insensitive filesystems); nonexistent declared roots keep the lexical judgment, with the materialization/TOCTOU residual unchanged and disclosed.
M37-7 closed by fix.
get_writable_rootsdrops a root that canonicalizes to/at the shared enumeration — seatbelt's(subpath /)full-write allow is gone, and both platforms now fail closed identically.M37-10 closed by fix.
workspace_scope_matchesowns the scope SUBTREE: a scoped deny (and ask) fires for a call judged anywhere inside its scope — the fail-open for/repo/subvs scope/repois closed (pin included, outside-scope negative included).M37-4 closed. The in-app plain
/forkand/savestamp the live set through the same validating intake as every other fork face (refusal pin: poisoned live set refused, nothing persisted, session not switched).M37-6 closed. Session approval grants resolve the
cwd/working_diroperand against the CURRENT primary before keying (the resolved spelling also closes the symlink-respelling key drift) and the patch family is root-bound; the grant/denial sets clear on every workspace installation (apply_workspace_runtime_state) and on fork/full-reset/session-change (Op::SyncSessionarm). Pins: relative-operand re-key, absolute-redirect family stability, patch root binding, and the clear.M37-9 closed. The approval card's Dir preview reads
["cwd", "working_dir"]exactly like the exec/key sides — aworking_dirredirect is visible on the card that approves the grant keyed by it (pin included).M37-1 closed. The
revert_turnmodel-facing description no longer carries review meta-text; the pin now asserts the negative.M37-8 closed. The
/cdand/fork <id>widening refusals ship in all 15 locale packs (machine-checked key/placeholder parity, 2,100 keys each); aSyncSessionthat installs a different root set discloses it via the standard roots notice.M37-11/M37-2 closed. The +110 CHANGELOG hunks are stripped per CONTRIBUTING (entries are written on
mainat merge time); the true API behavior — the intake cap (64), the canonicalize-at-intake promotion (replacing the "scheduled" wording), and the stored-row re-declaration strip convention — is recorded indocs/RUNTIME_API.md. The four false 404 faces, the falsifiedPOST /toolclaim, the stalestring_fieldreference, and the Windows scoped-deny fail-open all die with the CHANGELOG text they lived in; the scoped-deny direction itself is now FIXED (M37-10) rather than disclosed.M37-5 closed. Both commit messages corrected by a history reword (
0a9b01ed1→fa39ee8fa: the byte-identical claim replaced by the re-derivation facts;7d71a5750→bd1b84e71: thematching_ask_ruleclause re-attributed tob90c434aeinline). The rebuilt line's tree is byte-identical to the pre-reword line (verified); the pre-reword line is public onbackup/pre-r37-reword-20261011.Minors taken in the same wave: the
..-spelled head bypass (lexical strip everywhere, pins), theresolve_resume_rootsrelative-primary + explicit-replacement destruction guard, and the exec silent-erase leg (dead with the B37-1 anchor). Minors to the ledger (disclosed, none blocking): the constitution cache bounds/window/eprintln cluster, the pruned-worker posture resurrection leg, the B2 override-patch unparseable-pin idle, theask_rulespath-alias leg, the non-UTF-8 serialization fallback, the PATCH-during-turn 400/409 wording, the turn_looprfindpin scope, the noticesaturating_sub/.replace-chain display items,POST /v1/threadsall-400 mapping, and the B27 face-level wiring pins.Round-38 disposition
workspace_scope_matches_exactgates the Allow arm; ask/deny keep the subtree (their fail-open was the bug). The round-37 disposition's claim that the allow-direction disclosure "stays accurate as the mirror" was FALSE with the symmetric matcher and is corrected here: it is accurate again because the widening is reverted, pinned byworkspace_scoped_allow_stays_exact_for_subdirectory_operands(fires at the scope root, does NOT fire one directory down).sanitize_workspace_roots(drops filesystem-root and primary-ancestor entries with the same canonical+lexical judgment the intake applies, returns the drop count).seed_loaded_workspace_rootsuses it: the four load faces (/load, picker,/resume,exec --resume) now STRIP-AND-WARN (an explicit tolerance decision — the session stays loadable for repair, the drop is named in the transcript vianote_sanitized_loaded_roots) instead of arming a live/that contained every path for the in-process file tools. The autosave/checkpoint stamp routes through the same sanitize, so a poisoned live set can no longer launder onto disk turn after turn...-spelled head closure is now real. New core primitivestrip_workspace_roots_head(the lexical strip, one implementation);validate_stored_workspace_rootsdelegates to it and the five raw-equality sites (resume-thread, the bridge wire, the PATCH workspace-only move,/cd,/fork <id>) now call it — a..-spelled copy of the OLD primary drops with the head on every move (pin:workspace_move_strips_a_dot_dot_spelled_old_primary_copy). The round-37 "lexical strip everywhere, pins" claim was false when written; it is accurate now.cwd: null→ working_dir preview (Null skip like the key side, pin), the two new refusal copies localized into all 15 packs (2,097 keys, parity machine-checked), the/fork <id>refusal pin (refused, nothing persisted), the CLI fork at-cap pin, and the engine-arming poison-refusal half.resolve_deepest_existingconsolidation, the patch-key DefaultHasher collision class (pre-existing; the round-37 root-binding fixed the cross-root ride, not the offline collision), the restore-face boundary doc wording, and the parity-gate prefix filter coverage.Round-39 disposition (head
ebf211c35)fork_thread's None-inherit arm routes throughstrip_workspace_roots_head; a..-spelled copy of the parent's primary drops with the head on bare-fork inheritance (pin:fork_without_roots_strips_a_dot_dot_spelled_parent_head_copy). The "every move" invariant, the primitive's doc, and the RUNTIME_API sentence are accurate again.comparison_spellingnormalizer: verbatim\\?\stripped,\\?\UNC\mapped, case-folded (Windows only; Unix passthrough — case stays significant), plus a separator-boundarycomparison_contains. Pins: the component-boundary unit test (Unix) and a#[cfg(target_os = "windows")]mixed-spelling ancestor rejection that runs on the required windows-latest check. The execpolicy scope-matching\?\residual stays disclosed (minor 10 adds the deny-side fail-open clause to the body).ebf211c35after the lesson: fork-policy 309 files, +58,382/−14,644 (net 43,738 ≈ 29.16×; patience +47,047/−3,309, nets equal); This-push header and topic count corrected; Test record refreshed (core 125 / execpolicy 127 — the round-38 record had missed its own wave's pin — / app-server 111; tui 12,148 + the environmental band; drift 102 files +13,500/−793; 16 topic commits).POST /v1/threads(only an EMPTY workspace is refused — the collapsed shape is not legacy-only), and the first-entry demotion is documented for the present-workspace case too.SanitizedLoadedRoots, all 15 packs, 2,098 keys,{count}parity machine-checked): no folded-source space runs, an actionable remedy (re-attach through/workspace; the next autosave persists the reduced set), and a receipt pin (count substituted, clean loads stay silent)./v1/sessionsarming poison refusal answers 400 like the resume face (the provider-resolver reason family stays ledgered: pre-existing at base). Minors 7+8 — the two display-only filters and the core move arm route through the shared strip. Minor 9 — the body's/cdspellings corrected to/workspace(ten sites). Minor 11 — the pin wave: sanitize's canonical leg (unix symlink fixture), the empty-workspace 400 (HTTP harness), and the autosave belt's non-identity case. Minor 12 —ATTACHED_ROOTS_NOT_REVERTED_NOTEledgered as an explicit EN-surface exception on the pre-existing receipt surface.Round-39 P2 correction (head
5235ceb9a)The parent-side review (JensenChen28, on codewhale-hq#484 @
29d991a0a) reproduced a real regression in the B39-2 boundary heuristic with real Linux directories::and\are legal Unix filename characters, so treating them as separators madevalidate_workspace_rootsREJECT andsanitize_workspace_rootsDROP legal sibling attachments (data:vsdata:archive/ws;datavs a sibling literally nameddata\archive) — three variants, core functions, real dirs. Fixed by platform-splitting the ancestor predicate: the NATIVEPathcomponent comparison on Unix (canonicalize produces no verbatim prefix there — the mixed-spelling hazard the heuristic addresses does not exist), the spelling-normalized comparison confined to Windows where it is real. Pins: the colon/backslash sibling fixtures on Unix (both guards, both directories), the boundary assertions moved undercfg(windows). All figures above re-measured at this head.Round-40 disposition (head
0a4b4b662)case_insensitive(true)on every host. The law's contract is that it may only ADD holds, so folding case universally is the contract-safe direction (it can over-hold two case-distinct directories on a case-sensitive host; it can never bypass a block) and it sidesteps host-level FS detection entirely. Pin:wrong_cased_create_shaped_tail_hits_a_block_glob— your exact shape (vendor/**block,vendor/exists, write toVENDOR/new/x.rs) is held. Unicode NFC/NFD divergence stays a disclosed residual (case-folding does not normalize).backup/pre-r40-reword-20261012): the perf commit's correction no longer names the unresolvable pre-squash SHA — it points at the carrier by position and probe (git log -S 'static candidate slice'), and the residue-wave commit now names its own execpolicy hoist hunk and thetyped_ask_absolute_path_rule_matches_a_call_outside_every_rootpin.create_session_from_thread) validates the cloned set; the threadOp::SyncSessionships the same validated, head-stripped set the engine armed with; the approval card skips empty-string operands like Null; the sub-agent write collector is alias-aware (path_param_value) like the session side; ACPsession/loadrefuses a relative workspace likesession/newand the REST lane; the placeholder-parity gate covers theFork*/Save*/Sanitized*prefixes; zh-Hans/Hant full-width punctuation and the Catalan elision in the new keys; the model-facingAccessible folders:line escapes the list separator; and theSessionUpdatedid-divergence path clears the grant sets when the workspace is unchanged./permissionssubtree under-report (display); the seatbelt ×2get_writable_rootsenumeration per exec (~12–15 ms/exec at the 64-root cap — the round-40 minor 9 ledger line, alongside the disclosed constitution-cache cluster); the--continuerelative-workspace legacy niche (self-heals on next persist); and the stdio-32004envelope qualification.0a4b4b662.Round-41 disposition (head
707962849)resolve_resume_roots/ACP/session-new) — forwarding it used to collapse the declared set and mint a relative-primary thread nothing else loads. The ACP guard comment and the round-40 disposition sentence that claimed the REST lane already refused this row are corrected. Pin: the relative spelling 400s beside the empty one on the HTTP harness.strip_workspace_roots_headcanonicalizes both sides (lexical fallback; the strip only runs on cold move/load faces), so a symlink alias of the OLD primary — admitted at intake because the ancestor rejection exempts equality — now drops with the head on every move face. Pin: the symlink alias drops and the re-validated set on the new primary never re-arms the abandoned directory.\\localhost\C$\…vs aC:\…primary) is named in RUNTIME_API with its exploitability tempering (share of the ancestor;C$needs admin) and the execpolicy POSIX-rooted note; the ASCII-only case-fold precision rides the same paragraph (law contract keeps the direction safe).de eluncontractions. Now: the four zh-Hans/Hant refusal keys punctuate full-width (byte-verified), Catalan contracts todel workspaceeverywhere with theL'workspace/d'workspaceresidue cleared (7 keys), and the fr/uk/ru/hi/vi grammar defects in the new family are repaired. The round-40 commit message's zh credit is corrected by this record (its diff contained only the Catalan half).--jsonresume warning regains its output-format gate; recorded here), 14-partial (the Op::SyncSession doc says full cwd-first form; the model-facing folders line uses the shared strip), plus the body-truth corrections (the stale "not canonicalized" headline, the--workspaceabsolutize qualification forexec/serve --mcp, the canonical-judgment reach, the register/head three-way inconsistency, and the test-record basis moved to this head with the band-composition caveat).resolve_deepest_existingmirror alignment, the restore-face partial, the engine-level pin for the raw-spelling comment), and 15 (the perf ledger: seatbelt ×2 base-identical but +2 realpath chains per root per exec on macOS from this PR's canonicalization, collapsible to one enumeration + shared canonical map — schedule).Round-42 disposition (head
52f2de016)52f2de016(header, counts, fork-policy, test-record basis — see the sections above); the M41-6 wording corrected to the single realizable shape; theexec --resumesentence corrected to validate-and-refuse (the round-41 minor-12 drop is owned here); the parent-register sentence now names the API-measured gitlink (this body's register paragraph retires with the re-pin PR anyway, but the sentence is true at this head)./var↔/private/vardivergence dies for mixed-existence pairs (your deterministic red pin goes green; both legs now mirror the same normalize-then-compare discipline), and the platform doc states the fallback semantics. The colon/backslash sibling classes stay distinct (component comparison)./workspacehas no attach arm; the pin updated to forbid the nonexistent producer), 6 (the M41-1 pin asserts nothing was persisted), 7 (the export-mint degrade pinned at the HTTP seam with store-level poison), 8 (the PUT mint arms run the sanitize belt), 9 (the TUI restore lane refuses relative primaries like its siblings; fixtures pinned to absolute workspaces), 12 (the AUTHORIZATION_ORDER lead qualified — the Allow arm is exact; the UNC analogy dropped), 13 (the stale "lexical" comment sweep and the exec gate-comment contradiction), and the disclosure corrections (minor 4's both-policies wording and pins-claim qualification ride the body).\\.\device note), and 15 (the parent-repo fork-posture registration — due with the re-pin PR per its own note).Round-44 disposition (historical basis head
d94bdb5d7; superseded by the round-45/46 record above)/var/folders/…/T/newdir+/TPOST/PATCH shape now rejects. Red/green-pinned on Linux with the symlink-alias-ancestor-under-ghost-primary shape (your revert-control premise holds for the OLD code: the pre-fix predicate was inert); the predicate doc and RUNTIME_API's canonicalization sentence are corrected alongside, and the M42-4 "goes green" bullet is corrected BY THIS RECORD (the round-43-fix wave did not address the round-43 R1 — that omission is owned here).normalize_path_lexically— execution's own containment identity), soghost/../attlands on the resolved root's law while a genuinely nonexistent root still judges raw spellings (fail-closed preserved). Pinned with the folded spelling through the realrepo_law_plan_decision.d94bdb5d7(drift 101 files +14,447/−889; fork-policy 350 files, +60,356/−9,998 ≈ 33.57× — the understated 13% corrected); protocol 71; core 127. The CI-coverage truth replaces the B39-2 sentence: this PR's checks are fork-ci on ubuntu only —ci.yml(the sole 3-OS matrix) triggers on master/main, whichpinvou3-cleanis not, so thecfg(windows)pins compile nowhere in this PR's automation and the macOS suite does not run; the two formerly-red pins are green by construction/walk-fix, and platform validation lands with the merge-target's own CI when feat(fork): multi-root workspace_roots authorization infrastructure (runtime lanes) on v0.9.12 #54 merges./cdreferences), 9 (terminology — вилка→форк etc. rides the next locale sweep), 10 (the gitignore mirror gap), 11 (the raw-newline description — weakened pin noted), 12/13 (the state-layer round-trip; the mutex-across-parse), 14 (the staging transposition — in-tree confession already self-corrects).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 codewhale-hq#549. (codewhale-hq#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, codewhale-hq#484's gitlink keeps pinning this branch's head (the re-pin commit riding codewhale-hq#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 and at the round-38 head as ~95 lines across 27 non-test files (comments and RUNTIME_API.md prose only — verified no model-facing meta-text anywhere), 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)./workspacevalidates 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.