feat(session): agent-invokable session rescope via move_session tool - #4688
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ff74c0a to
2c276b1
Compare
|
@probepark @snowykr — adversarial exact-head review requested for head Scope: agent-invokable
The hardening commit If clean, please approve the exact head |
2fbc4a5 to
461965e
Compare
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The follow-up hardening improves path validation and rendering, but the model-invoked rescope still changes only part of the session's project identity. This leaves policy, host, and project-scoped runtime consumers split between the launch root and the moved root.
Findings / Required Changes
-
[P1] Preserve workflow mutation guards after a rescope —
packages/coding-agent/src/sdk/session.ts:2013-2015
move_sessionis available while planning-workflow state is rooted at the original<cwd>/.gjc/_session-<id>. Moving into a descendant makes the cwd-local workflow guard fail open, so a session indeep-interview/ralplancan rescope and then invoke guarded mutations from the new cwd. Reject moves while a blocking workflow is active, or migrate/rebind the workflow guard state atomically and cover mutation denial after the move. -
[P1] Retarget the workspace tree and project-scoped runtime —
packages/coding-agent/src/sdk/session.ts:2059-2064
The move updatesSessionManagerand clears caches, but workspace-tree, extension/custom-tool/plugin, and related project services were constructed with the launch cwd. Subsequent turns can receive the target cwd paired with a launcher-root tree, and already-loaded project tools can retain old-root execution context. Rebuild or rebind all cwd-capturing services/tools as one committed transition; add a regression that verifies the next-turn tree and project tools use the target root. -
[P1] Synchronize SDK/ACP host and broker authority —
packages/coding-agent/src/sdk/session.ts:2053-2062
SDK host/broker records, query metadata, reconciliation state, and location projections retain the startup cwd/state root. Aftermove_session, clients can display or resume against the old root while relative tool execution uses the new root, includingendpoint_staleresume failures. Propagate the moved scope through the host/router/broker atomically, or disallow this tool for SDK/ACP-hosted sessions. -
[P1] Do not mutate process-global cwd for one SDK session —
packages/coding-agent/src/sdk/session.ts:2059
setProjectDir()callsprocess.chdir()and changes module-global project state. The SDK supports concurrent top-level sessions, so moving one session can redirect ambient cwd/capability state for sibling sessions. Keep rescope state session-scoped, or enforce isolation/serialization that makes this operation safe. -
[P1] Hide and serialize the state-changing tool correctly —
packages/coding-agent/src/sdk/session.ts:2013;packages/coding-agent/src/tools/move-session.ts:25-38
The availability check only usestaskDepth, although canonical sub-sessions are also identified byparentTaskPrefixandcurrentAgentType. In addition, the tool has neither exclusive scheduling nor non-abortable lifecycle semantics. Gate with the canonical sub-session predicate, make the transition exclusive and non-abortable, and join it during disposal so concurrent tools/teardown cannot observe a mixed root. -
[P2] Complete edge-case and CI coverage —
packages/coding-agent/src/sdk/session.ts:2015-2025,2039,2053-2068;packages/coding-agent/scripts/generate-tool-catalog.ts:116;packages/coding-agent/src/tools/move-session.ts:68-90
Add coverage for re-entrant moves, post-commit SSH-refresh failure, renderer behavior, and catalog reproducibility. Also make the descendant check separator-aware (..foois a valid child name) and define the one-move latch across logical session transitions/resume.
CI / Verification
- Reviewed exact head:
461965e82146d1a5829ae5fd56cbb8bfc58c262b. - Exact-head affected CI reports the move-session and descriptor/index tests, package check, TypeScript build, CLI smoke, and virtual integration as successful.
- The failed PR-contract jobs are
needs-humancontract verdicts and are excluded from this review as requested. - This review was static/read-only; no PR code was executed locally.
Axis Coverage
| Axis | Coverage |
|---|---|
| A1 — Intent / Policy / Contract | Reviewed tool/session contract, sub-session gating, workflow policy, and one-move semantics. |
| A2 — Architecture / Correctness / Failure | Reviewed transition ordering, workspace-tree lifecycle, concurrency, abort/disposal, and error paths. |
| A3 — Security / Privacy / Trust | Reviewed model-controlled rescope authorization, workflow guard bypass, and ACP path authority. |
| A4 — Verification / Tests / CI | Reviewed exact-head CI evidence and targeted-test/coverage gaps without executing PR code. |
| A5 — Context / Compatibility / Platform | Reviewed SDK/ACP host-broker resume behavior, project-scoped tools, generated artifacts, and cross-session global state. |
Review response — head
|
Independent exact-head re-review: APPROVE (architecture CLEAR)An independent architect review of the current head
Verdict: @snowykr @probepark — the two review-response commits ( — |
|
@snowykr @probepark — fresh exact-head review requested for Your prior CHANGES_REQUESTED items are addressed in
Answered on-thread as /move-parity (not introduced by this PR): Please verify on the exact head: hosted-session/workflow disallow-or-rebind, session-scoped cwd, canonical subagent gate, exclusive non-abortable transition, separator-aware descendant, and the reentry/SSH-failure/render/catalog/latch tests ( If clear, please APPROVE the exact head |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The exact-head hardening closes several earlier gaps, but the new model-controlled transition still leaves durable workflow policy and concurrent session authority unsafe. In particular, a restored workflow can be moved out of its cwd-scoped guard, and the advertised exclusive/non-abortable transition is not joined across all move paths or disposal.
Findings / Required Changes
-
[P1] Preserve the workflow mutation guard for resumed sessions —
packages/coding-agent/src/sdk/session.ts:2032-2040
The new refusal checks onlysession.getActiveSkillState(), which reads the in-memory prompt marker. Resumed active workflows are held inAgentSession.#restoredWorkflowSkillState(packages/coding-agent/src/session/agent-session.ts:9326-9353) and are invisible to that accessor. Moving then leaves the durable.gjc/_session-<id>state at the old cwd; later cwd-local mutation checks can fail open at the target. Reject moves for the effective restored-or-live active workflow state, or atomically rebind/migrate the workflow state and guard. Add a resumed-workflow regression. -
[P1] Make the transition actually exclusive and lifecycle-safe —
packages/coding-agent/src/sdk/session.ts:2020-2084;packages/coding-agent/src/tools/move-session.ts:35-37
The localmoveInFlightlatch does not cover SDK/ACPsession.cwd.move, which invokesSessionManager.moveTo()directly, so client and model moves can relocate the same session concurrently. Also,nonAbortableonly suppresses the tool signal; the executor can publish an aborted/skipped result while the relocation continues, and disposal can force-abort/close the manager during that work. Centralize the move mutex and committed-transition lifecycle in the session manager/owner, and ensure disposal joins it before closing state. -
[P1] Rebind or revoke launch-root project authority on rescope —
packages/coding-agent/src/sdk/session.ts:2068-2079
The move resets selected caches but retains startup MCP/custom-tool authority (and pre-created project-scoped tools such as the Python descriptor) from the broad launcher root. After a nominal scope narrowing, those old-root tools and credentials can remain callable. Recreate/disconnect cwd-capturing managers and tools as one transition, or do not exposemove_sessionfor sessions with such authority. -
[P1] Do not rely on a string-only precheck for the narrowing boundary —
packages/coding-agent/src/sdk/session.ts:2049-2069
The target isrealpath-validated as a descendant, butmoveTo()later resolves/uses the path again. A concurrent replacement of the validated child with a symlink/reparse target can move the session outside the validated root. Commit against a no-follow directory identity or revalidate the canonical target immediately at the state-changing boundary. -
[P2] Cover the remaining transition and generated-artifact cases —
packages/coding-agent/test/move-session-tool.test.ts;packages/coding-agent/src/sdk/session.ts:2028-2031,2069-2077
Add regressions for overlapping calls,bashAllowedPrefixesgating, a failed post-commit SSH refresh, resumed workflow state, and renderer control-character/error output. Also add the tool-catalog reproducibility test to the affected-path CI mapping: this PR changestool-catalog.generated.ts, but the exact-head affected CI does not runpackages/coding-agent/test/tools/tool-catalog.test.ts.
CI / Verification
- Reviewed exact head:
805b7712f4dff5d4a1880fcee80d6cb8f24bccb9. - Exact-head Dev CI run
32191759484passed the affected move-session and descriptor tests, coding-agent check, TypeScript build, CLI smoke, affected aggregate/evidence, and virtual integration. - The PR-contract bootstrap failures are the intentional
needs-humanverdict gate and are excluded from this review. - This was a static multi-axis review; PR code was not executed locally.
Axis Coverage
| Axis | Coverage |
|---|---|
| A1 — Intent / Policy / Contract | Reviewed model tool contract, workflow policy, public availability, and scope-narrowing promise. |
| A2 — Architecture / Correctness / Failure | Reviewed session ownership, relocation ordering, concurrency, abort/disposal, caches, and failure boundaries. |
| A3 — Security / Privacy / Trust | Reviewed model-controlled path authority, workflow-guard bypass, stale project authority, and symlink race exposure. |
| A4 — Verification / Tests / CI | Reviewed exact-head CI evidence and targeted test coverage without running PR code. |
| A5 — Context / Compatibility / Platform | Reviewed SDK/ACP move paths, MCP/custom tool context, generated catalog CI coverage, and platform path semantics. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head 805b7712 — merge blocked. Making rescope model-invokable raises the bar a lot, and the relocation is not yet a session-wide transaction.
major 1 — the move is not serialized against in-flight tool calls
sdk/session.ts:2020-2084. The latch is closure-local to move_session. The SDK/ACP session.cwd.move control operation calls SessionManager.moveTo() directly (modes/runtime-init.ts:437-439, modes/controllers/extension-ui-controller.ts:396-398), so it does not pass through the latch at all. And exclusive only orders calls within one model response (packages/agent/src/agent-loop.ts:3668-3712), while relative tools read live cwd at execution time (tools/bash.ts:1102).
So a move can land after a relative call was planned and admitted but before it executes, and redirect it to a different root. That is the defining hazard of making this agent-invokable: the model reasons about paths under root A and the call runs under root B.
Centralize every move surface behind a session-owned mutex or root generation, and define the join/reject semantics across active turns, tools, jobs, subagents, abort and disposal.
major 2 — narrowing does not revoke launch-root authority
:2068-2079 re-roots SessionManager/session-file state, the dynamic ToolSession cwd getter, the process-global projectDir, capability/plugin-root caches and SSH refresh. Still rooted at launch:
contextFilesand the prompt closure (:3155-3171)- the workspace-tree service (
runtime/workspace-tree-service.ts:16-30) andToolSession.workspaceTree - inherited context and tree for future subagents (
task/index.ts:1952-1954,2093-2095) - Settings and project ignore-policy inputs
- loaded custom/plugin/MCP tools and extensions (
:2292-2901) - SDK host stateRoot/locator/query cwd (
sdk/host/session-runtime.ts:2699-2703,2980-2986) - startup LSP status and old clients
- the status-line HEAD watcher (
modes/components/tool-status-header.ts:278-289)
A "narrowing" operation that keeps the old project's rules, tools and tree is not narrowing — it pairs a new cwd with old authority, which is strictly worse than not moving. Rebuild or revoke every cwd-capturing service atomically.
major 3 — the workflow guard misses resumed workflows
:2032-2040 checks getActiveSkillState(), which exposes only in-memory #activeSkillState (session/agent-session.ts:3721-3728). Resumed workflows live in #restoredWorkflowSkillState (:9326-9354).
So a resumed workflow can move away from its durable .gjc/_session-<id> state and the cwd-local mutation guards fail open. Check effective restored-or-live state, or migrate it atomically as part of the move.
major 4 — the descendant boundary is checked, then committed against a name
:2049-2069 does the string/realpath check before commit. A background process can replace the validated descendant pathname with a symlink or reparse point before moveTo/chdir, putting effective filesystem access outside the approved root. For a boundary whose entire purpose is confinement, TOCTOU is the whole game. Commit against a no-follow directory identity, or revalidate device/inode/type at the state-changing boundary.
major 5 — the failure path leaves three roots disagreeing
:2049-2081. Validation never proves read/search access, and after SessionManager.moveTo() the latch is consumed before the fallible setProjectDir() and registry resolution. setProjectDir assigns the global projectDir before process.chdir (packages/utils/src/dirs.ts:147-150), so an EACCES can leave SessionManager, global projectDir and the process cwd on three different roots while the tool reports failure.
Preflight R_OK/X_OK and the target services, then either make commit plus follow-ups atomic or return an explicit committed-with-warning result.
major 6 — chdir is process-global
:2070-2075. setProjectDir() performs a process-global chdir, so moving one top-level SDK session redirects ambient cwd and every getProjectDir() consumer for sibling sessions in the same process. Either keep SDK rescope session-scoped, or state and enforce a single-session process invariant.
minor — coverage and body
test/move-session-tool.test.ts:46-366 has no pins for overlapping model/client moves, admitted-relative-path redirection, abort/disposal, background work and subagents, resumed workflows, unreadable targets, post-commit failure, stale prompt/tree/tools/SDK locator, renderer controls, or catalog affected-CI mapping.
The body says five tests; the file has twelve. It also claims project plugins reload and the next-turn tree is narrowed — caches are cleared, but loaded project authority and the launch-bound tree service remain, so neither claim holds.
prior findings
From 461965e8: the workflow guard is partially fixed (live only), and canonical gating plus descriptor flags are fixed. Workspace tree/project runtime, SDK/ACP host and broker authority, process-global cwd, central serialization/abort/disposal, and most edge/CI coverage are unfixed. Separator-aware ..dots and the one-move test did land.
At this head specifically, all five previously-raised items remain: resumed-workflow guard, transition exclusivity across surfaces, launch-root project authority, identity-bound narrowing, and coverage.
coverage detail
Pins: top-level exposure with bash cwd; relative target; missing-directory message; outside-scope refusal followed by a valid move; current-directory refusal; one successful move; symlink canonicalization; ..dots child.
Guards that pass on base purely because the tool does not exist there: taskDepth subagent absence, parentTaskPrefix/currentAgentType absence, restricted-profile absence. The live-workflow refusal is a feature-level pin but does not cover restored state.
merge precondition
Conflicts with dev.
Reviewed by @probepark — method: detached worktree at 805b7712, trace of every move ingress to determine what the latch actually covers, enumeration of cwd-capturing services still rooted at launch, workflow-state source comparison between live and restored, validate-versus-commit ordering analysis including setProjectDir internals, per-test base-vs-head discrimination. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:09f501b2af5a7bab2fa0327f4ec9b2db3f619caa3607f87ae3b81dbe884e3bb4 reviewer:human reviewer-id:probepark evidence:exact-head-805b7712-agent-invokable-move-not-serialized-against-in-flight-calls-and-retains-launch-root-authority
805b771 to
c7ef3b7
Compare
|
@codex review exact head Reconstructed 805b771 onto current origin/dev and closed remaining snowykr P1s:
Please APPROVE or CHANGES_REQUESTED the exact head only. Independent exact-head review via the Codex code-review path. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Head
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head c7ef3b74 — merge blocked. One of six majors is fixed, and the new serialization attempt has a defect of its own.
fixed
Resumed-workflow guard. getEffectiveActiveWorkflowSkillState() now checks both #activeSkillState and same-session #restoredWorkflowSkillState, and rescopeSessionCwd rejects on a canonical workflow. The restored-workflow regression genuinely discriminates against 805b7712. Good fix.
The renderer sanitization and the affected-CI catalog mapping are real improvements too.
major 1 — the new mutex is not a mutex
session-manager.ts:10358-10373:
async runExclusiveCwdTransition<T>(fn: () => Promise<T>): Promise<T> {
if (this.#cwdTransitionDepth > 0) {
this.#cwdTransitionDepth += 1;
try { return await fn(); } finally { this.#cwdTransitionDepth -= 1; }
}
...#cwdTransitionDepth is instance-global, not async-context-local. The early branch is intended as reentrancy for nested calls inside one transition, but it carries no owner identity — so any unrelated caller arriving while a transition is in flight also sees > 0 and runs fn() immediately, skipping the queue entirely. The fence is open exactly when it is needed.
And relative tools still never join it: bash reads live this.session.cwd at tools/bash.ts:1102, so a move can still land after admission and before execution — the original hazard. TUI side effects also remain outside moveTo() (modes/controllers/command-controller.ts:1156-1161), so the "entire move transaction" is not inside the lock either.
Needs a real queued mutex with a private already-locked path keyed to the running transition, plus cwd-generation or admission fencing across every relative tool.
major 2 — launch-root authority still survives
sdk/session.ts:2061-2064,3224-3260. The prompt closure still uses the immutable startup const cwd from :1278, along with startup contextFiles, rules, skills and workspaceTreePromise. ToolSession.workspaceTree is fixed; runtime/workspace-tree-service.ts:19-30 captures launch cwd; subagents inherit stale context/tree (task/index.ts:1952-1954,2093-2095). Settings/ignore policy, extensions (:2984-3102), startup LSP metadata (:3908-3913), SDK host stateRoot/locator (sdk/host/session-runtime.ts:2699-2703,2980-2982) and status watcher authority all remain.
rebindCwdCapturingAuthority() covers only selected Python/subskill/plugin/MCP tools, runs after commit, and swallows its own failures. So the rebind is best-effort on a subset, after the point of no return.
The body also defers tree rescope to #4628 while simultaneously claiming the next-turn tree is narrowed. Those cannot both be true.
Either rebuild or revoke every cwd-capturing authority fail-closed before publishing the new root, or do not expose the model tool in sessions where you cannot.
major 3 — identity is checked once, then not held
session-manager.ts:10404-10417. The added (dev, ino) lstat catches replacement before moveTo begins, which is an improvement. But it is a single check followed by many asynchronous session/artifact operations before this.cwd = resolvedCwd at :10664 and the later chdir. The pathname can be swapped inside that window. Hold an opened no-follow directory handle and commit against its verified identity.
major 4 — failure atomicity
sdk/session.ts:2149-2169 commits moveTo, sets moveConsumed = true, then runs fallible setProjectDir, registry resolution and authority rebinding, with no R_OK/X_OK or service preflight. setProjectDir writes module-global projectDir before process.chdir (packages/utils/src/dirs.ts:147-150), so an EACCES still leaves SessionManager, projectDir and process cwd disagreeing while the tool returns an error.
major 5 — process-global chdir
sdk/session.ts:2154-2156. The public SDK permits multiple top-level sessions and this PR enforces no single-session invariant, so one model-controlled move redirects ambient cwd and getProjectDir() for siblings in the same process. Parity with interactive /move does not transfer — /move is a human acting on the session they are looking at; this is a model acting on one of several.
minor — the new race tests do not discriminate
test/move-session-tool.test.ts:426-507:
- The overlap test delays before the real lock and accepts either final cwd, so it passes with the broken depth shortcut above. It is a feature pin, not a serialization pin.
- The abort test calls
tool.executedirectly, bypassing agent-loopnonAbortabledispatch, andagent.abort()supplies no tool signal — so it does not pin the lifecycle behavior it names. - The identity test starts from a deliberately wrong identity rather than swapping after validation, so it pins immediate mismatch, not the TOCTOU window in major 3.
Body says five tests; the file has nineteen.
scope
The four newly touched areas (workflow/tool rebinding, SessionManager transition/identity, live-cwd Python construction, CI mapping) are all responses to prior findings rather than unrelated creep. But the generic moveTo() mutex changes every TUI/SDK/ACP move callsite, which is how major 1 now affects paths that previously had no concurrency story at all.
coverage
Real pins: top-level exposure with bash cwd; relative target; missing-directory rejection; outside-scope then valid move; current-directory refusal; one successful move; ..dots descendant; symlink canonicalization (ordinary only); restored-workflow refusal; SSH-refresh-after-commit treated as non-fatal; renderer sanitization.
Base guards that pass only because the tool is absent on base: taskDepth > 0, parentTaskPrefix/currentAgentType, bashRestrictionProfile, bashAllowedPrefixes.
Missing: admitted relative-path execution under the original root; genuinely overlapping critical sections across model and client moves; move admission during disposal; unreadable/unsearchable target; setProjectDir or registry failure after commit; stale prompt/context/tree/extensions/custom tools/MCP/host locator/LSP/status; sibling-session ambient cwd isolation.
Reviewed by @probepark — method: detached worktree at c7ef3b74, direct read of runExclusiveCwdTransition to establish the reentrancy shortcut has no owner identity, enumeration of cwd-capturing authorities still bound to startup, check-versus-commit window analysis in moveTo, setProjectDir internals, per-test discrimination against both base and the prior head. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:a22ec5871c43cf10d747f3d0dc930a5b3e2a5d0ac2e1839bf224940622789741 reviewer:human reviewer-id:probepark evidence:exact-head-c7ef3b74-reentrancy-shortcut-lets-concurrent-callers-bypass-the-cwd-mutex
c7ef3b7 to
e3cad12
Compare
|
@codex review exact head Rebased onto current origin/dev and closed remaining probepark exact-head blockers:
Please APPROVE or CHANGES_REQUESTED this exact head only. Do not treat the previous 805b771/c7ef3b74 reviews as current. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@probepark — one exact-head re-review request for Your c7ef3b7 majors were addressed on this head:
23 move_session tests, including queue, generation fence, sibling isolation, hide, identity swap. Codex review is quota-exhausted; this is the single independent re-review request for this head. |
22ac045 to
737f8d6
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 5b57a3fc — still merge blocked. Nothing changed.
$ git diff --stat 9c88dccc 5b57a3fc -- sdk/session.ts session-manager.ts agent-session.ts move-session-tool.test.ts
(empty)
The rescope implementation and its tests are byte-identical to the head I blocked. The head moved for other reasons. All five majors carry over, so this is a short restatement rather than a fresh review.
For clarity on the raised threshold the maintainer set — I re-argued each by concrete harm rather than carrying them forward automatically:
1. Admission does not cover execution (agent-session.ts:8237-8244, tools/bash.ts:1051-1102). The wrapper checks the generation, then bash yields at await expandInternalUrls(...) and only afterwards computes commandCwd from live this.session.cwd. A move starting in that window runs a root-A command in root B. That is real misdirected execution.
2. The model is told cwd B and shown A (sdk/session.ts:3264-3283, runtime/workspace-tree-service.ts:19-35, task/index.ts:2047-2094). I said last round I would only treat this as major if it produces wrong behavior rather than stale convenience data. It does: the volatile message uses current sessionManager.getCwd() while attaching the launch-bound tree, and subagents launch at live cwd while inheriting startup contextFiles, skills and workspaceTree. The model gets root B's cwd with root A's files and project instructions — that drives wrong path selection, not just a stale display.
3. The handle is never the authority (session-manager.ts:10462-10498). It is used only for stat(); commit is this.cwd = resolvedCwd and the caller later runs setProjectDir(canonicalTarget), which is process.chdir(resolved) on the name. A replacement after the last comparison escapes the validated descendant — the exact confinement this tool exists to enforce.
4. Failure after commit (sdk/session.ts:2164-2182). moveTo commits, moveConsumed = true, then fallible setProjectDir, LSP shutdown, registry resolution and rebindCwdCapturingAuthority run, the last swallowing plugin/MCP failures. The tool can reject after the session moved, or succeed with partially stale authority.
5. ownsProcessCwd is a guess (sdk/session.ts:2161-2174). Two SDK sessions launched at the same root both satisfy processCwd === canonicalFrom, so moving one calls process-global setProjectDir, resetCapabilities, shutdownAllLspClients and plugin-cache clearing under its sibling. This reaches real execution — browser tab work passes { cwd: process.cwd() } (tools/browser/tab-supervisor.ts:342-344).
the fixed parts are still fixed
The ALS-owner mutex (session-manager.ts:10504-10523), the effective resumed-workflow guard, the fs.access(R_OK|X_OK) preflight, and setProjectDir chdir-before-assign all remain correct.
coverage, as comments
move-session-tool.test.ts:569-594 starts the move before bash admission, so it tests waiting on an active transition rather than a move beginning mid-await. :635-658 replaces the target before moveTo(), not after the final check. :550-567 never creates two sessions sharing one root. :614-633 covers only SSH refresh, not setProjectDir/LSP/registry/tool-replacement failure. These are gaps, not the basis for the block — the block is the code above.
Reviewed by @probepark — method: detached worktree at 5b57a3fc, git diff against the previously blocked 9c88dccc over the rescope sources and tests (empty), then re-argument of each carried finding by concrete harm rather than by carryover.
gajae.pr-review-verdict.v1 merge-blocked sha256:90d3dbe830ea4ea87a7a59548b234bda5aad903f37e3c6a21d6192c7693a0a12 reviewer:human reviewer-id:probepark evidence:exact-head-5b57a3fc-rescope-sources-unchanged-since-9c88dccc-all-five-majors-carry-over
5b57a3f to
f8ecc84
Compare
Signed disposition of all five majors — exact head
|
f8ecc84 to
6d56baa
Compare
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Non-owner rescope mutates process-global state and Rebound MCP manager loses runtime callbacks. These findings require changes before approval.
Findings / Required Changes
- [P1] Non-owner rescope mutates process-global state.
Reference:packages/coding-agent/src/sdk/session.ts:2250-2265
TheownsProcessCwdguard only protectssetProjectDir, but non-owner sessions still callresetCapabilities(),shutdownAllLspClients(), andclearPluginRootsAndCaches(), disrupting sibling sessions. Gate all process-global operations on ownership or provide session-scoped equivalents. - [P1] Rebound MCP manager loses runtime callbacks.
Reference:packages/coding-agent/src/sdk/session.ts:4114-4130
After move_session replaces the owned MCP manager, prompt/resource callbacks remain attached only to the launch-time manager; wire these callbacks onto every replacement manager so MCP prompts, resources, and notifications continue functioning after rescope.
CI / Verification
- Reviewed the exact remote head:
6d56baa6f073b4ca338d320da578654c02cb1683. - CI summary: 14 passing, 2 failing, 30 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap. - Non-successful checks without pass evidence:
Affected path validation / test:packages/coding-agent/test/session-manager-resident-cache.test.ts,Affected path validation / test:packages/coding-agent/test/notifications-live-stream.test.ts,Affected path validation / workflow-permissions,Affected path validation / ci-dry-run,Affected path validation / ci-selftest,Affected path validation / install-methods,Affected path validation / check:@gajae-code/utils,Affected path validation / test:packages/coding-agent/test/tool-discovery/initial-tools.test.ts. - Passing evidence reviewed:
Affected path validation / test:packages/natives/test/windows-avx2-probe.windows.test.ts,Affected path validation / cli-smoke,Affected path validation / test:packages/coding-agent/test/move-session-tool.test.ts,Affected path validation / test:packages/coding-agent/test/tools/tool-catalog.test.ts,Affected path validation / test:packages/coding-agent/test/autoresearch/python-tool.test.ts,Affected path validation / test:packages/coding-agent/src/runtime/workspace-tree-service.test.ts,Affected path validation / check:@gajae-code/coding-agent,Affected path validation / test:packages/coding-agent/src/tools/descriptors.test.ts. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Rescope correctly narrows session scope, but owned MCP manager replacement breaks post-move prompt/resource callback compatibility. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency fencing, serialized moves, read leases, and session rescope behavior show no established correctness defect. |
| A3. Security / Privacy / Trust | APPROVED | The rescope implementation maintains descendant confinement, identity checks, authority rebinding, and session isolation; no security or privacy defect was established. |
| A4. Verification / Tests / CI | APPROVED | A4 review found targeted move-session coverage and passing affected tests, but incomplete and failed CI prevents a full regression verdict. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | A5 integration and platform isolation are not fully correct because a sibling session can have its global capabilities, LSP clients, and plugin caches reset during another session’s rescope. |
Limitations
- PR contract and bootstrap CI jobs are failing, while several affected-path jobs remain pending; full CI validation cannot be claimed.
|
@probepark — exact-head re-review request for
This head = the five-major remediation (detailed in the disposition comment above) + a biome formatting commit + a rebase onto current Each of your five majors now has a code change plus a test that fails without it:
Your four named coverage gaps are closed by tests that exercise the harm rather than its neighbourhood: a move starting mid-await (not before admission), a replaced target after the final check, two sessions sharing one root, and rebind/rollback failure beyond SSH refresh. Plus Verification on this head: Please APPROVE or CHANGES_REQUESTED this SHA only — reviews on — |
Merge PR #4688 onto current dev, preserve current-dev autoresearch changes, isolate non-owner sessions from process-global cache resets, and retain MCP callbacks when the owned manager is rebound. Lore-id: pr4688-current-dev-fix-forward Constraint: no release, tag, or publish mutation Confidence: high Scope-risk: wide Reversibility: revertible Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/move-session-tool.test.ts; bun test packages/coding-agent/test/tools/tool-catalog.test.ts packages/coding-agent/test/tool-discovery/initial-tools.test.ts packages/coding-agent/src/tools/descriptors.test.ts Not-tested: Windows and remote CI lanes
|
Fix-forward status for exact head
Merge remains blocked pending an independent authenticated exact-head APPROVED review. The PR contract correctly reports |
331bd42 to
5361ace
Compare
Merge PR #4688 onto current dev, preserve current-dev autoresearch changes, isolate non-owner sessions from process-global cache resets, and retain MCP callbacks when the owned manager is rebound. Lore-id: pr4688-current-dev-fix-forward Constraint: no release, tag, or publish mutation Confidence: high Scope-risk: wide Reversibility: revertible Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/move-session-tool.test.ts; bun test packages/coding-agent/test/tools/tool-catalog.test.ts packages/coding-agent/test/tool-discovery/initial-tools.test.ts packages/coding-agent/src/tools/descriptors.test.ts Not-tested: Windows and remote CI lanes
5361ace to
b600f75
Compare
|
@probepark @snowykr — please perform the independent authenticated exact-head review for PR #4688 at fdff4da. The current exact-dev diff digest is sha256:2b9719fa3b6ef96f65f9b9bfb50f61e435e03e56f00bb7860390cf3c42778fbc. Prior P1 findings are addressed with explicit flush rollback, post-publication committed-result handling, exact MCP-config hiding, agentDir-aware conventional/plugin MCP rebuild with callback wiring/classification, workflow recheck under the transition lock, and 34 focused move-session tests. Please review this SHA only and return APPROVED or CHANGES_REQUESTED; no approval is being presumed. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head fdff4da2 — merge blocked.
Prior invokability, authority, workflow, and path-identity fixes remain present. The current delta introduces three lifecycle wedges:
session-manager.ts:10551-10557leases cwd reads while nested tool dispatch can occur from an outer eval (eval/js/tool-bridge.ts:32-33,103-108). If a writer queues between the outer and nested reader, the nested reader waits for the writer while the writer waits for the outer reader: durable deadlock, with no AbortSignal cancellation.sdk/session.ts:2574-2608,4365-4393claims process-cwd ownership before tool/plugin/MCP construction but failure paths beforeAgentSessiondo not release it. A retained manager can poison later valid sessions' cwd/global rebinding.sdk/session.ts:2187-2188,2414-2427performs a supposedly best-effort post-commit state refresh that can throw aftermoveTohas committed. The caller reports failure although storage/cwd moved, then the one-shot budget prevents retry, leaving contradictory lifecycle state.
Make nested read leases re-entrant or leaf-scoped, release ownership on every construction failure, and make post-commit refresh genuinely nonthrowing or prepare it before commit with rollback.
Reviewed by @probepark — method: compared current changed blobs to 5b57a3fc and traced only the new lease, ownership, and post-commit refresh paths.
gajae.pr-review-verdict.v1 merge-blocked sha256:2b9719fa3b6ef96f65f9b9bfb50f61e435e03e56f00bb7860390cf3c42778fbc reviewer:human reviewer-id:probepark evidence:exact-head-fdff4da2-nested-cwd-lease-deadlock-ownership-leak-postcommit-error
|
@probepark — exact-head fix-forward is ready at |
|
@probepark — the final exact-head fix-forward is now |
Merge PR #4688 onto current dev, preserve current-dev autoresearch changes, isolate non-owner sessions from process-global cache resets, and retain MCP callbacks when the owned manager is rebound. Lore-id: pr4688-current-dev-fix-forward Constraint: no release, tag, or publish mutation Confidence: high Scope-risk: wide Reversibility: revertible Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/move-session-tool.test.ts; bun test packages/coding-agent/test/tools/tool-catalog.test.ts packages/coding-agent/test/tool-discovery/initial-tools.test.ts packages/coding-agent/src/tools/descriptors.test.ts Not-tested: Windows and remote CI lanes
Current dev renamed and added AgentSession rescope helpers, so the generated operation inventory rejected the valid ported implementation. Keep the internal helpers explicitly excluded from the public SDK surface and regenerate the checked artifact. Lore-id: pr4688-current-dev-sdk-seams Constraint: do not expose internal move_session helpers as public SDK operations Confidence: high Scope-risk: narrow Reversibility: revertible Tested: bun test packages/coding-agent/test/sdk-operation-inventory.test.ts Not-tested: sdk-downgrade rollback fixture under Bun 1.4.0
A failed flush or durable move could leave process cwd and project authority at the target while SessionManager stayed at the launch root. The rescope now restores launch authority on pre-commit failure, treats post-publication metadata errors as committed, rechecks workflow admission under the transition lock, excludes exact MCP-config sessions, and rebuilds conventional/plugin MCP authority with the session agent directory and tool classification after a move. Lore-id: pr4688-rescope-atomicity Constraint: rejected rescope must not leave process, authority, and durable session roots split Constraint: exact MCP config sessions must not silently swap authority Constraint: conventional MCP discovery must follow the target agent directory Confidence: high Scope-risk: wide Reversibility: revertible Tested: bun test packages/coding-agent/test/move-session-tool.test.ts; bun --cwd=packages/coding-agent run check Not-tested: live external MCP server callback delivery and Windows exact-config fixtures Directive: do not consume the one-move budget when flush or durable publication fails
Exercise the post-publication metadata failure branch so move_session returns the committed destination instead of reporting a false rejection or allowing a second move. Lore-id: pr4688-post-publication-proof Constraint: never report a durable move as rejected after publication Confidence: high Scope-risk: narrow Reversibility: revertible Tested: bun test packages/coding-agent/test/move-session-tool.test.ts
Nested eval tool dispatch could deadlock behind a queued cwd writer, construction failures could retain process-cwd ownership, and post-commit prompt or SSH refresh errors could reject an already committed move. Make read leases re-entrant per async owner, release ownership on every startup failure path, and turn post-commit refresh into diagnostic-only work with regressions for each seam. Lore-id: pr4688-fdff-lifecycle-wedges Constraint: nested tool dispatch must not deadlock a fair cwd writer Constraint: failed construction must never poison later process-cwd ownership Constraint: post-commit refresh cannot contradict durable move state Confidence: high Scope-risk: wide Reversibility: revertible Tested: bun test packages/coding-agent/test/move-session-tool.test.ts; bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/sdk-move-cwd.test.ts packages/coding-agent/test/session-manager-resident-cache.test.ts packages/coding-agent/test/notifications-live-stream.test.ts; bun test packages/coding-agent/test/agent-session-python-cleanup.test.ts Not-tested: Windows nested eval lease scheduling
Async lease re-entry must not let a nested call on a different session bypass that manager's queued writer. Bind the reentrant context to the owning SessionManager while preserving same-session eval dispatch. Lore-id: pr4688-lease-owner-scope Constraint: reentrant read leases are session-local Confidence: high Scope-risk: narrow Reversibility: revertible Tested: bun test packages/coding-agent/test/move-session-tool.test.ts
7c72ae0 to
2d721d4
Compare
Current dev's expanded startup surface can push two admission-only tests beyond Bun's five-second default when the file runs concurrently. Keep their assertions unchanged and give only those tests a bounded 15-second budget. Lore-id: pr4688-rebased-test-budget Constraint: timeout changes must not mask a functional failure Confidence: high Scope-risk: narrow Reversibility: revertible Tested: bun test packages/coding-agent/test/move-session-tool.test.ts
|
@probepark — current |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Delta review at exact head bca63cae — approved.
All prior blockers are fixed: same-manager nested cwd read leases are re-entrant without deadlocking behind a writer; process-cwd ownership is released on every pre-AgentSession construction failure; post-commit refresh failures are diagnostic-only and cannot contradict a committed move; and the stale managerless AsyncLocalStorage bypass is fixed by binding re-entry to a per-SessionManager owner symbol.
The regression tests cover nested reader/writer abort, ownership release, prompt-refresh failure, SSH-refresh failure, and manager-specific lease ownership. No new concrete harm or wedge found.
Reviewed by @probepark — method: finite delta/object review from fdff4da2, including the stale 9d4a8f0d managerless fix; no historical restart.
gajae.pr-review-verdict.v1 merge-approved sha256:d2db5bc71c19751224b90f9e9678608cd30d553a2c61a79de3308b2ca8107137 reviewer:human reviewer-id:probepark evidence:exact-head-bca63cae-all-session-move-lifecycle-blockers-fixed
Superseded by the current-dev fix-forward and probepark exact-head approval; both reported findings are addressed in the reviewed head.
3 participants
What
Adds an agent-invokable
move_sessiontool for top-level, unrestricted sessions that have converged from a broad launcher root into one repository.The implementation is ported onto the current
devhead without carrying unrelated old-base changes. It preserves the existing user/ACP move authority while adding a model-facing optional accessor with the same session lifecycle:process.chdirauthority;The current-dev port also refreshes the SDK seam inventory for the renamed
replaceSkillsandretireWorkspaceTreeForRescopeinternals without exposing them as public SDK operations.Why
Sessions launched from a multi-repo root pay a per-turn workspace scan cost and the user-only
/movesurface requires the operator to notice the correct repository first. The model already has the conversation context needed to narrow safely;move_sessionlets it perform that strict descendant-only rescope without widening authority.The tool is deliberately absent for canonical sub-sessions, restricted bash surfaces, caller-owned MCP authority, and frozen/injected workspace-tree sessions where cwd-capturing authority cannot be rebound fail-closed.
Review disposition
All prior live findings from snowykr and probepark were reconciled against the ported current-dev implementation:
chdiridentity is checked;The remaining merge gate is an independent authenticated exact-head approval. No approval is fabricated.
Exact-head fix-forward
Probepark’s exact-head review at
fdff4da25identified three lifecycle wedges. The fix-forward atbca63caedcloses them with discriminating regressions:AgentSessionconstruction failure, and a replacement session can acquire the claim;Testing
Passed locally on exact head
bca63caed:bun test packages/coding-agent/test/move-session-tool.test.ts— 37 pass;bun test packages/coding-agent/test/tools/tool-catalog.test.ts packages/coding-agent/test/tool-discovery/initial-tools.test.ts packages/coding-agent/src/tools/descriptors.test.ts— 42 pass;bun test packages/coding-agent/test/session-manager-resident-cache.test.ts packages/coding-agent/test/notifications-live-stream.test.ts— 24 pass;bun test packages/coding-agent/test/agent-session-python-cleanup.test.ts— 11 pass;bun test packages/coding-agent/test/sdk-operation-inventory.test.ts— 19 pass;bun --cwd=packages/coding-agent run check— pass;bun scripts/check-visible-definitions.ts,bun scripts/verify-g002-gates.ts,bun scripts/rebrand-inventory.ts --strict, andgit diff --check origin/dev...HEAD— pass;bun --cwd=packages/natives run buildand canonicalbun run build— pass.bun test packages/coding-agent/test/sdk-move-cwd.test.ts— 1 pass; platform/tool regression suites — 58 pass.bun run checkreached the SDK closure suite but the repository's pinned rollback manifest expects Bun1.3.14while this workstation runs Bun1.4.0; the focused SDK inventory and package checks pass. This environment mismatch is recorded rather than hidden.GJC verdict
high-riskdev—
[repo owner's gaebal-gajae (clawdbot) 🦞]