Repository navigation
refactor: agent provider + handoff architecture overhaul (E, A, B, D, F) - #543
Merged
Merged
Conversation
…nRequest) Candidate E of the agent provider + handoff overhaul (#538). - IAgentProvider.sendMessage(...15 params) becomes sendTurn(req: TurnRequest<P>). - Per-Provider knobs (contextWindowMode, thinking, codexFastMode, copilotAgent) move into a discriminated providerOptions bag keyed by ProviderId, so generic call sites cannot reach the wrong Provider's knobs. - resumeFrom replaces the setSdkSessionId(...) + resume:true two-step dance. - interactionMode absorbs the retired setPlanQuestionMode (Cursor derives auto-answer suppression per Turn). - setSdkSessionId and setPlanQuestionMode leave IAgentProvider. setPlanAnswerMode also leaves the interface but relocates to ClaudeProvider as a concrete method invoked via cast (like setGoal/clearGoal): the binary interactionMode cannot carry the question-vs-answer ExitPlanMode distinction without changing behaviour. Verified: typecheck 5/5 packages, lint pass, full server unit suite 1064 pass (only snapshot-service git-integration tests time out under sandbox git latency, unrelated to this change and passing at a higher hook timeout).
Candidate A of #538. NarrativeEntry is one chronologically-ordered narrative entry (assistant message body / tool call / narration segment / hook); turn.load returns the ordered list so reloaded turns render in source order, replacing the race-prone message.list + narrative.list hydration pair.
Candidate A of #538. NarrativeStore.load returns one chronologically-ordered NarrativeEntry list (assistant body / tool calls / narration / hooks) by (sequence, sortOrder), with the final-response segment surfaced as the message body. Wired as the turn.load RPC, replacing the race-prone message.list + narrative.list hydration pair. Integration tests cover ordering, final-response surfacing, cross-message sequence, and user/system exclusion.
Candidate A of #538. Thread hydration now fetches the persisted narrative via one server-ordered turn.load call instead of the racy narrative.listBatch pair, grouping entries back into the per-message shape the renderer consumes (the final-response body is skipped — it already lives on the message row). narrative.list stays as the lazy per-message fallback. 88 web unit tests pass.
…ifecycle unit tests Candidate B foundation (#538). Generic per-Provider session lifecycle: pool, lazy 60s idle eviction with a real lastUsedAt+isBusy guard, converged Windows JobObject attachment + taskkill /T /F hard close (acting on PIDs the adapter surfaces from spawn), env snapshot via EnvService, graceful interrupt then close. ProtocolAdapter is the per-Provider protocol I/O seam (spawn/isBusy/ interrupt/close/isStale). 7 unit tests with a fake adapter lock the contract (pool reuse, stale respawn, eviction predicate incl. the busy guard, stop ordering, shutdown timer clear, env pass-through). Provider migrations follow.
…Adapter Candidate B (#538). CodexProvider now implements ProtocolAdapter<CodexSessionState> and holds a SessionRuntime instead of a hand-rolled session pool + TTL-only eviction. Converges the busy-guard drift row: isBusy = pendingTurnId != null, so idle eviction never reaps a mid-turn session. JobObject/taskkill stay delegated to CodexAppServer (it owns the child PID), runtime hard-kill is a no-op for Codex. 124 codex tests pass.
Candidate B (#538). All four providers now hold a SessionRuntime<TState> and implement ProtocolAdapter, replacing hand-rolled pools + eviction + teardown. Drift rows converged: - Copilot gains a real busy guard (turnActive) instead of TTL-only eviction. - Cursor's inline JobObject.assign + taskkill move into the runtime (it surfaces child.pid from spawn); teardown split into interrupt (cancel) / close (cleanup) / runtime hardKill. - Claude gains taskkill via the runtime (spawn surfaces Windows child PIDs by diffing direct children); busy guard (pendingToolUses + pending permission) preserved. - Codex (prior commit) busy guard via pendingTurnId. 321 provider+runtime tests pass.
…e B) Candidate B (#538). CONTEXT.md gains the Session runtime and Protocol adapter glossary entries under Providers; provider-architecture.md documents the shared SessionRuntime<TState>/ProtocolAdapter<TState> lifecycle and the contributor rule (implement the adapter seam, do not hand-roll pools/eviction).
…igration The constructor-bypassing makeProvider() helper set the removed sessions field; runSideChannelQuery now reads parent liveness via this.runtime.get, so the helper provides a Map-backed runtime stub. 3 tests pass.
…didate D) Candidate D (#538). Each Provider exposes a forker: SessionForker; the pipeline reduces to provider.forker.fork(req) with an explicit DeterministicForker cross-forker fallback on classified error/timeout. CleanForker (Claude, path B+ B-prime), MutatingForker (Cursor, path A — kept; F-A clean-resume verification deferred), DeterministicForker (Codex/Copilot + fallback). runSideChannelQuery/ runHiddenTurn leave IAgentProvider (stay as concrete provider methods the forkers call); sessionForkOnResume is now metadata only. Artifact types moved to contracts so the interface can type forker. 53 handoff tests pass incl. a new DeterministicForker unit test.
Candidate F-3 (#538). The deterministic fallback no longer drops everything but a 2000-char last assistant message. The pipeline gathers last_compact_summary, the fork-anchor body, recent tool_call_records + thought_segments, and aggregated files_changed, and passes them on the ForkRequest; runPathDDeterministic renders structured sections (Summary/Recent context, Files changed, Tool activity, Narration highlights, Fork-anchor context), omitting empty ones, no truncation. Deterministic input->output tests across many seeded shapes. 62 handoff tests pass.
Candidate F-core (#538). New ScopedPreGrantService: the handoff pipeline issues a one-shot, path-scoped, Turn-scoped grant so a forked child can Read its handoff doc on its first Turn without prompting, bypassing permissionMode for that single Read. Path-scoped (exact resolved path, no prefix match), one-shot (latched on first consume), Turn-scoped (clear() on the granted Turn's end). 6 unit tests. DI-registered. Wiring into the pipeline + canUseTool follows.
Candidate F-core (#538). The handoff is delivered off-band by default: the full doc is written to a stable OS temp path, the child's inline first-Turn prompt shrinks to a pointer + 2-3 sentence graceful-degradation summary + the user's message, and AgentService issues a ScopedPreGrant so the child Reads that one file on its first Turn without prompting. ClaudeProvider.canUseTool consumes the grant (path-scoped, one-shot) before any permission_request; the grant is cleared on the Turn's end (turn-scoped). Full mode / Minimal mode / Character budget / Overflow retire (HandoffMode narrowed to "full"); maxInputCharactersPerTurn is now decorative for handoff delivery. New test: a pre-granted Read auto-allows with zero permission prompts; non-granted paths still prompt.
…Path A retirement note Candidate F (#538). CONTEXT.md: Handoff rewritten for off-band-by-default delivery; new Scoped pre-grant term; Modes and budgets retitled to Handoff delivery (Full/ Minimal/Character budget/Overflow retired); B/A/D ladder gains a Path A retirement- pending note; sessionForkOnResume + per-turn input cap marked metadata/decorative.
…arrativeStore Candidate A part 2 (#538). The narrative write seam now lives in NarrativeStore alongside the read seam: parentToolCallId enrichment (SDK-first + exactly-one- running-Agent stack fallback), AssistantMessageBoundary classification, the is_final_response suffix tag, and tool_call/thought/hook persistence move out of the 2689-line agent-service.ts. Per-turn buffers + agentCallStack move into the store; AgentService delegates and keeps turn-level concerns (snapshots, files_ changed, turn.persisted broadcast, late hooks, TodoWrite). index.ts enrichment calls narrativeStore.getCurrentParentToolCallId. Behavior-neutral: all six narrative traps preserved, +13 write-seam integration tests; 145 tests pass.
…load cutover Candidate A cleanup (#538). The eager batch-prefetch RPC was orphaned when hydration switched to the single ordered turn.load call (prefetchNarratives now calls loadTurn). Removes the unused narrative.listBatch end-to-end: contracts WS method schema, ws-router handler, ws-transport client method, both transport interface decls, and the test mocks. narrative.list (singular, lazy per-message fallback) and NarrativeBatchResult are still used and stay. typecheck 5/5; web hydrator/narrative suites green.
📝 WalkthroughWalkthroughReplaces provider sendMessage with typed sendTurn and adds SessionRuntime/ProtocolAdapter. Introduces NarrativeStore with turn.load, off-band handoff via SessionForker and ScopedPreGrant, updates contracts/WS methods, DI, web hydration, and many tests. ChangesContracts and RPC surface Provider TurnRequest and options typing NarrativeEntry/TurnRange schemas and WS methods SessionForker and handoff contract types SessionRuntime and provider refactors SessionRuntime core and tests Provider refactors → ProtocolAdapter + sendTurn Narrative pipeline, AgentService refactor, and turn.load hydration NarrativeStore (read/write seams) and tests AgentService delegates to NarrativeStore and off-band handoff Server DI, WS routing, and web hydration Handoff pipeline, deterministic Path D, and ScopedPreGrant Handoff orchestration and forkers Handoff prompt and types ScopedPreGrant service and integration tests Docs and context Estimated code review effort Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/agent-service.ts (1)
1366-1369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlan-mode forks currently block the first-turn handoff read.
buildPlanPrompt(...)tells the child not to use tools or read files, but the off-band handoff path only works if the child performs aReadon the temp file first. For branched threads created withinteractionMode === "plan", the model only gets the short inline summary instead of the full handoff context. Either inline the handoff for plan forks, or use a plan-specific wrapper that explicitly allows the one pre-grantedReadbefore the no-tool rule applies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/agent-service.ts` around lines 1366 - 1369, The plan-mode branch sets providerInput using buildPlanPrompt(providerWireOverride), but buildPlanPrompt enforces "no tools/read files" so branched threads never perform the required initial Read of the handoff temp file; this causes first-turn handoff reads to be blocked. Fix by ensuring plan forks receive the full handoff before the no-tool restriction: either inline the handoff content into the plan prompt when interactionMode === "plan" (compose providerWireOverride with the handoff body and pass that into buildPlanPrompt), or create a plan-specific wrapper (e.g., buildPlanPromptAllowOneRead) that prefixes a short allowance telling the child to perform a single Read of the temp file before applying the no-tool rule; update the providerInput assignment to use the chosen approach so the child can do the pre-granted Read on first turn.
🧹 Nitpick comments (1)
apps/server/src/services/session-runtime.ts (1)
179-181: ⚡ Quick winCatch eviction sweep failures to avoid unhandled async rejections.
The interval callback fire-and-forgets
evictIdle(). IfisBusy()/stop()throws unexpectedly, the rejection is unobserved.Safe interval callback pattern
private ensureEvictionTimer(): void { if (this.evictionTimer) return; this.evictionTimer = setInterval(() => { - void this.evictIdle(); + void this.evictIdle().catch((err) => { + logger.warn("SessionRuntime eviction sweep failed", { error: errMsg(err) }); + }); }, EVICTION_INTERVAL_MS); this.evictionTimer.unref?.(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/services/session-runtime.ts` around lines 179 - 181, The interval currently fire-and-forgets this.evictIdle() which can produce unhandled promise rejections if isBusy()/stop() throw; change the setInterval callback to invoke evictIdle within a try/catch (e.g. make the callback an async IIFE or call .catch) and log the error instead of letting it bubble (use existing logger like this.logger.error or fallback to console.error), keeping EVICTION_INTERVAL_MS and evictionTimer usage unchanged; ensure any thrown errors from evictIdle(), isBusy(), or stop() are caught and recorded to avoid unobserved rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/__tests__/copilot-provider.test.ts`:
- Around line 305-310: The helper waitForEnded(events: AgentEvent[]) currently
retries up to 50 times then returns silently, which masks stalled tests; modify
waitForEnded to throw an Error when the loop completes without finding an
event.type === "ended" so tests fail fast and deterministically (update the
function named waitForEnded to throw a descriptive error like "waitForEnded
timed out waiting for 'ended' event" after the retry loop finishes).
In `@apps/server/src/providers/claude/claude-provider.ts`:
- Around line 826-847: The reuse check omits ClaudeSessionState.cwd, so update
the reusable determination to also compare existing.cwd === cwd (i.e., reusable
= existing !== undefined && existing.permissionMode === permissionMode &&
existing.contextWindowMode === contextWindowMode && existing.cwd === cwd), and
when logging the recreation include cwd: { from: existing.cwd, to: cwd }; keep
the same teardown flow (set existing.suppressEnded = true and await
this.runtime.stop(sessionId)) so the subprocess is recreated when cwd changes
and ensure any other staleness logic (e.g., isStale/acquire) is consistent with
the new cwd check.
- Around line 1146-1206: The resume waiter (retryPromise) never gets signaled on
a healthy resume because `_streamDone:${sessionId}` is only emitted on teardown;
fix by emitting the doneEvent when a resume successfully starts so retryPromise
resolves. Specifically, after successful acquire/recording of the runtime
(references: retryPromise, failedEvent/doneEvent, this.runtime.acquire,
this.runtime.recordUsage, pendingStops), emit the doneEvent
(this.emit(doneEvent)) to resolve the doneHandler path and allow runSpawn(true)
/ sendTurn() to proceed.
In `@apps/server/src/providers/codex/codex-provider.ts`:
- Around line 462-467: The pooled session reuse logic misses changes to working
directory—update the CodexSessionState to persist the cwd when sessions are
created/registered and modify isStale(state: CodexSessionState, args: { cwd:
string; permissionMode: string }) to return true if state.server.isAlive is
false OR state.sandboxMode differs OR state.cwd !== args.cwd; ensure wherever
new CodexSessionState instances are constructed the current cwd is stored so the
comparison is valid.
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 597-600: When detecting that a stop won the spawn race (if
(this.pendingStops.has(sessionId))), drop the just-cached Copilot session before
returning: perform the same teardown that spawn() uses by removing the cached
state for sessionId from the pool and invoking/awaiting the state's teardown
(e.g. call the existing session cleanup method or
state.disconnect()/state.dispose()) so isStale() cannot leave a dead session in
the pool, then return.
In `@apps/server/src/providers/cursor/cursor-provider.ts`:
- Around line 379-390: spawn() currently calls spawnChild() then
openLogicalSession(), but if openLogicalSession(state, ...) throws the spawned
child is leaked; wrap the openLogicalSession call in a try/catch, and in the
catch perform deterministic cleanup of the newly spawned process (e.g.
kill/close the process handle on state.child, await any async teardown helper if
one exists), ensure liveSessionIds does not keep the session (remove it if
added), then rethrow the original error; reference the spawn(), spawnChild(),
openLogicalSession(), state.child and liveSessionIds symbols to locate where to
add the try/catch and cleanup.
In `@apps/server/src/services/agent-service.ts`:
- Around line 1225-1235: The temp handoff file is created at handoffTempPath
using writeFile(..., "utf8") with a predictable name and default open
flags/permissions and the pre-granted Read is issued via
this.scopedPreGrant.issue; update creation to use a private temp dir and a
cryptographically-random filename, create the file with exclusive creation flags
(wx) and mode 0o600 (owner-only) to avoid symlink/truncation attacks, and write
atomically; additionally reconcile plan-mode behavior by making the
interactionMode check around buildPlanPrompt vs buildOffBandHandoffPrompt
explicit: if interactionMode === "plan" ensure the prompt passed (via
buildPlanPrompt or deriveHandoffSummary) does not instruct the model to call
Read (or alternatively change plan-mode constraints to allow the pre-granted
Read), and ensure the pre-grant (this.scopedPreGrant.issue) is only issued when
the prompt and interactionMode actually expect the model to perform a Read
(i.e., issue pre-grant for Read together with calls to buildOffBandHandoffPrompt
and not when using buildPlanPrompt/deriveHandoffSummary).
In `@apps/server/src/services/handoff/path-d-deterministic.ts`:
- Around line 143-156: The metadata object is embedded directly into the handoff
comment via JSON.stringify in the markdown construction (see metadata,
HANDOFF_MARKER and markdown), which allows user/project strings to include the
comment terminator "-->" and break parsing; before building markdown, serialize
metadata then sanitize or encode any occurrences of the comment terminator (for
example replace "-->" with a safe escaped sequence such as "\u002d\u002d\u003e"
or otherwise encode the entire metadata string), and use that sanitized string
when composing markdown so the embedded JSON cannot prematurely close the HTML
comment.
In `@apps/server/src/services/session-runtime.ts`:
- Around line 95-127: Concurrently calling acquire can cause duplicate spawns
because both callers miss this.sessions.get and both call this.adapter.spawn; to
fix, implement a single-flight guard inside acquire keyed by args.sessionId
(either by adding a pending promise map or by storing a temporary "in-flight"
entry in this.sessions) so the first caller performs the spawn and subsequent
callers await that in-flight promise; ensure you check this.adapter.isStale and
call this.stop only once, create the in-flight placeholder before calling
this.adapter.spawn, on successful spawn replace the placeholder with the real {
state, pids, lastUsedAt } entry and run the Windows jobObject assignment, and on
spawn failure remove the placeholder and propagate the error so no unmanaged
processes remain (reference functions/fields: acquire, this.sessions,
this.adapter.spawn, this.stop, ensureEvictionTimer, this.deps.jobObject).
In `@packages/contracts/src/models/narrative-entry.ts`:
- Around line 17-55: Wrap the two non-trivial Zod schemas with the repo's
lazySchema pattern: replace the exported constants NarrativeEntrySchema and
TurnRangeSchema with lazySchema(() => ... ) wrappers (e.g., NarrativeEntrySchema
= lazySchema(() => z.discriminatedUnion(...)) and TurnRangeSchema =
lazySchema(() => z.object(...))) so they are constructed on first use and
callable as functions (NarrativeEntrySchema() / TurnRangeSchema()). Also update
the NarrativeEntry type to infer from the lazy schema function return (e.g.,
z.infer<ReturnType<typeof NarrativeEntrySchema>>) so consumers use the
function-style access used across packages/contracts.
---
Outside diff comments:
In `@apps/server/src/services/agent-service.ts`:
- Around line 1366-1369: The plan-mode branch sets providerInput using
buildPlanPrompt(providerWireOverride), but buildPlanPrompt enforces "no
tools/read files" so branched threads never perform the required initial Read of
the handoff temp file; this causes first-turn handoff reads to be blocked. Fix
by ensuring plan forks receive the full handoff before the no-tool restriction:
either inline the handoff content into the plan prompt when interactionMode ===
"plan" (compose providerWireOverride with the handoff body and pass that into
buildPlanPrompt), or create a plan-specific wrapper (e.g.,
buildPlanPromptAllowOneRead) that prefixes a short allowance telling the child
to perform a single Read of the temp file before applying the no-tool rule;
update the providerInput assignment to use the chosen approach so the child can
do the pre-granted Read on first turn.
---
Nitpick comments:
In `@apps/server/src/services/session-runtime.ts`:
- Around line 179-181: The interval currently fire-and-forgets this.evictIdle()
which can produce unhandled promise rejections if isBusy()/stop() throw; change
the setInterval callback to invoke evictIdle within a try/catch (e.g. make the
callback an async IIFE or call .catch) and log the error instead of letting it
bubble (use existing logger like this.logger.error or fallback to
console.error), keeping EVICTION_INTERVAL_MS and evictionTimer usage unchanged;
ensure any thrown errors from evictIdle(), isBusy(), or stop() are caught and
recorded to avoid unobserved rejections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15911aec-0d28-48ab-90ca-813a408b9486
📒 Files selected for processing (57)
CONTEXT.mdapps/server/src/__tests__/agent-service-turn-started.test.tsapps/server/src/__tests__/claude-provider-assistant-boundary.test.tsapps/server/src/__tests__/claude-provider-eviction.test.tsapps/server/src/__tests__/claude-provider-permission-mode.test.tsapps/server/src/__tests__/claude-provider-queue.test.tsapps/server/src/__tests__/claude-provider-result-error.test.tsapps/server/src/__tests__/claude-provider-scoped-pre-grant.test.tsapps/server/src/__tests__/copilot-provider.test.tsapps/server/src/__tests__/model-cache-service.test.tsapps/server/src/container.tsapps/server/src/index.tsapps/server/src/providers/claude/__tests__/side-channel-sessionless.test.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/providers/codex/__tests__/codex-provider-permission.test.tsapps/server/src/providers/codex/codex-provider.tsapps/server/src/providers/copilot/copilot-provider.tsapps/server/src/providers/cursor/cursor-provider.tsapps/server/src/services/__tests__/agent-service-gate.test.tsapps/server/src/services/__tests__/agent-service-goal-command.test.tsapps/server/src/services/__tests__/agent-service-narrative-persist.test.tsapps/server/src/services/__tests__/agent-service-plan-marker.test.tsapps/server/src/services/__tests__/agent-service-snapshot-flag.test.tsapps/server/src/services/__tests__/agent-service-stack-fallback.test.tsapps/server/src/services/__tests__/agent-service-turn-cleanup.test.tsapps/server/src/services/__tests__/narrative-store.test.tsapps/server/src/services/__tests__/scoped-pre-grant.test.tsapps/server/src/services/__tests__/session-runtime.test.tsapps/server/src/services/agent-service.tsapps/server/src/services/handoff/__tests__/fork-flow.e2e.test.tsapps/server/src/services/handoff/__tests__/handoff-pipeline.test.tsapps/server/src/services/handoff/__tests__/handoff-prompt.test.tsapps/server/src/services/handoff/__tests__/path-d-deterministic.test.tsapps/server/src/services/handoff/__tests__/session-forker.test.tsapps/server/src/services/handoff/handoff-pipeline.tsapps/server/src/services/handoff/handoff-prompt.tsapps/server/src/services/handoff/handoff-types.tsapps/server/src/services/handoff/path-d-deterministic.tsapps/server/src/services/handoff/session-forker.tsapps/server/src/services/narrative-store.tsapps/server/src/services/scoped-pre-grant.tsapps/server/src/services/session-runtime.tsapps/server/src/transport/ws-router.tsapps/web/src/__tests__/mocks/transport.tsapps/web/src/lib/thread-hydrator/__tests__/thread-hydrator.test.tsapps/web/src/lib/thread-hydrator/thread-hydrator.tsapps/web/src/lib/thread-hydrator/types.tsapps/web/src/transport/types.tsapps/web/src/transport/ws-transport.tsdocs/guides/narrative-pipeline.mddocs/guides/provider-architecture.mdpackages/contracts/src/index.tspackages/contracts/src/models/narrative-entry.tspackages/contracts/src/providers/__tests__/turn-request.type-test.tspackages/contracts/src/providers/interfaces.tspackages/contracts/src/providers/session-forker.tspackages/contracts/src/ws/methods.ts
chuks-qua
changed the title
…, refactor-safe) - session-runtime: dedup concurrent acquire() so two racing turns can't spawn two processes for one session (+ unit test). - claude: a healthy cold resume now releases the runSpawn(true) waiter via a new _resumeOk signal (was hanging until session teardown since _streamDone only fires on exit); _resumeFailed still drives the sessionless fallback. - cursor: kill the ACP child if openLogicalSession throws (no leak when the runtime never received the pid). - copilot: evict the pooled session when a stop wins the spawn race (dead session could otherwise be reused). - handoff Path-D: escape comment terminators in the embedded metadata JSON so a '-->' in user/project strings can't close the HANDOFF_MARKER comment early. - contracts: wrap NarrativeEntrySchema + TurnRangeSchema in lazySchema per repo convention; call sites invoke them as functions. - test: copilot waitForEnded throws on timeout instead of returning silently. Deferred (pre-existing / scope): cwd in Claude+Codex reuse checks, handoff temp file hardening + plan-mode pre-grant reconciliation.
Contributor
Author
|
Thanks @coderabbitai. Triaged against the goal of this PR (a behavior-preserving refactor). Addressed the defects/regressions + safe items in Fixed
Deferred (intentionally out of scope for this refactor; not introduced by it)
Verification: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/providers/claude/claude-provider.ts (1)
1191-1205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid emitting
Endedtwice on the pending-stop path.
spawn()already startedstartStreamLoop(), andawait this.runtime.stop(sessionId)will unwind that query. The stream loop’sfinallyemits the normal terminalEnded, so this branch currently emits a second one for the same cancelled turn.Suggested fix
removeResumeListeners(); await this.runtime.stop(sessionId); - // TurnStarted was already emitted by AgentService before calling the - // provider, so the frontend thinks the agent is running. Emit Ended - // to clear that state. - this.emit("event", { - type: AgentEventType.Ended, - threadId: tid, - } satisfies AgentEvent); return "stopped";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/providers/claude/claude-provider.ts` around lines 1191 - 1205, The pending-stop branch in spawn() currently calls await this.runtime.stop(sessionId) and then emits an Ended event, but startStreamLoop() will also emit Ended in its finally block causing a duplicate; remove the explicit this.emit("event", { type: AgentEventType.Ended, threadId: tid }) from the pendingStops.delete(sessionId) branch (keep removeResumeListeners() and the await this.runtime.stop(sessionId) so the runtime unwinds and the stream loop emits the terminal Ended). Ensure you only return "stopped" after stopping without emitting Ended here.
♻️ Duplicate comments (1)
apps/server/src/providers/copilot/copilot-provider.ts (1)
597-609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume the pending stop here.
This now evicts the dead pooled session, but keeping
sessionIdinpendingStopsmeans every new turn on the same thread within the 10-second expiry window is treated as the old cancelled spawn and gets torn down again. Usedelete()so the stop only applies once.Suggested fix
- if (this.pendingStops.has(sessionId)) { + if (this.pendingStops.delete(sessionId)) { void this.runtime.stop(sessionId).catch((err: unknown) => logger.debug("CopilotProvider: stop of pending-stop session failed", { sessionId, error: err instanceof Error ? err.message : String(err), }), ); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 597 - 609, The pending stop is checked but not removed, causing subsequent turns to be wrongly treated as cancelled; update the block in CopilotProvider (the pendingStops handling around this.pendingStops.has(sessionId)) to remove the sessionId from pendingStops (call this.pendingStops.delete(sessionId)) before invoking this.runtime.stop(sessionId) so the stop is consumed once, then proceed to call runtime.stop and return as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/server/src/providers/claude/claude-provider.ts`:
- Around line 1191-1205: The pending-stop branch in spawn() currently calls
await this.runtime.stop(sessionId) and then emits an Ended event, but
startStreamLoop() will also emit Ended in its finally block causing a duplicate;
remove the explicit this.emit("event", { type: AgentEventType.Ended, threadId:
tid }) from the pendingStops.delete(sessionId) branch (keep
removeResumeListeners() and the await this.runtime.stop(sessionId) so the
runtime unwinds and the stream loop emits the terminal Ended). Ensure you only
return "stopped" after stopping without emitting Ended here.
---
Duplicate comments:
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 597-609: The pending stop is checked but not removed, causing
subsequent turns to be wrongly treated as cancelled; update the block in
CopilotProvider (the pendingStops handling around
this.pendingStops.has(sessionId)) to remove the sessionId from pendingStops
(call this.pendingStops.delete(sessionId)) before invoking
this.runtime.stop(sessionId) so the stop is consumed once, then proceed to call
runtime.stop and return as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d592a444-3a48-46a0-9cf7-02ad4df26f19
📒 Files selected for processing (10)
apps/server/src/__tests__/copilot-provider.test.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/providers/copilot/copilot-provider.tsapps/server/src/providers/cursor/cursor-provider.tsapps/server/src/services/__tests__/session-runtime.test.tsapps/server/src/services/handoff/__tests__/path-d-deterministic.test.tsapps/server/src/services/handoff/path-d-deterministic.tsapps/server/src/services/session-runtime.tspackages/contracts/src/models/narrative-entry.tspackages/contracts/src/ws/methods.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/contracts/src/models/narrative-entry.ts
- apps/server/src/services/handoff/tests/path-d-deterministic.test.ts
- apps/server/src/tests/copilot-provider.test.ts
- apps/server/src/services/tests/session-runtime.test.ts
- apps/server/src/services/handoff/path-d-deterministic.ts
- packages/contracts/src/ws/methods.ts
- apps/server/src/providers/cursor/cursor-provider.ts
|
The deferral rationale for both items is sound:
The verification results (typecheck 5/5, lint clean, suites green, known flake isolated to 🐇 ✨ ✅ Actions performedComments resolved. Approval is disabled; enable |
This was referenced Jun 4, 2026
Merged
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Lands the five-candidate agent provider + handoff overhaul from the architecture review (E, A, B, D, F) onto one branch. Largely a refactor: it deepens module boundaries and converges Provider behaviour without changing the product surface, plus two user-facing fixes (off-band handoff delivery and page-load narrative ordering).
Why
The agent layer had a 2,689-line
AgentService, ~70% duplicated session-lifecycle code across the four Providers (with accidental drift), a 15-parameter per-Turn Provider call that leaked every Provider's knobs to every call site, a handoff capped by the child's per-Turn input budget, and a page-reload bug where Tool calls rendered before the assistant message. See #538 for the full PRD.Key Changes
IAgentProvider.sendMessage(...15 params)becomessendTurn(req: TurnRequest<P>). Per-Provider knobs ride in a discriminatedproviderOptionsbag;resumeFromreplaces thesetSdkSessionId+resume:truedance;interactionModeabsorbssetPlanQuestionMode. (setPlanAnswerModerelocated off the interface toClaudeProvideras a concrete method - the binaryinteractionModecannot carry the question-vs-answer ExitPlanMode distinction; documented in-PR.)AssistantMessageBoundaryclassification, persistence - moved out ofAgentService) and the read seam (one orderedturn.loadRPC). The client hydrates from that single ordered source, fixing the page-load render-ordering bug. All six narrative-pipeline traps preserved + covered by new integration tests.SessionRuntime<TState>and implementProtocolAdapter<TState>(composition). Converges the accidental drift: Codex/Copilot gain a realisBusyguard, all Providers gainJobObjectattachment + Windowstaskkill /T /Fvia the runtime.provider.forker.fork(req)withCleanForker/MutatingForker/DeterministicForkerand an explicit deterministic cross-forker fallback.runSideChannelQuery/runHiddenTurnleaveIAgentProvider.ScopedPreGrant, and the deterministic Path-D builder now composes fromlast_compact_summary, tool-call records, narration segments, files changed, and the fork-anchor body. Full/Minimal mode, Character budget, and Overflow retire.Session runtime,Protocol adapter,Scoped pre-grant, off-bandHandoff,Handoff delivery, and the Path-A retirement-pending note.CONTEXT.md terms implemented
Turn /
TurnRequest, Plan mode + Build mode (per-Turn), Provider, Session runtime, Protocol adapter, Handoff (off-band), Scoped pre-grant, Handoff delivery, the B/A/D ladder (now forker adapters), Path D (deterministic builder).User stories satisfied (from #538)
8, 9, 10 (narrative ordering/trail) - A; 11, 12, 20, 21, 22 (per-Turn knobs, typed call, providerOptions, resumeFrom) - E; 13-17, 19, 25, 28 (resume, Windows cleanup, idle eviction, uniform lifecycle, isolated lifecycle tests, drift convergence) - B; 23, 26 (typed forker selection, isolated deterministic builder) - D; 1-7, 24, 27, 29 (full off-band context, scoped pre-grant, deterministic fidelity, single read seam, CONTEXT.md) - A+F. Story 18/30 (Cursor clean-resume / Path-A retirement) is deferred - see below.
Verification
bun run verifyfast gate (typecheck 5/5 packages + lint): passes.snapshot-service.integrationtests, which time out at the 10s hook limit under sandbox git latency (unrelated to this work; they pass at a 60s timeout).Deferred / follow-ups (not blockers)
MutatingForkeris kept (the PRD's safe default) and marked retirement-pending in CONTEXT.md.Closes #538
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Improvements