Skip to content

feat(fork): multi-root workspace_roots authorization infrastructure (runtime lanes) on v0.9.12 - #54

Merged
asto18089 merged 28 commits into
Pinvou:pinvou3-cleanfrom
qiuYliangM:pinvou3/workspace-roots-v12
Oct 14, 2026
Merged

asto18089 merged 28 commits into
Pinvou:pinvou3-cleanfrom
qiuYliangM:pinvou3/workspace-roots-v12

Conversation

@qiuYliangM

@qiuYliangM qiuYliangM commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

History compression (2026-09-30, disclosed). Per the author's decision the branch was squashed from the 56-commit reviewed topic line into the single commit 4095834e ("feat(workspace): squash the workspace_roots topic onto the r3 closure"), parented on the r3 closure 61cb769be. The tree is byte-identical to the round-23-reviewed head 5063be0e8 (git diff 5063be0e8 4095834e is empty) — no content change ships with this push. Pre-squash SHAs cited in the dispositions below resolve on the fork backup branch backup/pre-squash-54-20260930 and in this PR's timeline — with EIGHT exceptions that resolve only as fork-remote objects (API-resolvable, none git-fetchable), not in the backup branch's 56-commit ancestry: d872c6663, e01e3a7b4, 8ba62fd7e, ad466982a, 0c366b9ed, 9f5f4d34a, 83bf482e3, bbc90540a (round-26 M26-4c said five; round-28 body re-check added the last three). The backup branch itself lives on the FORK remote (qiuYliangM/CodeWhale), not origin; the dispositions are unchanged historical records. Round-33/37 correction (M37-5): the "byte-identical to 5063be0" claim was superseded when the line was re-derived against the public base 0bc868ff0 (the round-33 rebase moved 68 files, +7,765/−654, at re-squash time); the corrected wording ships in the commit message itself after the round-37 reword (tree unchanged by the reword — verified byte-identical). Round-24 update (2026-09-30, same day): the branch carries five fix commits on top of the compression — head dee19f9a1 (the round-24 disposition names each). Round-25 update (same day): two further commits close the recorded P3 deferral ledger — head 8419d0132 (b227aa043 accuracy/residue wave, 8419d0132 perf wave; the round-25 disposition names every item, including the four that stay recorded). Round-26 update (same day): one further commit closes B26-1 (the round-25 boundary-memo isolation regression) + M26-1/2/3 with their pins and the M26-4 prose corrections — head eec20a2d8. Round-28 update (2026-10-02): one further commit closes the round-27/28 blocking set — B27-1/B27-2 (the round-26 seed shape corrected back to pure-load-under-record-primary), B27-3+A28-3 (the apply_patch grouping key re-keyed onto the real parser's plan), A28-1 (relative-cwd rejection on resume/fork/start), A28-2 (ACP session/load validates the persisted set), A28-4/M27-1 (both macOS fixture bugs), M27-2/3/4 — head 9e8912329; the round-28 disposition names each. the parent codewhale-hq#484 re-pins its gitlink to eec20a2d8 and re-derives its fork-guard register (EXPECTED_COMMITS 57 → 58) in the same push; the disclosed verify-public-submodule transition red is unchanged. Round-26 M26-4a disclosure: the execpolicy matching_ask_rule static-slice change (and its absolute-fallback pin) landed in b227aa043 while 8419d0132's message claims it — a staging transposition, both messages' other claims are accurate.

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 /thread face 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 ACP session/new passes no roots — and the default-path tools (grep_files/list_dir/file_search) resolve against the primary, so a model that reads the Accessible 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 head f27672935 (#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 at 9e234030e)

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)

  • R45-1 the red required pin → asserts the localized boundary note on an explicit 8 MiB stack (P2-8 folded). R45-2 hi.json deduped byte-level + a real segment-level no-duplication gate over all 15 packs (red-first developed; 6-word-window skeleton matching). R45-3 SessionRestoreWorkspaceInvalid in all 15 packs; /restore rides the host-localized note through a new contract facet whose default is the stable English const (single-sourced in command-contract, re-exported from crate::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 _dropped rebind. 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)

  • [Round-47 update] The compile-broken window is now closed AT THE SOURCE. The round-47 rebase onto 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-commit cargo check --workspace --all-targets pass 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 at 4755bac88/2b1704ca0/9e234030e, where the window was still live; it stands as the historical record of what shipped to review.
  • A 25-commit compile-broken window, repaired at the tip (historical, pre-round-47). The round-45 rebase replayed the line onto 480bebf61 as the machine carrier acdaf8d2c; its replay of the maintenance batch mangled crates/tui/src/core/engine/tests.rs in exactly two places — (1) the toolbox_note_survives_a_batch_without_results assert! left unclosed, which also swallowed the Round-20 B20-1 test into the macro (parse error), and (2) the Op::SyncSession literal in session_sync_drops_a_parked_toolbox_eviction_note missing this topic's own workspace_roots: Vec::new(), (E0063; the replay updated 14 of the file's 15 literals). Every commit acdaf8d2c..59344db94 therefore fails cargo check --workspace --all-targets at its own tree; the tip 4755bac88 repairs 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-ci check+gate green).
  • The carrier's message describes a re-derivation that is not the one that happened. acdaf8d2c still tells the round-33 story ("re-derived against the public base 0bc868ff0 … moved 68 files (+7,765/−654)") while its actual tree is the old carrier plus the 155-file, +6,212/−10,969 maintenance replay of 3118c36b8→480bebf61 (the manufactured 2026-09-30 author date the message sits on pre-dates 0bc868ff0). 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.
  • Figures re-measured at 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 upstream dcd4c200f 445 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

  • P2-1 — 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/../target the 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.
  • P2-2 — the validate_workspace_roots doc's stale ghost-primary "existence-dependent residual" claim corrected (R44-1 closed it; the pin is named).
  • P2-4 — the round-44 dedupe had glued sentence boundaries in ru/vi (.Перезагрузите / .Tải lại, 4 keys); spacing restored byte-level.
  • P2-3, ledgered per the round-39 minor-12 EN-surface precedent — /undo's primary_only transcript 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.
  • Segment-gate limits, recorded where the gate is claimed — the no-duplication gate is Latin/Devanagari-strong and CJK-coarse: unpunctuated ja/zh clauses tokenize as single words and curly quotes/・/「」 are outside the delimiter set, so a pasted CJK clause can still slip (P2-12).
  • Round-47 rider (item 1, P2-5) — the PATCH active-turn fence's message now carries the already has an active turn conflict substring, so map_thread_err maps it 409 like the four sibling turn-start guards (an IDE client no longer sees a retryable-mid-turn PATCH as a permanent 400).
  • Process disclosure — the round-46 gh pr edit that 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 /cd should-fixes:

  • The cwd slot is validated on every intake lane, and the chokepoint fails closed. normalize_workspace_roots returns 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, since normalize_path(".") is the empty path). Empty cwd/workspace is rejected at create_thread, app-server thread/start+resume+fork (resolve_resume_roots is Result now), ACP session/new+session/load, the TUI session-restore funnel, CLI resolve_workspace, and standalone run_mcp_server. Behavior fix that fell out: relative --workspace values (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.)
  • Repo-law overshoot clamps exactly like execution — by construction. normalize_lexical_components now delegates to tools::spec::normalize_path (execution's own normalizer), so an overshoot spelling like /w/x/../../../att/vendor/lib.rs judges the clamped landing path and is held by the attached root's vendor/** 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).
  • A worktree child's exec lane is confined to the worktree. Both clear sites (spawn + resume) re-derive WorkspaceWrite.writable_roots from the cleared set (rederive_sandbox_policy_roots, same idiom as workspace_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.
  • Should-fixes: the /cd persistence receipt is typed (Degraded → Warning toast, Failure → sticky Error — the "persisted, actor unavailable" arm no longer screams failure); the /workspace blocked message is localized (WorkspaceSwitchBusy × 15 packs, placeholder-free) and names no request, mirroring /clear.
  • Body corrections (this edit): the fork-policy sentence (registration lands with the parent re-pin PR), the worktree disclosure (exec lane included), the intake and repo-law sentences, and the single-root-visible list.

Round-16 remediation (ad466982a). The round-16 pass reviewed 9f5f4d34a, 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:

  • Approval grants re-key on the exec cwd operand (the cache-key half of round-16 should-fix 4; the rule-evaluation half was round 15's B15-3): the shell grouping key is now shell:<prefix>@cwd:<operand> when the call carries cwd:/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).
  • The ancestor-root repo-law pin (relative_target_landing_under_an_ancestor_root_is_held): session cwd inside an attached root, a relative target lands inside that root, whose sub/** glob must hold the landing path. Red when the candidate judgment is re-narrowed to absolute spellings.
  • Contract comments corrected: the parity test no longer claims single-root threads omit 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):

  • B15-1 — repo law judges both spellings of every root, plus the execution candidate. repo_law_plan_decision canonicalizes each root once with raw fallback (the boundary_roots() idiom — a non-resolving root keeps its constitution); push_normalized dual-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_existing strips the resolved path against the canonical root exactly as carve_out_target_allowed does. 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.
  • B15-2 — PUT /v1/sessions persists a paired workspace + root set. The update branch stamps metadata.workspace from the same snapshot beside the roots and re-normalizes (the /fork idiom), 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).
  • B15-3 — the exec ask-rule path judges the resolved effective cwd. The cwd:/working_dir: operand is resolved exactly as the execution lane joins it and fed as ExecPolicyContext.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 with PathEscape); a grant scoped to the attached root fires exactly where execution lands. Disclosed narrowing: the round-15 judgment was lexical — a symlinked link/.. 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 a cwd: operand pointing at a subdirectory of the primary (fail-closed; no-operand path byte-identical). Pin red without the fix (Some(Allow) vs None).
  • M15-2 — intake validation at the single chokepoint. normalize_workspace_roots drops empty/relative roots entries: an empty-string root made Path::starts_with("") true for every path, nulling resolve_path containment for reads under approval Auto in every posture. The state reader warns through its existing channel when a persisted row holds such an entry. Three pins, red without the filter.
  • M15-3 — rollback surfaces name their boundary. Snapshot/restore stays primary-bound; 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.
  • Should-fix batch. The cached-resume override writeback uses a targeted 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 covers Workspace* MessageIds (synthetic-pack pin; all 15 packs clean); the /cd failure receipt promotes as a typed sticky Error in any locale; /cd guards on session_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 PATCH Some([]) 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).
  • B15-4 — examined and refuted with evidence (full transcript in the round-15 reply): 0c366b9ed and 83bf482e3 are siblings both parented at bbc90540a, 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_out judges 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's cwd: 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 symlinked link/.. 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: null spellings 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-targets green; cargo fmt --all --check clean; 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.
  • Suites (lib targets, cargo test -p <crate> --lib at 4755bac88, 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.)
  • Full codewhale-tui --lib under LC_ALL=en_US.UTF-8 RUST_MIN_STACK=16777216 on Linux arm64: 12,164 passed at 52f2de016 (the remote_control trio under the parallel run passes 83/83 single-threaded; composition varies within the band — the pair vs trio shuffles with no_new_privs, see round-41 minor 10) — the two are the documented environmental band (remote_control scheduling; no_new_privs fails 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 whole remote_control suite passes single-threaded here (83/83), including both flakes.
  • New round-37 pins, red-green verified: the core stored-row primitive (at-cap round trip / dot-dot head strip / poison refusal / cwd-move lexical strip), the fork+arming at-cap round trip, the ACP full-load load, the resume-thread full-load resume, the bridge wire strip (unit + both wire-level harness tests re-anchored), the exec resume anchor trio, the approval-card working_dir preview, the /save + plain-/fork refusal, the intake symlink-ancestor pair, the /-canonical root drop, the scoped-deny subtree pair, the grouping-key root-binding trio, and the workspace-move/conversation-change clear.
  • Environment notes: the known default-stack overflows need RUST_MIN_STACK=16777216; locale-sensitive lanes pass under LC_ALL=C. Linux CI remains the green authority.
  • Counts at be2cd01ef: 28 topic commits over the public maintenance head f27672935; drift vs the maintenance head: 100 files, +14,904/−945; every commit authored by qiuYliangM with exactly one matching DCO sign-off (the replay carrier acdaf8d2c carries 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/PATCH live-session 409 Conflict promised 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: NewTaskRequest carries no workspace_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 a NewTaskRequest shape 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 explicit cwd: 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_snapshot opens the SnapshotRepo on the primary root only; carve-out-approved writes under a git attached root leave nothing app-level to restore and /undo will 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 IMMEDIATE and rolls back on any in-transaction failure.

  • import_foreign silently 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 (~/shared is refused with an error string — HTTP 400 on HTTP lanes, a JSON-RPC internal error 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::canonicalize returns \\?\-prefixed verbatim paths that scope matching cannot produce, so at this head every cwd:-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 RuntimeThreadHint roots 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/threads and the stdio thread/start equivalent (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_analyze resolves against the primary only and rejects absolute paths (a shipped tool that cannot see attached-root images while turn_meta advertises them); read_lints and project_map are primary-rooted; the default-rooted search/read tools (grep_files/list_dir default path=".", file_search defaults 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_id is 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 workflow source_path gate, subagent resident_file, and subagent cwd: validation are primary-only; @-mention/completion resolution never traverses attached roots. ACP sessions enforce the full set and, since round-41 M41-1, session/load REFUSES poisoned/relative rows with the intake reason instead of tolerating-and-arming (acp_server.rs, pins alongside); the turn_meta line and session_configuration still 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_field no longer trims its extracted value (round 13) — load-bearing for the raw-spelling alignment, a cwd: " /repo " operand now fails resolution instead of being silently trimmed; an exact-scoped allow rule no longer fires when an exec cwd: operand points at a subdirectory of the primary (round 15, fail-closed); the cached-resume cwd override now persists the row and bumps updated_at (base did neither); a missing-thread resume/fork answers HTTP 404 (was 200 + status:"missing") and stdio -32004 (was success); /workspace persists synchronously and composes persistence receipts into the closing status; a claim-less worktree-child resume flips isolated_worktree false→true via the launch-manifest fallback; the string_field trim 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 --workspace values are absolutized instead of silently disabling containment (the exec/serve --mcp subcommands 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 /workspace blocked message moved from a hardcoded English string to the localized WorkspaceSwitchBusy, with the degraded-success receipt downgraded from sticky Error to Warning. Round-38 M38-5 additions: the apply_patch plan-time gates fold PATH_ALIASES exactly 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 source session_id/task_id (M27-2 — a forked thread no longer replays the source session file into the model context; wire-shape change), get_writable_roots canonicalizes every configured writable root (base mounted verbatim — observable where spellings diverge), and ACP session/new rejects 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 dcd4c200f at this head be2cd01ef (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::SyncSession and the persistence stamp carry the SAME validated, saved-primary-anchored set — the CLI --workspace never 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 in exec_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 reuses validate_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_roots drops 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_matches owns the scope SUBTREE: a scoped deny (and ask) fires for a call judged anywhere inside its scope — the fail-open for /repo/sub vs scope /repo is closed (pin included, outside-scope negative included).

M37-4 closed. The in-app plain /fork and /save stamp 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_dir operand 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::SyncSession arm). 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 — a working_dir redirect is visible on the card that approves the grant keyed by it (pin included).

M37-1 closed. The revert_turn model-facing description no longer carries review meta-text; the pin now asserts the negative.

M37-8 closed. The /cd and /fork <id> widening refusals ship in all 15 locale packs (machine-checked key/placeholder parity, 2,100 keys each); a SyncSession that 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 main at 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 in docs/RUNTIME_API.md. The four false 404 faces, the falsified POST /tool claim, the stale string_field reference, 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: the matching_ask_rule clause re-attributed to b90c434ae inline). The rebuilt line's tree is byte-identical to the pre-reword line (verified); the pre-reword line is public on backup/pre-r37-reword-20261011.

Minors taken in the same wave: the ..-spelled head bypass (lexical strip everywhere, pins), the resolve_resume_roots relative-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, the ask_rules path-alias leg, the non-UTF-8 serialization fallback, the PATCH-during-turn 400/409 wording, the turn_loop rfind pin scope, the notice saturating_sub/.replace-chain display items, POST /v1/threads all-400 mapping, and the B27 face-level wiring pins.

Round-38 disposition

  • M38-1 — allow arm restored to EXACT matching (the round-37 subtree matcher widened scoped ALLOW eligibility; the standing body disclosures promised the opposite). workspace_scope_matches_exact gates 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 by workspace_scoped_allow_stays_exact_for_subdirectory_operands (fires at the scope root, does NOT fire one directory down).
  • M38-2 — the seed/persist lanes close at the root cause. New core tolerant companion 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_roots uses 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 via note_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.
  • M38-3 — the ..-spelled head closure is now real. New core primitive strip_workspace_roots_head (the lexical strip, one implementation); validate_stored_workspace_roots delegates 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.
  • M38-4/M38-5 — the fork-policy figure refreshed at the current head (309 files, +57,678/−14,710, net 42,968 ≈ 28.65×) and the two single-root-visible behavior records added (apply_patch plan-time alias folding; the carve-out's untrimmed-spelling branch), both above.
  • Minors closed in the wave: the resume-thread empty-workspace 400 mapping, the RUNTIME_API relative-workspace serialization sentence, the intake rustdoc contract header, the approval card's 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.
  • Minors to the ledger (disclosed, non-blocking): the HTTP-level 404/over-cap/roots-class pins (mappings verified by reading; the resume-thread harness exists for the at-cap pin already shipped), the grant-clear conversation-change leg's dedicated pin (the workspace-move half is pinned; the arm is three lines beside it), the perf cost notes as measured (canonicalize ~2× per sandboxed exec, constitution-cache cluster — exactly as disclosed), the resolve_deepest_existing consolidation, 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)

  • B39-1 — the sixth conversion face closed. core fork_thread's None-inherit arm routes through strip_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.
  • B39-2 — the Windows mixed-spelling fail-open closed with the repo's own precedent shape. Both guards (intake + sanitizer) compare through a comparison_spelling normalizer: verbatim \\?\ stripped, \\?\UNC\ mapped, case-folded (Windows only; Unix passthrough — case stays significant), plus a separator-boundary comparison_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).
  • B39-3 — every head-specific figure re-measured AT the pushed head ebf211c35 after 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).
  • B39-4 — both RUNTIME_API sentences fixed: a fresh relative-workspace row IS mintable through 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.
  • Minor 1 (mandated with the wave) — the strip-and-warn receipt is a real localized message (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).
  • Minors 2+3 — the sanitize drop count is now the difference between stored candidates and what the tolerant load actually arms, covering shape-normalizer drops, cap truncation, and poison in one number (pins for the relative entry and the 70→65 truncation). Minor 4 — the PUT /v1/sessions arming 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 /cd spellings 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_NOTE ledgered 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 made validate_workspace_roots REJECT and sanitize_workspace_roots DROP legal sibling attachments (data: vs data:archive/ws; data vs a sibling literally named data\archive) — three variants, core functions, real dirs. Fixed by platform-splitting the ancestor predicate: the NATIVE Path component 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 under cfg(windows). All figures above re-measured at this head.

Round-40 disposition (head 0a4b4b662)

  • M1 — FIXED, not scheduled: the constitution repo-law globs compile 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 to VENDOR/new/x.rs) is held. Unicode NFC/NFD divergence stays a disclosed residual (case-folding does not normalize).
  • M2 — both messages reworded, tree byte-identical (pre-reword line public on 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 the typed_ask_absolute_path_rule_matches_a_call_outside_every_root pin.
  • Minors closed: the minting lane (create_session_from_thread) validates the cloned set; the thread Op::SyncSession 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 grant sets when the workspace is unchanged.
  • Minors ledgered: the fresh-spawn worktree clear-site harness test (the capture hook exists; disclosed); the /permissions subtree under-report (display); the seatbelt ×2 get_writable_roots enumeration 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 --continue relative-workspace legacy niche (self-heals on next persist); and the stdio -32004 envelope qualification.
  • Body truth: the test-record suite counts and This-push header had silently missed their round-38/39 edits (unmatched replace patterns — the exact failure mode you flagged); every figure in this body is re-measured and post-verified at 0a4b4b662.

Round-41 disposition (head 707962849)

  • M41-1 — fixed, not minimized: the REST resume-thread face refuses the RELATIVE session workspace with a 400 (one guard, matching 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.
  • M41-2 — the strip gains a canonical leg: strip_workspace_roots_head canonicalizes 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.
  • M41-3 — disclosed per the round-21 B21-4 precedent: the Windows SMB/UNC alias class (\\localhost\C$\… vs a C:\… 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).
  • M41-4 — owned and actually done this time: the round-40 "zh punctuation + Catalan elision" closure was FALSE — the zh edit was a same-character no-op script bug and the Catalan fix introduced de el uncontractions. Now: the four zh-Hans/Hant refusal keys punctuate full-width (byte-verified), Catalan contracts to del workspace everywhere with the L'workspace/d'workspace residue 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).
  • M41-5 — the three AUTHORIZATION_ORDER sentences fixed: the scoped-allow claim qualified (an allow scoped to an ATTACHED ROOT auto-approves calls judged there, including cwd redirects; the primary-scoped narrowing holds), scope matching described as subtree containment, and the ask/deny reach corrected (roots inside the scope; UNSCOPED rules span all).
  • M41-6 — the hollow pin reworked onto the mixed-existence shapes the normalizer actually rescues (existing primary + not-yet-materialized ancestor, and the inverse; the both-existing fixture passed with the fix reverted), plus the sanitize leg's first Windows shape.
  • Minors closed: 1 (the export mint degrades to the SANITIZED remainder with a warn — a refused set keeps its legal additions instead of collapsing to bare-cwd), 2 (the PUT re-stamp runs the sanitize belt like the TUI autosave), 9 (the exec --json resume 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 --workspace absolutize qualification for exec/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).
  • Minors ledgered: 7 (the named pin gaps), 8 (the commit-message zh credit — corrected by this record rather than a third history reword), 13 (dedup'd duplicates naming as dropped — hand-edited rows only), 14-rest (the resolve_deepest_existing mirror 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)

  • M42-1 — completed for real, and the dropped ja item accounted: vi's missing "ngoài" and hi's garbled tail fixed, ru's new-family terminology unified to «рабочего пространства», and ja's four refusal keys punctuate full-width (codepoint-verified) — ja was silently dropped from the round-41 closed AND ledgered lists; it is closed here. The round-41 disposition's "fr/uk/ru/hi/vi repaired" claim is corrected by this record (fr gender and uk agreement were real; vi/hi/ru were partial; ja was absent).
  • M42-2 — record corrected, no third reword: the r37 reword's line tip carried the +12/−9 B37-3 wire-harness re-anchoring, so the backup branch differs from the line by exactly that test delta — the body's "tree byte-identical (verified)" is corrected to name the delta and its cause (the post-reword clippy/fmt amends); the r40 reword remains the genuinely byte-identical control. The verification path (compare against the backup) now agrees with the record.
  • M42-3 — everything re-measured at 52f2de016 (header, counts, fork-policy, test-record basis — see the sections above); the M41-6 wording corrected to the single realizable shape; the exec --resume sentence 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).
  • M42-4 — the preferred fix: the Unix ancestor leg canonicalizes both sides with lexical fallback before the native component comparison — the macOS /var ↔ /private/var divergence 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).
  • Minors closed: 1 (the per-turn canonical cost ledgered; the strip's "cold faces only" claims and the engine.rs comment corrected), 2 (the constitution cache single-reads and inserts only on Ok — the probe/parse race that could cache EMPTY rules), 3 (the remedy copy names the reload path in all 15 packs — /workspace has 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).
  • Minors ledgered: 5 (the grant re-key record correction — in-memory sets, nothing persisted re-keys), 10 (the model-requested escalation arm's default policy), 11 (the RUNTIME_API restore-shape PARTIAL), 14 (the Windows generic existence-dependent axes; the \\.\ 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)

  • R44-1 — the ghost-primary hole CLOSED, not reverted: the Unix guard resolves the primary through its deepest EXISTING ancestor (the resolve walk you prescribed) before the component comparison, so both sides carry canonical spellings for every component that exists — the macOS /var/folders/…/T/newdir + /T POST/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).
  • R44-2 — the wave's own three defects fixed: the alias-target pin canonicalizes its expected prefix through the same walk (the mixed-spelling class, green on both platforms by construction now); the doubled remedy sentences in hi/ja/zh-Hans/zh-Hant deduplicated with a segment-level no-duplication verification across all 15 packs; and the "script-aware verification" claim is RETIRED — the localization delta was one MessageId and no structural gate exists (the dedupe this round is the mechanical proof of why that claim was false).
  • R44-3 — the folded-spelling constitution bypass CLOSED: repo law keys discovery and all four target strips on the lexically RESOLVED spelling (normalize_path_lexically — execution's own containment identity), so ghost/../att lands 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 real repo_law_plan_decision.
  • R44-4 — everything re-measured at 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, which pinvou3-clean is not, so the cfg(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.
  • Minors: taken — the macOS-red class (both pins), the dedupe, the R44-1/R44-3 doc comments. Ledgered with precision: 1 (scoped ask/deny membership semantics — pin name/wording), 2 (the deny-face disclosure overbreadth), 3/4/5/6 (the latent fail-closed lanes), 7 (the empty-cwd preview/exec divergence), 8 (the 28 /cd references), 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 (faa1190b8 at 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 gitlink faa1190b8, EXPECTED_COMMITS 101, drift 97 files +8,824/−553); verify-public-submodule stays 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.

  • B19-1 (blocker, padded session-id bypass of the live-session 409): the live-session registry judges the trimmed id on every lane — is_live_session trims the query and fails closed on a poisoned lock (ownership unknown = live = conflict), set_live_session recovers a poisoned write lock instead of dropping the claim, and the API boundary normalizes once: the PUT /v1/sessions body id and the PATCH /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.
  • B19-2 (super-root intake): new codewhale-core::validate_workspace_roots gives 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.
  • B19-3 (behavior breaks unrecorded): CHANGELOG [Unreleased] entries for the missing-thread resume/fork 404 / -32004, the live-session 409, intake validation, relative --workspace absolutization, worktree exec-lane confinement, and the string_field trim removal; a RUNTIME_API.md paragraph documents the 404 and the intake rules.
  • B19-4 (false ~ 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 --check clean.

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:

  • Fixed (P2): the resume lane that moves the primary (cwd without workspace_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. 18c50a3f6 validates 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.
  • Disclosed (P3, decisions/post-merge): a filesystem-root primary (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 to JsonRpcError::internal rather than a typed invalid_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-workspace 200-OK, explicit-null deserialization 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)

  • B20-1 (P0, exec judged-cwd lexical vs canonical execution): the ask-rule judgment canonicalizes the cwd:/working_dir operand the way execution resolves it (canonicalize() on the joined path, lexical fallback for nonexistent operands matching resolve_nonexistent_path) — the link/.. 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.
  • B20-2 (grant re-key bypass): shell_cwd_operand skips null/non-string values like the check and execution sides (cwd: null + working_dir now 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).
  • B20-3 (the live-session guard) — decision: removed, per the review's sanctioned option (b): the process-local registry cannot coexist with the runtime HTTP server in any shipped topology, so the PUT/PATCH 409 could never engage. The HTTP-lane guards and their four route pins are removed; the registry itself is retained (same-process retention pruning consults it — that consumer is real) and its round-19 trim/fail-closed pins stay meaningful for that consumer. The breaking-change framing is retracted from the body, CHANGELOG, and RUNTIME_API.md. The B20-3 residuals (case-folding, DELETE/retention live-awareness, claim clearing) do not apply to the retained consumer's same-process scope and are recorded as post-merge work.
  • B20-4 (lexical intake): the minimum acceptable — every rejection-class description (body, RUNTIME_API.md, CHANGELOG, the validator's own doc) now qualifies the checks as lexical, with canonicalize-at-intake named as the scheduled promotion; the validator doc states the two working defeats explicitly (realpath ancestor, child-spelled link).
  • B20-5 (body/CHANGELOG): the three safety sentences corrected, "typed error" ×3 re-qualified (error strings; stdio maps them to internal), the producer claim corrected (stdio thread/start included), 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, /cd receipt severity + guard widening, cached-resume updated_at, isolated_worktree flip, string_field narrowing).
  • Should-fix 1: /workspace validates the re-based set through validate_workspace_roots before 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 headless exec --resume lane 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.
  • Should-fix 2 (round-20), recorded: the resume-lane worktree child keeps the parent's plugin registry — the resume clear site does not re-scope, so workspace-scoped plugins and their authority receipts cross into the isolated child; one-line fix at the existing clear site, post-merge.
  • Should-fix 3 (round-20), corrected: the worktree fail-open disclosure names only InterruptedOrCompleted, but is_terminal also includes Interrupted — both policies are equally exposed; the correct disclosure is "the policies both end-to-end pins drive (InterruptedOnly and InterruptedOrCompleted)".
  • Should-fix 4 (round-20), partially closed here: POST /v1/snapshots/{id}/restore is the one rollback face whose response carried no boundary clause — the clause is added with the round-21 commit; the runtime patch-undo note remains unpinned (disclosed).
  • Should-fix 5–9 (round-20), recorded for post-merge: relative-primary collapse qualification (5) and the fs-root primary asymmetry (6) are already in the known-scope section; the engine-build re-arm warn (7); ACP session/load of 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/−553 over the r3 closure (fork surface ≈21.46× at faa1190b8).

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 --check clean; two session_state locale-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 reviewed faa1190b8)

  • B21-1: the Known-scope 409 bullet is rewritten to the retraction (the earlier revision still promised the removed guard — caught).
  • B21-2: the should-fix ledger is completed — SF-2 (resume-lane plugin-registry re-scope), SF-7 (engine-build re-arm warn) and SF-8 (ACP session/load legacy-row re-declare unreachable) are now recorded; SF-3's disclosure corrected to both policies (is_terminal includes Interrupted); 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.
  • B21-3: the canonicalize-failure fallback no longer keeps the lexical join — it walks lexically to the deepest existing ancestor (trailing .. included), canonicalizes it through any symlink, re-appends the tail and normalizes, mirroring resolve_nonexistent_path (the first cut used resolve_deepest_existing, whose Option returns 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.
  • B21-4: the two canonicalize-at-judgment regressions (Windows \\?\ verbatim spellings never matching scopes; declared-vs-canonical scope spellings silencing persisted scoped allows) are disclosed in the body and CHANGELOG per the review's minimum — 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).
  • B21-5: figures refreshed to 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).
  • B21-6: the headless exec --resume moved-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.
  • P3 batch taken: the stale guard-era comment sweep, the phantom architecture-guard citation removed, resolve_deepest_existing shared (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 ACP session/new no-roots ledger line added, the 409-removal entry moved under a ### Removed heading, and three missing single-root-visible CHANGELOG entries added. Deferred: SessionMutator dead-parameter removal and the localized /cd refusal 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 reviewed 270b503a4)

  • B22-1: repo law's judgment adds a leg on the PRE-normalization raw candidate via the shared resolve_deepest_existing — the symlink+.. hop (/p/l/../secret through /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.
  • B22-2: filePath joins 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.
  • B22-3: the two canonicalize-at-judgment disclosures are actually written (Known-scope bullet above + CHANGELOG) — the round-21 disposition claimed them without writing them; owned in the commit body. The disclosure names both spelling directions and marks deny-silencing fail-open.
  • B22-4: the restore face now joins the snapshot's owning thread and, when the thread's set has a root outside the primary, appends the attached-roots boundary clause to {"restored": id} — "every rollback surface names that boundary" is true including this face. Pin: multi-root carries the clause, single-root stays byte-identical.
  • B22-5: the judged-cwd resolution moved into a shared core primitive (resolve_operand_cwd) used by BOTH the engine lane and Runtime::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.
  • SF22-3: the round-21 exec-resume guard reverted (the lane never minted moved rows; the guard hard-blocked legitimate resumes and named a nonexistent flag); warning re-worded accurately. SF22-8: MAX_WORKSPACE_ROOTS = 64 intake cap with a pin. SF22-4: the HTTP /thread face answers 400 on intake-validation errors (typed IntakeValidationError; stdio -32603 unchanged) 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.

@github-actions

Copy link
Copy Markdown

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 CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, 2774
  • crates/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 a RwLock, so this branch's self.rulesets.iter() will not compile against current base; and
  • #37's absolute_path_rule_matches fallback 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_SYMBOL has no runtime consumer (definition + doc link + value-pinning test only), and its doc ("surfaces carry this symbol") overstates: every materialization point hand-rolls normalize_workspace_roots. Delete it or reword; the pinning test is tautological.
  • ACP dead data flow: prepare_acp_tool_admission reads registry.context().workspace_roots, but the registry context is built without with_workspace_roots, and load_session drops 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_dir for cwd (same class as the resume bug fixed here); the ThreadForkParams wiring has no test.
  • The resume running-threads branch (applying resolve_resume_roots to the cached thread) has no direct test.
  • Thread DTO always serializes "workspace_roots": [] while params skip when empty — inconsistent wire convention; add skip_serializing_if or a pinning test.
  • update_thread_workspace_rejects_active_turn was 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_context becomes write-only after the workspace_changed gate (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-clean practice 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.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_tool passes workspace_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 through ToolCallRequest.
  • Read-only Scout cannot cd into an attached root: resolve_path is 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.rs has no multi-root case.
  • Parent-repo registration: the new forkguard_workspace_roots_* tests are not yet in the fork-guard.sh registry / 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
asto18089 self-requested a review September 15, 2026 08:18

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_workspace follows Event::SessionUpdated mid-run while latest_workspace_roots is 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): only context.workspace is 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 use registry.context().workspace_roots, while AutoReviewContext::from_tool_call passes &[] for the carve-out judgment. Conservative direction, but same fact, two answers — unify or comment. Also build_acp_tool_registry (acp_server.rs:2111) never calls with_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 by workspace_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. A tracing::warn! matches the crate's existing tolerance-with-visibility precedent.
  • CreateThreadRequest.workspace_roots lacks skip_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

  • 63f4d15bb leaves the tree uncompilable under --all-targets (E0063 ×4 in app-server, ×3 in config tests) until 058d9b37b six commits later; ca702df9e is a similar rebase fixup. Confirmed by checking out 15cd15eab. 058d9b37b, ca702df9e, and e2dc637a0 (the born-dead WORKSPACE_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-1062 has mangled test indentation; one unrelated blank line removed in execpolicy/src/lib.rs:2417. Pure diff noise — please sweep.
  • The v5 migration block doesn't update the local user_version variable (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.

@qiuYliangM
qiuYliangM force-pushed the pinvou3/workspace-roots-v12 branch from 98f57ff to 890d5f6 Compare September 16, 2026 04:13
@asto18089
asto18089 self-requested a review September 16, 2026 05:43

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 a toggle 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_roots prepends the cwd and both spawn paths persist the full set, so a newly created single-root thread carries workspace_roots: [cwd] on the wire and in the state row — skip_serializing_if never fires for threads created by this build (protocol/src/lib.rs:94-97 comment vs core/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_workspace likewise stores a full normalized set into app.workspace_roots (documented additional-roots-only, app.rs:1594-1597) and into session metadata, so /cd'd session files contain the duplicated primary; and the core/session.rs:171-175 doc 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_roots dedups lexically, not canonically (core/src/lib.rs:71-79): /var/x + /private/var/x both 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_workspace swap, or the LoadSession ordering 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 in resume_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:882 and config/status.rs:282 were fixed in 890d5f61d, and the remaining lib.rs:12079 is 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 SyncSession handler records the action's set before the fallible provider-identity return (apply.rs:1282 vs the Ok(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_roots lacks #[serde(default)] (latent, state/src/lib.rs:79); the round-3 commit message says the v5 migration "updates its local user_version mirror" while the change deliberately does the opposite (code and its comment state/src/lib.rs:659-664 are correct — the message is inverted); dead store in fork'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.

@qiuYliangM
qiuYliangM force-pushed the pinvou3/workspace-roots-v12 branch from 890d5f6 to 28ba314 Compare September 16, 2026 07:25
@qiuYliangM

Copy link
Copy Markdown
Collaborator Author

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):

  1. ViewEvent::SessionSelected seeds app.workspace_roots from the target session's metadata after the fallible restore steps succeed.
  2. Startup --resume/--continue seeds on the success arm of apply_loaded_session_with_goal, so a resumed multi-root session no longer runs single-root.
  3. codewhale fork stamps the source's set before the save, with the same sticky-erasure rationale as the in-app /fork.

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 fork is removed; ThreadMetadata.workspace_roots gains #[serde(default)]; the protocol DTO comment now states the real key-omission scope (legacy/imported rows only - both spawn paths persist at least the cwd); and a core test pins the running-threads cache writeback (resume with roots, then a parameterless resume must not resurrect the stale set).

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 build_direct_workflow_tool is a headless enforcement surface, not a display path.

Scheduled follow-ups as you listed: canonicalize once at normalize_workspace_roots intake, align the additional-roots doc wording with the full-set storage convention, red-green tests for the TUI seeding/stamp paths, a deny-rule-scope test, and the SyncSession parity test.

@asto18089
asto18089 self-requested a review September 17, 2026 06:28

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 /C runs 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:779 stamp → direct save_session at :826, no merge), while /cd cannot 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. /cd needs 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_thread keeps 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_roots neither 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 history skips 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_session stamps the source's set without re-normalizing against the fork's actual workspace (session.rs:154), so the durable record can have roots[0] != workspace, violating SessionMetadata'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:779 never refreshes metadata.workspace), amplifying a pre-existing staleness.
  • The new workspace_changed gate (crates/tui/src/core/engine.rs:3505-3517) also skips the AGENTS.md re-read for a same-path SetWorkspace — 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.
  • SyncSession failure 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_key on (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_roots canonicalization doesn't dedup (macOS /var/x plus a cwd under /var/x both 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.

@qiuYliangM

Copy link
Copy Markdown
Collaborator Author

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):

  1. /cd now persists its swap: the lifecycle merge treats disk as authority only against a roots-blind (empty incoming) writer, and switch_workspace additionally persists a direct snapshot save right after the swap (the fork paths' pattern). A session_manager test pins the full switch -> autosave merge -> restart round trip, including the abandoned-directory-must-not-resurrect assertion.
  2. Runtime::invoke_tool carries a disclosure comment for the empty root set (app-server lane; under-prompting only, byte-identical to base), and the lane is added to the body's known-scope list with the scheduled follow-up.
  3. Scope and path now pair per root: the filters are one gate, and a rule's rooted path matching runs only against its scoped candidate roots. Negative + control test added with a Deny rule carrying both workspace and a relative path - exactly the shape that survived the independent filters.
  4. Worktree subagent children clear the inherited set (the worktree is the isolation boundary); explicit cwd: swaps keep the parent's set, now disclosed in the body as the pinned behavior. The Some([]) explicit-clear leg is pinned on the wire (decodes to Some(vec![]), re-encodes with the key present), and the non-empty assert now checks the encoded frame.

Non-blockings landed: fork_from_session re-normalizes the stamped set against the fork's actual workspace; the struct-literal blank-line noise is swept. The remaining notes (fork_thread fallback-cwd, normalize absolutizing, the history-carrying resume cache revert, save_current_session stale workspace, the same-path SetWorkspace AGENTS.md skip, the layer-over-action deny->ask downgrade note, writable-root dedup) are tracked in the body's known-scope/scheduled list.

@qiuYliangM
qiuYliangM force-pushed the pinvou3/workspace-roots-v12 branch from 12b8867 to 33b23a4 Compare September 17, 2026 09:21

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. "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 and Op::SyncSession frames gain the workspace_roots key (additive, legacy decoders tolerate it — exactly what the DTO comment now correctly states). Please narrow the claim in the Behavior boundary section to match.
  2. 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-155 clones the source's set verbatim; re-normalization happens transitively in the engine on SyncSession and on the next autosave. Please reword.
  3. The Runtime::invoke_tool disclosure 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)

  • /cd at a bare prompt (no session yet) unconditionally saves and mints an orphan session file per invocation (session_state.rs:655-673; the engine-side Op::SyncSession right below is guarded by !app.api_messages.is_empty(), the save is not). A current_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_roots primary-swap semantics; worth pinning as the documented choice.
  • PUT /v1/sessions update branch stamps fresh roots over the stale workspace (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/2ec068ed7 describe a WORKSPACE_ROOTS_SYMBOL anchor that does not exist anywhere in the branch, and 635a47822's "fork flow inherits the parent's roots" only became true in dfc5866c1. 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.

@qiuYliangM

Copy link
Copy Markdown
Collaborator Author

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):

  1. workspace_changed gate reverted. The Op::SyncSession handler now reloads the project context unconditionally, byte-identical to base on same-workspace re-syncs (undo/backtrack, provider, runtime). You were right that the gate was wider than its comment — it suppressed the reload on roots-blind syncs too, and no test locked either direction, so the revert is the clean resolution. The loader still reads the primary root only, so the multi-root discipline (AGENTS.md discovery stays primary-root-only, test-locked) is unchanged.
  2. Runtime::invoke_tool comment reworded to "attached-root ask and deny rules never fire" — agreed, the deny side is under-blocking, not under-prompting.
  3. Body corrections applied: the Behavior boundary section now distinguishes inbound decode identity from emitted frames (which always carry at least [cwd] — additive, legacy-tolerated), and the round-5 fork_from_session wording now says the stamp is verbatim with re-normalization happening transitively on SyncSession/autosave.

Non-blocking items acknowledged for the follow-up list: the bare-prompt /cd orphan save, the cross-workspace /fork primary-swap divergence (will pin as the documented choice), the PUT /v1/sessions mixed-vintage record one-liner, an App-layer seeding-order test, and the three stale commit-message claims (moot under squash merge). The macOS pass on the final tree remains outstanding.

Verified on Linux at 9abaa87: cargo check --workspace --all-targets green, cargo fmt --all --check clean, core suite 89/0, and the tui filtered groups green (forkguard 72 / carve_out 13 / sandbox::policy 15 / workspace_roots 16 / runtime_threads 163+2 ignored).

@asto18089
asto18089 self-requested a review September 18, 2026 05:41
qiuYliangM added a commit to Pinvou/pinvou-agent that referenced this pull request Sep 18, 2026
…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>
qiuYliangM added a commit to Pinvou/pinvou-agent that referenced this pull request Sep 18, 2026
…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 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &params.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_session stamps 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 /cd and PATCH. The round-5 body wording ("re-normalization ... transitively") discloses the mechanism but not this consequence.
  • N2 — /rename//title sync metadata.workspace from 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 /cd whose direct save failed, a subsequent /rename writes workspace: new with the stale pre-switch set — resurrecting the abandoned directory on the next resume.
  • N3 — /resume <export> re-points current_session_id/api_messages without resetting app.workspace_roots or 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::SyncSession apply path mutates app.workspace before the fallible provider step but records roots only after it (apply.rs:1284 vs :1316): on a provider-restore failure the next re-sync sends workspace: new with 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/sessions Ok-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_SYMBOL constant that exists nowhere in the PR; 4c3a7ea claims a golden frame that landed nowhere; 33b23a4 claims fork_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.

Comment thread crates/core/src/lib.rs
Comment thread crates/tui/src/commands/groups/session/session.rs
@qiuYliangM
qiuYliangM force-pushed the pinvou3/workspace-roots-v12 branch from 9abaa87 to 88405ff Compare September 18, 2026 07:41
@qiuYliangM

Copy link
Copy Markdown
Collaborator Author

Thanks for the round-7 pass — M1–M3 and the non-blocking tail are addressed in 88405ffbc (pushed). The four flagged commit messages were rewritten and the branch force-updated, so SHAs below the round-6 head changed while the round-6 tree content is byte-identical to the reviewed one (verified with git diff between the pre- and post-rebase heads).

Must-fix

  1. M1 — the bridge ran every turn single-root. RuntimeThreadHint now carries the response thread's workspace_roots and create_runtime_thread sends them in the POST /v1/threads body, which the runtime API already accepts. An empty set stays off the wire, so single-root frames are unchanged. Tests pin both halves: thread/start with a declared set records it in the hint, and the bridge request frame carries workspace_roots for a multi-root hint and omits it for an empty one.
  2. M2 — /save dropped the set. The live app.workspace_roots is stamped before the write, the fork paths' pattern, so /save-then-quit and explicit-path saves both persist it. Red-green: with the stamp line removed the new test fails left: [].
  3. M3 — the wire could not express inherit. ThreadForkParams.workspace_roots is now Option<Vec<PathBuf>> mirroring Resume, and Runtime::fork_thread inherits the parent record it already loads when the field is absent — the fork's cwd takes the primary slot and the parent's additional roots survive (the cwd-only-resume rule); Some([]) stays an explicit clear. Parity tests pin the legacy shape decoding to None and the Some([]) leg re-encoding with the key present; core tests cover inherit, cwd-only swap, and explicit clear.

Non-blocking

  • N1 /fork <id> now applies the primary swap (/cd and PATCH semantics): the source's abandoned primary leaves, its additional roots survive re-normalized against the fork's workspace. Test pins both the persisted workspace/roots pairing and that the abandoned directory does not re-enter.
  • N2 /rename//title sync the paired roots from the live App on the main path, and the pre-first-snapshot rebuild stamps them too.
  • N3 /resume <export> re-derives app.workspace_roots from the imported record instead of leaving the previous session's set to be stamped by the next autosave.
  • N4 repo law is now per root: repo_law_plan_decision takes the root set, judges the write once per root in that root's own namespace, and keeps the strongest decision (law can only add holds). Both call sites (turn loop, ACP admission) pass their set; an empty set keeps the single-root behavior exactly, so the ACP lane is unchanged. Tests: an attached root's own block fires, primary globs do not leak across roots, and a cross-root block outranks an ask.
  • N5 the Op::SyncSession apply path records app.workspace and app.workspace_roots in the same step, before the fallible provider restore, so a failed restore can no longer leave workspace: new paired with the old set.
  • N6 the state writer warns through the same channel as the reader when workspace_roots cannot be serialized, keeping the tolerant empty-set fallback so an upsert cannot fail over one non-UTF-8 path.
  • N7 the exec stamp test now asserts the persisted set and workspace; the PUT /v1/sessions test changes the thread's roots through the runtime PATCH between the POST and the PUT, so the re-save must write the newer set (it was satisfied vacuously before). The worktree-subagent root clearing still has no direct test — covering it needs a full worktree spawn harness that does not exist — and is disclosed as an open gap in the body rather than claimed.

Body corrections

The phantom "golden frame in the AGENTS.md lock test" and the inverted "v5 migration updates its local user_version mirror" items are removed/corrected in the round-3 list; the verification paragraph no longer carries the contradictory macOS sentence (the round-6 head macOS pass is recorded as yours); and the four commit messages citing WORKSPACE_ROOTS_SYMBOL, a golden frame, or a fork_from_session re-normalization the diff did not make were rewritten.

Verification (head 88405ffbc, Linux)

  • cargo check --workspace --all-targets green; cargo fmt --all --check clean.
  • Suites: protocol 91, core 96, state 29, execpolicy 126, app-server 106.
  • tui groups: forkguard 72, carve_out 13, sandbox::policy 15, workspace_roots 17, runtime_threads 163, repo_law 15, commands::groups::session::session 31.
  • One environment note: two unrelated codewhale-tui --lib widget/i18n tests (tui::widgets::tests::repo_law_approval_has_distinct_authority_grammar, commands::groups::session::remote_env::tests::localized_copy_preserves_composed_placeholders) overflow the default 2 MiB test-thread stack on this Linux box and abort the process; both pass with RUST_MIN_STACK=16MiB and neither path is touched by this PR. A macOS re-run of the listed suites on this head is still welcome.

@asto18089
asto18089 self-requested a review September 18, 2026 07:53
@asto18089
asto18089 force-pushed the pinvou3/workspace-roots-v12 branch from 88405ff to 98b87d5 Compare September 18, 2026 08:04

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::session matches 0 tests; the correct filter commands::groups::session::session matches exactly 31 (the number itself is real).
  • Round-7 remediation section: "/rename and /title ... both paths now carry the live set" — see M1; only /rename was 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_threads but 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 (:669 params.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 calls persist_thread) or documenting the boundary would do.
  • N2 — the AGENTS.md primary-root forkguard test is hollow at runtime: project_context.rs:1305-1344 only 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 &Path signature (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_normalized keeps 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, and strongest_hold_wins_across_roots pins it as intentional, but the doc at repo_law.rs:44-51 ("a root's constitution holds writes under that root") is narrower than the pinned behavior.
  • N4 — Op::SyncSession.workspace_roots has no wire-level parity case; every_variant only counts it with an empty vec, so a future change to its default/skip_serializing_if attributes 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, unlike invoke_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/x both survive) is only disclosed in the body's follow-up list. One doc line would close it. (Trivial: 25a655669 is typed docs(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.

@qiuYliangM

Copy link
Copy Markdown
Collaborator Author

Thanks for the round-8 pass — the must-fix, the M2 decision, and the non-blocking tail are addressed in 0d855be39 (pushed on top of the current rebased head).

Must-fix

  1. M1 — /title was the last unstamped writer. Your read is right: round 7 fixed rename.rs (both paths) and the shared first-snapshot fallback, but /title mirrors that write path in its own function. title.rs::set_window_title_with_manager now stamps app.workspace_roots beside app.workspace, with the same reasoning comment as /rename. The test seeds exactly the post-/cd mismatch (disk pair = before-cd + shared, live pair = after-cd + shared) and asserts the persisted pair; verified red-green — without the stamp it fails with the pre-switch set.
  2. M2 — landed option (a); the ACP lane keeps its roots. build_acp_tool_registry now takes the root set and applies it to both the ToolContext (with_workspace_roots) and the per-turn sandbox policy; AcpSession stores the set so the mode-change registry rebuild preserves it; session/load seeds it from saved.metadata.workspace_roots; and the auto-review context at the same call site reads registry.context().workspace_roots instead of a literal &[], so ask/deny, repo law, and auto-review all see one set. Test: a saved multi-root session loads with the set on the session, in the registry context, and in the materialized writable roots. The body disclosure for this lane is gone; the build_direct_workflow_tool gap keeps its own comment (N5).

Non-blocking

  • N1 the cached history-free resume branch now persists the resolved cwd/roots (refreshing updated_at) instead of writing the cache only — a later history-carrying resume bypasses the cache and re-read the stored row, silently reinstating the pre-override set. The new test primes the cache, overrides through it, then resumes with history; verified red-green.
  • N2 renamed to forkguard_workspace_roots_instruction_discovery_takes_only_the_primary_root, and the comment now states that the guarantee is the loader's single-root signature (compile-time), with the test pinning the consequence (the block is a function of the primary root alone).
  • N3 the repo_law_plan_decision doc now describes the pinned behavior: each root is judged in its own namespace, and a relative target is judged against every root — fail-closed, an extra prompt or block at worst — while execution resolves relative paths against the primary.
  • N4 Op::SyncSession.workspace_roots has a wire-level case now: empty stays off the wire, non-empty round-trips, legacy-without-key decodes empty.
  • N5 build_direct_workflow_tool carries the same disclosure comment Runtime::invoke_tool has (enforcement-only, byte-identical to base, scheduled follow-up). The ACP half of this item is resolved by M2.
  • N6 normalize_workspace_roots's doc records the lexical-dedup boundary (symlink dual spellings survive; enumerating callers canonicalize per root).
  • The one item not changed: the commit typed docs(tui) that carries the behavior-locking AGENTS.md test. Retyping it rewrites a published commit, so I left it — say the word and I will rebase that one commit to test(tui).

Body corrections

Note on the push: the branch was sitting on a detached HEAD at 98b87d527 after the earlier rebase, so my first git push targeted the stale local branch ref and was rejected; I pushed the detached tip explicitly and re-attached pinvou3/workspace-roots-v12 to it. The head is 0d855be39 (5 commits on top of the reviewed 88405ffbc tree: /title stamp, ACP roots, resume persist, protocol parity case, disclosure alignment).

@qiuYliangM
qiuYliangM force-pushed the pinvou3/workspace-roots-v12 branch from 0d855be to 0aea9fe Compare September 18, 2026 09:19
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>
@qiuYliangM
qiuYliangM force-pushed the pinvou3/workspace-roots-v12 branch from 9e23403 to be2cd01 Compare October 14, 2026 13:12
@qiuYliangM

Copy link
Copy Markdown
Collaborator Author

Round-47 follow-up — head be2cd01ef (28 commits over f27672935): the compile-broken window is now closed AT THE SOURCE, per your "better" option.

The base advanced (#107 route limits), forcing a rebase. I ran the per-commit cargo check --workspace --all-targets discipline over the whole replayed line — it immediately caught the window's two damage sites in the carrier (the unclosed toolbox assert + the Op::SyncSession literal missing workspace_roots), which I then repaired in the carrier commit itself, plus one maintenance-signature interplay in the round-24 residue commit (restore_snapshot_for_workspace moved to id: &str on the base; the topic's mid-line commit briefly reintroduced &Snapshot — the call sites now match each commit's own signature). The verification pass over the finished line reports zero failures: all 28 commits compile at their own trees — the per-commit verification claims inside the window are now TRUE at their own trees, not replay-inherited.

The body's provenance disclosure is updated accordingly: the window/repair/provenance facts stand as the historical record of the 4755bac88/2b1704ca0/9e234030e pushes (what shipped to review), prefixed with the round-47 source-closure note; figures re-measured at be2cd01ef (drift 100 files +14,904/−945; fork surface 445 files +67,187/−21,066 ≈30.6×; the link check's transient red during the round-47 body edit was the placeholder-probe window — the post-edit run is green).

Local gates at be2cd01ef: fmt clean; core 117/0; runtime_api 179/0 (one parallel-class flake on the first run, green on rerun — the known marketplace::recycle_bin shape); fresh fork-ci running on the head.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!( in toolbox_note_survives_a_batch_without_results and the workspace_roots-less Op::SyncSession literal in session_sync_drops_a_parked_toolbox_eviction_note are repaired in every commit of the replayed line (zero failures), and cargo check --workspace --all-targets passes at the two previously-uncompilable trees I probed (80a1b3021 the carrier, e70ee0599 the 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 dcd4c200f 445 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 turn substring (runtime_threads.rs:5998) that map_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/target composed vs <ws>/target lexical — I traced walk_deepest_existing component by component; both spellings in-tree is structurally true at every consumer).

Criterion verdicts (fresh, this round)

  1. 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.
  2. Elegance: PASS. The R45-4 one-walk consolidation, the v5 BEGIN IMMEDIATE migration (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).
  3. 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.rs pub(crate) widening, shell_dispatcher.rs lint fix); zero TODO/FIXME/dbg!/commented-out code introduced.
  4. 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)

  1. fs-root primary doctrine gap (new; pre-existing shape, but this PR wrote the doctrine): POST /v1/threads accepts {"workspace": "/", "workspace_roots": []} — create_thread guards the workspace only for emptiness (runtime_threads.rs:5490-5493) and validate_workspace_roots checks 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 .git marker 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.
  2. The P2-14 "pinned" closure is hollow (new): symlink_dotdot_root_declaration_keys_the_fold_not_the_kernel_truth (repo_law.rs:552-616) declares link → <parent>/elsewhere — inside the parent — so realpath(<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.
  3. Snapshot exclude-list drift (new): SNAPSHOT_EXCLUDED_DIRS (snapshot/mod.rs:145-168) hand-copies BUILTIN_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 but restore_covers_primary_only judges it covered and withholds the boundary note — /restore overclaims coverage. Derive one list from the other or add a parity pin.
  4. Case-variant scoped deny on macOS (round-46 P2-6 / round-47 ledger 2, still open, re-confirmed by trace): normalize_workspace_scope preserves POSIX case (execpolicy/src/lib.rs:1113-1119) while the path channel folds on case-insensitive platforms — a deny scoped /REPO never fires for /repo on 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.
  5. 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.
  6. Carried (round-47 ledger 5): /rename and /title stamp app.workspace_roots verbatim (rename.rs:125/:180, title.rs:171) while /save and /fork re-validate — one future App-writer away from a laundering lane.
  7. Carried (round-47 ledger 8): the 8 MiB explicit-stack wrapper is hand-rolled triplicate, and restore.rs:440-443 still asserts CI's RUST_MIN_STACK "is load-bearing" for a test that wraps its own stack.
  8. RUNTIME_API.md gaps: the turn_meta Accessible folders: disclosure (zero hits in docs/) is invisible to API readers; the restore boundary firing 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).
  9. 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).
  10. 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 at 480bebf61; the window range acdaf8d2c..59344db94 is 24 commits (25 counts the carrier, which is also broken, so the substance holds).
  11. PATCH-fence pin weaker than the mapping it protects: update_thread_roots_rejects_active_turn (runtime_threads/tests.rs:6181) asserts only contains("active turn") while the 409 hinges on the exact substring already has an active turn — a wording drift re-creates the retry-forever 400 under a green suite. Pin the exact substring.
  12. Stored-row faces run the canonical strip before the intake cap (core/src/lib.rs:569-594 via validate_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.
  13. 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_candidates runs 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.

@asto18089
asto18089 merged commit 803e672 into Pinvou:pinvou3-clean Oct 14, 2026
6 checks passed
asto18089 pushed a commit to Pinvou/pinvou-agent that referenced this pull request Oct 14, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants