feat: clear reviewer context (fresh eyes) on each coder handoff - #2291
feat: clear reviewer context (fresh eyes) on each coder handoff#2291lsm wants to merge 19 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956fa7b315
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES — 2 P1, 5 P2.
The core design is sound and well-documented. I independently verified the central decision: the SDK exposes no in-place clear (clear is only an EXIT_REASONS value in sdk.d.ts:621, plus compact-boundary hooks), so rotating the SDK-internal sdkSessionId to start a non-resuming conversation is the correct — and only viable — mechanism short of piping a /clear string (which the spec forbids). I also confirmed: resume is correctly skipped when sdkSessionId is undefined (query-options-builder.ts:729-735); ensureQueryStarted() is a safe no-op after the internal startStreamingQuery() (no double-start, query-lifecycle-manager.ts:589-611); the new session_id is captured without truncating sdk_messages (sdk-message-handler.ts:767); messageQueue.clear() only drops in-memory unconsumed input and is safe behind the !isBusy guard; and nothing keys UI threading on sdkSessionId (the sdk_messages schema has no sdk_session_id column). New unit/editor tests pass, bun run check is clean, and the 5-space-runtime-a/b + 1-core/agent shards pass.
However, I independently confirmed two P1 integration defects and several P2 propagation gaps. Several of these corroborate the unresolved chatgpt-codex-connector threads — I verified them against primary sources rather than trusting the prior review.
P1
1. Idle-state race on session reuse breaks every 2nd+ reviewer cycle. clearConversationContext() calls stateManager.setIdle() (agent-session.ts:800). setState publishes session.updated unconditionally — there is no same-state guard (processing-state-manager.ts:344-363) — so even an already-idle reused session re-fires the event, and publish awaits subscribers inline (internal-event-bus.ts:9, await Promise.all at :222). On cycle 2+, createSubSession's reuse branch registers a fresh completion callback (task-agent-manager.ts:1046-1051) before the kickoff is injected (:856). That callback fires on any idle with sdkCount > 0 (:2225-2231) and is one-shot (:2233-2243). So the clear's idle publication fires the callback during the clear — prematurely marking the NodeExecution idle (handleSubSessionComplete:2318) before the handoff is even enqueued, then self-unsubscribing. The agent's genuine completion of the new turn later has no callback → completion signaling is lost. This deterministically undermines the feature on every cycle after the first, and the existing unit tests don't catch it (they mock setIdle and don't register a completion callback). Fix: do not emit a client-visible idle during the clear — suppress/defer the publication, move the clear ahead of callback registration, or make the completion detector distinguish a clear-induced idle from a real end-of-turn (e.g. gate on a turn marker / message-count increase rather than raw idle).
2. Built-in workflow change doesn't reach existing spaces. computeWorkflowHash fingerprints only customPrompt.value per agent (template-hash.ts:187-189), not resetContextPerTurn. Adding the flag to the built-in Reviewer yields an identical hash, so the restamp drift gate (built-in-workflows.ts:2206-2207) skips already-installed spaces; and even if restamp ran, mergeNodeStructuralFieldsFromTemplate (:1350-1359) doesn't copy the field. Result: existing spaces with the Coding Workflow never get fresh-eyes on their Reviewer — only new spaces do. The headline use case silently fails for installed spaces. Fix: add resetContextPerTurn to the hash input and to the restamp merge field set.
P2
3. ACP-provider slots silently no-op. ACP sessions set acpSessionId, never sdkSessionId (acp-query-runner.ts), so the !!sdkSessionId gate (task-agent-manager.ts:3291) is always false → the clear never fires; and clearConversationContext only wipes sdk fields, so even if it ran it would not clear ACP context (the trailing startStreamingQuery would resume the same ACP session). Latent (the built-in Reviewer is SDK-based) but the flag is documented as generic/data-driven. At minimum document the SDK-only limitation; ideally extend the gate/primitive to ACP.
4. Export/import drops the flag. export-format.ts:370-384, ExportedWorkflowNodeAgent (space.ts:2458-2513), and the Zod schema enumerate sibling fields but omit resetContextPerTurn → cross-Space export/import loses it.
5. Template picker drops the flag. workflowToTemplate/buildTemplateNodes (workflow-templates.ts) copy field-by-field and omit it (this also drops timeoutMs/toolGuards for daemon templates) → template-created workflows silently lose fresh-eyes.
6. Editor single↔multi conversion drops the flag. NodeConfigPanel.tsx primary-slot construction (≈373-379) and removeAgent (≈273-282) don't carry resetContextPerTurn. The reverse direction is a fresh inconsistency: disabledSkillIds IS promoted but resetContextPerTurn is not.
7. Unguarded resolveNodeAgents on the delivery path. slotResetsContextForSession (task-agent-manager.ts:2597) calls resolveNodeAgents unguarded, and is itself called unguarded in the && at :3292 inside injectMessageIntoSession. resolveNodeAgents throws on a node with empty agents + no legacy agentId (space-utils.ts:49). A mid-flight-edited or corrupted node would drop the handoff instead of falling back to "deliver without clear" — inconsistent with the codebase's own guard pattern (node-agent-tools.ts:513, channel-router.ts:481) and with this PR's own try/catch on the clear invocation. Wrap the lookup (or the whole shouldClearContext evaluation) in try/catch → false.
Not a finding (matches spec intent)
Another reviewer flagged external-event digests being classified 'task' and clearing context. The task spec explicitly accepts this: "GitHub events would inherit the same clear behavior automatically if reviewers ever subscribe — that is a separate subscription decision, out of scope here." So that behavior is intended, not a defect.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bc0b2625c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66f52c1782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6904eec801
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12ddfebf3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES — 1 P2, 1 P3 (down from 2 P1 + 5 P2 last round).
Round 2 is strong. I independently verified every prior finding addressed and all three pushbacks justified.
Verified fixed
- P1 #2 (propagation):
template-hashnow fingerprintsresetContextPerTurn(only when non-empty, so unrelated built-ins don't mass-restamp) andmergeNodeStructuralFieldsFromTemplatecopies it → existing spaces restamp and receive the flag. Plus a boolean validation guard. ✅ - P2 #3 (ACP):
hasPriorContextnow includesacpSessionId;clearConversationContextclearsacpSessionId+acpInstructionsSentfor the ACP provider; the ACP runner finally is also suppressed. ✅ - P2 #4/#5/#6 (export/template/editor): the flag flows through export-format + Zod + import type,
workflowToTemplate/buildTemplateNodes(both branches), andNodeConfigPanelsingle↔multi (both directions). ✅ - P2 #7 (
resolveNodeAgentsguard): wrapped in try/catch →false. ✅ - Input-kind scoping: external-event digests (
agent.message.inject) and hook-failure notices (notifySourceSession) now pass'system'→ no longer clear. ✅ - Inject lock: verified no deadlock/re-entrancy (
clearConversationContextandhandleSubSessionCompletedon't re-inject); FIFO serialization is the intended fix. ✅ - Cost preservation: byte-identical to the normal
reset()path; the consumer atsdk-message-handler.ts:831-858confirms no loss or double-count. ✅
Pushbacks (independently assessed) — all justified
(3c) shared-session slot resolution is clearly correct: getByAgentSessionId returns the in_progress/active execution (SQL-ordered), and the runtime routes handoffs to the active node. (3a) busy-handoff and (3b) post-restart FIFO are genuine narrow latent gaps but correctly out of scope here — 3a's proposed defer wouldn't even work (deferred replay bypasses the inject layer, verified at query-mode-handler.ts:82/119); 3b is a pre-existing shape made deterministic by the lock.
Remaining — the P1 idle-race fix is almost fully closed
P2 — The load-bearing idle-suppression is not tested end-to-end. I confirmed the suppression is load-bearing, not defensive: query() is called with prompt: createMessageGeneratorWrapper() (query-runner.ts:757-760) — an async generator that yields messages across turns — so the SDK subprocess stays alive between turns and queryPromise is non-null at clear time. Therefore lifecycleManager.stop() during the clear really does run the runQuery finally (query-runner.ts:1438), and the && !isClearingConversationContext() guard is what actually prevents the idle publish. But the "P1-1 regression" test (agent-session-clear-context.test.ts:146) mocks lifecycleManager.stop() — it only verifies the flag is armed around the mocked call, never that the real finally reads the flag and skips setIdle. query-runner.test.ts just adds the interface stub (isClearingConversationContext: () => false) without exercising the branch. A future refactor removing the guard at query-runner.ts:1438 / acp-query-runner.ts:787 would pass every test and silently re-open the race; the test's own comment describes the finally mechanism but never runs it. Fix: add a QueryRunner test that drives runQuery to its finally with isClearingConversationContext() === true and asserts stateManager.setIdle is NOT called (and IS called when false) — exercise the actual load-bearing branch, not just the flag lifecycle.
P3 — Boolean-flag suppression has a narrow timing window. lifecycleManager.stop() awaits queryPromise settlement raced against a ~5s timer (query-lifecycle-manager.ts:292). If the SDK subprocess takes >5s to exit after the clear's abort, stop() returns via the timer, clearConversationContext resets the flag, and the late-settling finally sees isClearingConversationContext() === false and publishes idle → the race re-opens. Narrow (an idle subprocess should exit well under 5s) but not impossible. restart() avoids this with a query-generation/stale-query guard; the same pattern here would be robust to timing. Low priority.
Tests pass, bun run check is clean, space-runtime shards pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66a7010371
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — posted as COMMENT only because the PR author is the authenticated user (GitHub rejects APPROVE/REQUEST_CHANGES on your own PR). The verdict is explicitly APPROVE, zero P0–P3 findings.
Round-3 verification complete. All previously raised findings (round-1 P1/P2, round-2 P2/P3) are resolved, and the three propagation follow-ups are correct.
Idle suppression: generation bump replaces the flag (round-2 P2 + P3 — resolved)
The boolean _isClearingConversationContext flag is fully removed (zero references remain across packages/). clearConversationContext now bumps the query generation before stop() (agent-session.ts:831). The old query's runQuery finally checks isStaleQuery first (query-runner.ts:1376, acp-query-runner.ts:756); when stale it skips the entire cleanup block — including stateManager.setIdle() — so the clear no longer publishes a completion-eligible idle that could prematurely fire the node-agent completion callback before the cleared handoff is enqueued.
This is strictly better than the flag:
- Timing-robust (round-2 P3): a late-settling subprocess finally still sees the bumped generation → stale → idle suppressed. A flag would have been reset before a late finally ran.
- Rides on an already-tested guard (round-2 P2):
query-runner.test.ts:1217forcesisStaleQueryand asserts the finally skips cleanup. The new unit test now asserts the precondition (generation bumped beforestop()), not a mocked suppression path.
The cleanup-ownership invariant holds: stop() still closes the subprocess via its queryObject === snapshot check (the stale finally skipped close()), and the fresh query from startStreamingQuery() — which bumps the generation again — owns the next finally. The stop() → startStreamingQuery() sequence is unchanged from round 2.
Follow-ups
- NodeConfigPanel shorthand→explicit promotion now carries
resetContextPerTurn(NodeConfigPanel.tsx:473) — toggling skills on a single-slot node no longer drops the flag. - ACP usage estimate cleared —
acpContextUsageEstimateadded toSessionMetadata(types.ts:607) and reset alongsideacpInstructionsSent(agent-session.ts:859), so a fresh ACP turn doesn't inherit the prior token total. buildTemplateCanvasSignatureincludesresetContextPerTurnon both single- and multi-agent paths (VisualWorkflowEditor.tsx:102,110) — toggling the flag marks the canvas dirty.
Verification
bun run checkclean (lint + typecheck + knip + session/db/test-quality guards).- Daemon:
agent-session-clear-context(7),query-runner(150),acp-query-runner(28) pass. - Web:
serialization,NodeConfigPanel,VisualWorkflowEditor,workflow-templates(233) pass. - 21/21 review threads resolved; PR OPEN, MERGEABLE, targets
dev.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b0d59d7f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES (posted as COMMENT because the PR author is the authenticated user — GitHub rejects REQUEST_CHANGES on your own PR; the verdict is explicitly REQUEST_CHANGES). Two actionable findings (P2) + one optional defensive (P3).
Round-4 env-leak fix — verified correct ✅
The fix at agent-session.ts:833-842 is right: it restores this.originalEnvVars after stop(), mirroring the finally's logic (query-runner.ts:1420-1427) and running before startStreamingQuery() snapshots env. The leak was a genuine round-3 regression — clearConversationContext's "bump generation before stop" makes the old finally stale and skip restoreEnvVars, whereas restart() bumps after stop (old finally runs clean). The new test locks it in. agent-session-clear-context (8), query-runner+acp-query-runner (178) pass; bun run check clean.
I also did a full stale-finally blast-radius audit. stop() already covers startupTimer, abortController, processExitedPromise, queryObject.close, queryPromise/messageQueue (with comments documenting this exact hazard); setIdle is intentionally suppressed; restoreEnvVars is now handled. ACP client.close is covered via stop() closing the adapter (proxyBridge.close() runs unconditionally). restart() is immune (bumps after stop). Two gaps remain:
P2 — resetContextPerTurn: null round-trips into an import failure (anchored)
Validation exempts null, export copies it, but the import schema z.boolean().optional() rejects null. Drop the !== null exemption so validation matches the import schema. Narrow (UI never sends null) but real.
P2 — generation bump suppresses the finally but not the catch (anchored)
The catch runs before the finally; the retry branches (query-runner.ts:1012, 1070+; acp-query-runner.ts:826) call setIdle() with no generation check, guarded only by !isQueryInterrupted. For a clear at an idle turn boundary, stop() killing the subprocess may surface as a process-exit/connection error (not AbortError) → transient → premature idle → completion callback fires before the handoff is enqueued. Please confirm interrupt yields AbortError for the idle-kill case (+ test) or add a getQueryGeneration() guard at the retry setIdle sites.
P3 — _lastConsumedUserMessage is stale but not a live bug (no anchor)
After a completed turn it still references that turn; the stale finally skips its clear; startStreamingQuery reuses the same QueryRunner. Not a live bug — every retry/rate-limit path that reads it requires an API response, which requires the new handoff to be consumed first (by then it's updated). An optional defensive clear next to messageQueue.clear() (:822) would close the window; requires adding a small setter/method on QueryRunner, so feel free to skip if you agree it's unreachable.
Considered and rejected — kickoff injections vs the session lock
The codex-connector flagged spawnWorkflowNodeAgentForExecution (:865) and spawnPostApprovalSubSession (:4359) bypassing withSessionInjectLock. Not a bug for this feature: both inject into freshly-spawned sessions, and the clear gate requires hasPriorContext (!!sdkSessionId || !!acpSessionId, task-agent-manager.ts:3349) — a fresh session has neither, so no clear can occur and there's nothing to interleave. The reuse path (where prior context exists) correctly uses the lock (:1487).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d3b3a72b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bfc996573
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — posted as COMMENT only because the PR author is the authenticated user (GitHub rejects APPROVE on your own PR). Verdict is explicitly APPROVE, zero P0–P3 code findings.
Round-5 fixes all verified. The catch-side idle race (my round-4 P2) is closed with a cleaner design than I'd suggested — a single early-return instead of scattered per-site guards.
Catch-side idle race — fixed ✅
A single if (getQueryGeneration() !== queryGeneration) return; at the top of both catch handlers (query-runner.ts:887, acp-query-runner.ts:818), placed after the isCleaningUp() check but before handleError / error classification / messageQueue.clear / setIdle / retry. A stale query (clear bumped the generation before stop()) now touches no shared state from the catch — no premature idle, no queue clear, no retry, no error broadcast; the finally's existing isStaleQuery guard handles cleanup uniformly. This subsumes the per-site guards and removes the dependence on whether stop()'s subprocess kill surfaces as AbortError vs. a transient error. New test forces a stale query + transient error and asserts no setIdle and no retry.
_lastConsumedUserMessage — fixed ✅
clearLastConsumedUserMessage() on both runners, called from clearConversationContext (agent-session.ts:826) via the active this.queryRunner instance (agent-session.ts:284/409). The fresh turn's retry can no longer replay the prior handoff. New test asserts the clear.
resetContextPerTurn: null — fixed ✅
The !== null validation exemption is removed (space-workflow-manager.ts:737); null is now rejected, matching the z.boolean().optional() import schema. (Minor nit, non-blocking: no dedicated test asserts null → WorkflowValidationError; the round-trip/export tests cover the behavior, but a one-line null-rejection assertion would lock it in.)
Two codex follow-ups — scope-out accepted
Both are narrow timing windows with no data loss that don't affect the built-in Coding Workflow reviewer (whose turns are spaced — the coder waits for feedback before the next handoff):
- Turn-end context refresh: a clear landing between a turn-end result and the fire-and-forget context refresh could publish stale usage or enqueue
/compact. Fix understood (guard the refresh with the query generation). - First-turn provider-init pointer:
system:initlanding during spawn-setup awaits could cause one wasted first-turn restart. Fix understood (gate ongetSDKMessageCount() > 0).
Reasonable to defer with the documented commit to scope separately.
Verification
bun run checkclean (lint + typecheck + knip + session/db/test-quality guards).- Daemon:
agent-session-clear-context+query-runner+acp-query-runner(188) and the space suite (503, incl. export-format, export-import-round-trip, reset-context-per-turn, template-hash, built-in-workflows) pass. - 0 unresolved threads; PR OPEN, MERGEABLE, targets
dev.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR adds a per-slot
Confidence Score: 5/5Safe to merge — the context-clear path degrades gracefully on failure, all gating conditions are covered by tests, and the lock correctly serializes concurrent injects without risk of deadlock. The clear primitive's ordering (generation bump → stop → atomic DB write → restart) is carefully sequenced and matches the existing restart pattern. The promise-chain mutex is correct: tail resolves only after release(), so any concurrent caller waits regardless of whether the prior promise was already settled. InputKind classification exhaustively covers the inject entry points. No pre-existing invariants are broken: NeoKai session IDs are stable, sdk_messages rows are untouched, and the fingerprint change only affects workflows that actually set the flag. Files Needing Attention: No files require special attention — the core logic in task-agent-manager.ts and agent-session.ts is well-tested, and edge cases (empty agents array, clear failure, ACP provider, cost rollup atomicity) all have dedicated test coverage.
|
| Filename | Overview |
|---|---|
| packages/daemon/src/lib/agent/agent-session.ts | Adds clearConversationContext(): stops query with a pre-stop generation bump (so the old finally skips setIdle), wipes SDK/ACP session pointers, atomically persists cost rollup + pointer clear in one DB write, restores provider env, and restarts a fresh query. |
| packages/daemon/src/lib/space/runtime/task-agent-manager.ts | Adds per-session promise-chain serialization (withSessionInjectLock), slotResetsContextForSession() data lookup, and gating in injectMessageIntoSession. InputKind propagation correctly excludes human/system/recovery/external-event paths from triggering clears. |
| packages/daemon/src/lib/agent/query-runner.ts | Adds clearLastConsumedUserMessage() and an early-return stale-query guard in the catch block so a generation-bumped query doesn't retry, clear the queue, or surface errors from an intentional stop. |
| packages/daemon/src/lib/acp/acp-query-runner.ts | Mirrors query-runner.ts: adds clearLastConsumedUserMessage() and the stale-query guard in the catch block for ACP sessions. |
| packages/daemon/src/lib/space/workflows/built-in-workflows.ts | Enables resetContextPerTurn on the Coding Workflow reviewer slot and includes it in mergeNodeStructuralFieldsFromTemplate so re-stamps propagate the flag into installed spaces. |
| packages/daemon/src/lib/space/workflows/template-hash.ts | Adds nodeAgentResetContext to the fingerprint only when non-empty, so workflows without the flag preserve their pre-upgrade hash and avoid mass-restamping. |
| packages/shared/src/types.ts | Adds MessageInputKind ('task' |
| packages/shared/src/types/space.ts | Adds resetContextPerTurn?: boolean to WorkflowNodeAgent and ExportedWorkflowNodeAgent; backward-compatible optional field. |
| packages/web/src/components/space/visual-editor/NodeConfigPanel.tsx | Adds SlotResetContextToggle component and wires it into both single-agent and multi-agent UI paths. |
| packages/daemon/tests/unit/5-space/runtime/reset-context-per-turn.test.ts | Covers all gating cases (task/human/system/first-turn/busy/no-flag/concurrent serialization/empty-agents degradation/lock cleanup). |
| packages/daemon/tests/unit/1-core/agent/agent-session-clear-context.test.ts | Tests the clear primitive directly: pointer wipe, single DB write atomicity, messages preserved, generation bump ordering, ACP state, cost rollup, env restore, and runner last-consumed message cleared. |
| packages/daemon/src/lib/space/managers/space-workflow-manager.ts | Adds API-boundary validation rejecting non-boolean resetContextPerTurn (including null). |
Sequence Diagram
sequenceDiagram
participant C as Caller (node→node handoff)
participant TAM as TaskAgentManager
participant Lock as sessionInjectLock (per-session promise chain)
participant AS as AgentSession
participant SDK as SDK Query Runner
C->>TAM: "injectSubSessionMessage(sid, msg, isSynthetic=true)"
TAM->>TAM: "inputKind = 'task'"
TAM->>Lock: withSessionInjectLock(sid, fn)
Note over Lock: chains on prior promise, sets map[sid]=tail
Lock-->>TAM: acquired (await prev resolved)
TAM->>TAM: slotResetsContextForSession(sid)
Note over TAM: inputKind=task AND idle AND hasPriorContext AND flagSet
TAM->>AS: clearConversationContext()
AS->>AS: incrementQueryGeneration() BEFORE stop()
AS->>SDK: lifecycleManager.stop()
Note over SDK: old finally sees stale generation, skips setIdle()
AS->>AS: wipe sdkSessionId/sdkOriginPath + ACP fields
AS->>AS: db.updateSession(updates) single atomic write
AS->>SDK: startStreamingQuery() fresh empty conversation
AS-->>TAM: done
TAM->>AS: ensureQueryStarted() no-op or recovery retry
TAM->>TAM: db.saveUserMessage() + enqueue handoff
TAM->>Lock: release() map entry self-cleans if last waiter
Reviews (2): Last reviewed commit: "fix: persist clearConversationContext co..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES — one P1 correctness finding (a new idle-race the redesign introduces) plus one P3 stale-comment. The /clear redesign is a genuine improvement over stop→wipe→restart, but its central claim — "there is no query stop, so no generation-bump idle-race to suppress" — does not hold: /clear's own turn emits a completion-eligible idle that fires the once-firing completion callback before the handoff is reviewed.
P1 — In-stream /clear fires the completion callback before the handoff (new idle-race)
The mechanism (traced end-to-end):
- The reuse dispatch path registers a fresh completion callback for the execution turn (
task-agent-manager.ts:1323) before the handoff is injected. The listener subscribes tosession.updatedand fires on the first event carryingprocessingState.status === 'idle'withsdkCount > 0(task-agent-manager.ts:2697-2716). - The handoff inject then calls
clearConversationContext()→messageQueue.enqueue('/clear', true)(agent-session.ts:916), enqueuing/clearahead of the handoff into the persistent query's generator. - Because
/clearis an internal message,createMessageGeneratorWrapperskipssetProcessingfor it (query-runner.ts:1617-1623). The session therefore staysidle(turn boundary) through the/clearturn — it never transitions toprocessing. - The SDK then emits a
resultfor the/clearturn (confirmed by the new online test,clear-context-in-stream.test.ts, which counts/clear's result). That result hitssdk-message-handler.ts:690:if (isSDKResultMessage(message) && !this.usesSessionStateChangedTurnEnd) { await stateManager.setIdle(); }
setIdle()→setState()publishessession.updatedwithprocessingState.status === 'idle'unconditionally — there is no idempotency guard, so an idle→idle "transition" still publishes (processing-state-manager.ts:344,setState).- That publish matches the completion-listener predicate (
processingState.idle+sdkCount > 0— prior turns leftsdkCount > 0, untouched by the internal/clear). The callback fires on/clear's idle, before the handoff has been pulled.
Empirical confirmation of the lynchpin. I wrote a throwaway repro against ProcessingStateManager (since deleted): calling setIdle() on an already-idle state published exactly one session.updated carrying processingState.status === 'idle'. That is the precise event the listener fires on. The idle→idle re-publish is real, not theoretical.
Consequence. handleSubSessionComplete runs early and (a) flips the reviewer execution to idle before it reviews, and (b) consumes the once-firing callback — fired = true + unsubscribe — so the reviewer's real completion after the review is a no-op. The reviewer's session still processes the handoff (the session processing-state and the execution-record status are independent machines), so the review itself is not lost in the common case, and the workflow still advances on send_message. But:
- The execution record is prematurely
idlefor the entire review window, andexecution.status === 'idle'is consumed by multiple runtime-tick paths — notablyactivateRestartRecoveryDownstreamNodes(space-runtime.ts:6552), which treats an idle source-execution as a stalled transition to advance. A daemon restart landing in the/clear-idle window could therefore advance past the reviewer without a review. - More generally, the invariant the redesign rests on ("no idle event escapes the clear") is false, so any future consumer keyed on the completion callback / execution-idle inherits a latent hazard.
Why the tests don't catch it. agent-session-clear-context.test.ts stubs ensureQueryStarted/enqueue and never routes /clear's result through sdk-message-handler, so it cannot observe the idle publish or the callback. clear-context-in-stream.test.ts drives the raw SDK query() and asserts only context-clearing + id rotation — it does not exercise the daemon's completion-listener timing. Neither test covers the callback-during-clear path.
Suggested fixes (pick one, with a regression test that registers a completion callback and asserts it fires only after the handoff's result, not after /clear's):
- Preferred — make
setStateidempotent for no-op transitions (skip the publish whennewStateis observably equal to the current state)./clear's turn never setsprocessing(internal), so itssetIdlebecomes a silent idle→idle and the listener correctly waits for the handoff's genuine processing→idle. Audit callers that might rely on an idle re-broadcast first. - Or gate the completion listener to fire only on an idle that follows actual processing in the same logical turn (track last non-idle status).
- Or suppress the result→
setIdlepath for/clear's turn (e.g. a short-lived "clear in flight" token the result handler checks), mirroring howcreateMessageGeneratorWrapperalready skipssetProcessingfor internal messages.
P3 — Stale comment describes the removed stop→restart design
task-agent-manager.ts:468-472:
"A
resetContextPerTurnclear stops and restarts the SDK query across several awaits; without serialization a concurrent inject …"
Under the in-stream /clear redesign there is no stop/restart (the serialization rationale is still valid — it keeps /clear + handoff enqueue atomic — but the description is now inaccurate). Please update the wording.
What I verified
- PR state: OPEN, MERGEABLE, CLEAN; all checks SUCCESS (SHA
36fcdffdb); 0 unresolved review threads. - Orphan removal is clean (
clearLastConsumedUserMessage/isClearingConversationContextgone);handleSystemInitcapture-on-change is idempotent;pastSdkSessionIdsis capped at 50 with consecutive-dedupe; the cost rollup is a singleupdateSessionwrite; gating keys onsdkSessionIdso the ACP path is a true no-op. All of these are good. - The
resetContextPerTurnvalidation rejects non-boolean values, and the export/import round-trip preserves the flag.
The data-driven per-slot flag, the cost/trace handling, and the ACP gating are all sound. The one thing blocking is the idle-timing interaction above.
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE. The round-6 P1 (in-stream /clear fired the completion callback before the handoff) is fixed and verified; the two P3 stale comments are refreshed. Zero blocking findings.
P1 fix — verified correct
clearConversationContext now arms a one-shot suppressIdleOnNextResult token on the message handler before enqueuing /clear (agent-session.ts:921), releases it on enqueue failure (agent-session.ts:927), and the handler consumes it on /clear's result (sdk-message-handler.ts:937), skipping both the result→setIdle (:713) and finishTurn. The handoff's own genuine processing→idle then completes the turn.
I empirically confirmed this against the live SDK rather than just reading the code. I drove a streaming-input query(), yielded a normal turn, then /clear, then a probe, and logged every message /clear emits:
hook_started → hook_response → init (rotated id) → assistant → result(subtype=success)
Three things this establishes:
/clear's result issubtype=success→handleResultMessageruns → the token is consumed at:937. The suppression fires on exactly the path/cleartakes./clearemits nosession_state_changed→ thehandleSessionStateChangedMessage→finishTurnbypass path is not reachable for/cleartoday./clear'sassistantmessage does not set processing —handleAssistantMessageonly emits tool-use events/metadata, anddetectPhaseFromMessageis a no-op unless alreadyprocessing;createMessageGeneratorWrapperskipssetProcessingfor the internal/clear. So the/clearturn is genuinely idle→idle, which is why the spurious idle was firing and why suppressing it is the right call.
Net: /clear's turn publishes no completion-eligible idle, so the once-firing node-agent completion callback now fires only at the handoff's real processing→idle. The race is closed. (The deferred idempotent-setState fix would also have worked since it's an idle→idle no-op — both approaches are valid; the token has zero blast radius outside /clear's turn, which is a reasonable trade.)
The regression coverage is solid: /clear's result calls setIdle 0× and the next result completes normally; clearConversationContext arms before enqueue and releases on enqueue failure. I re-ran both files locally — 85 pass, 0 fail — and CI is fully green on 54083d748 (all unit + online shards).
P3 stale comments — fixed
Both refreshed: task-agent-manager.ts:470 and :1774 now describe the in-stream /clear mechanism instead of the removed stop→wipe→restart.
Informational note (not a request, not blocking)
The token is consumed inside the success-only handleResultMessage branch (:937). That is sound for /clear because it's a local slash command whose result is empirically subtype=success. If a future SDK ever made /clear emit a non-success result or a session_state_changed-driven turn-end, the token would need broadening (e.g. consume at the :713 guard for any result, and guard handleSessionStateChangedMessage too). Not the case today; flagging only so the assumption is on the record.
State
PR #2291: OPEN, MERGEABLE, CLEAN; all checks SUCCESS (run 30877193482); 0 unresolved review threads; HEAD 54083d748. Ready to merge.
Add an optional per-slot resetContextPerTurn boolean to WorkflowNodeAgent
(next to timeoutMs/toolGuards) so a workflow node can opt into fresh model
context at the start of each handoff. Data-driven — no role-name lookup, no
DB migration (optional JSON field; existing workflows default to off).
Also add MessageInputKind ('task' | 'human' | 'system') to classify injected
inputs at the injection layer so only task inputs (handoffs) can trigger the
clear; human input and system recovery nags are excluded by classification.
Give a workflow node fresh eyes by wiping its SDK model context at the start of each coder handoff, while keeping full NeoKai message history for the UI. - AgentSession.clearConversationContext(): the /clear equivalent. Stops the query, wipes the SDK session pointer (sdkSessionId/sdkOriginPath) in memory and DB, and restarts a fresh conversation with no resume. The SDK assigns a new session_id (captured by SDKMessageHandler); NeoKai's sdk_messages rows, keyed by the stable NeoKai session id, are untouched. Rotating sdkSessionId is required — a stale id would make a later daemon restart resume the pre-clear history, defeating the feature. NeoKai never keys UI threading on sdkSessionId, so the rotation is transparent to the frontend. - TaskAgentManager.injectMessageIntoSession classifies each input (task / human / system) and clears only for task inputs to a slot with resetContextPerTurn, at a turn boundary, when prior context exists. Human input, system recovery nags, the first turn (no sdkSessionId), a busy session, and slots without the flag never clear. - Enable resetContextPerTurn on the built-in Coding Workflow Reviewer slot. The flag is read purely from the workflow definition — no 'reviewer' name special-casing. Coder behavior is unchanged (its slot has no flag).
Add a 'Fresh context each turn' checkbox to the agent-slot section of NodeConfigPanel (single-agent and per-slot in multi-agent nodes), default off, with a help note explaining it wipes the agent's model memory between handoffs while preserving UI history. Persisted onto the agent slot via the same edit/save path as the other slot options; single-agent shorthand serializes through to WorkflowNodeAgent.resetContextPerTurn.
P1-1 idle-state race: clearConversationContext no longer emits a client- visible idle mid-clear. The session is already idle at the turn-start call site and stop()/startStreamingQuery() do not publish one on the normal path, so the previous setIdle() was redundant and would prematurely fire the one- shot node-agent completion callback (re-registered on session reuse) before the agent processed the handoff — deterministically breaking completion detection on every 2nd+ reviewer cycle. Added a regression test asserting no session.updated(idle) is published during the clear. P1-2 template propagation: add resetContextPerTurn to the workflow fingerprint (template-hash) so toggling it on a built-in template is detected as drift, and to mergeNodeStructuralFieldsFromTemplate so the restamp applies it — otherwise existing seeded spaces never receive the flag. P2-4 export/import: carry the flag through ExportedWorkflowNodeAgent (type + Zod schema + serializer + install path). P2-5 template picker: carry it through workflowToTemplate/buildTemplateNodes and the WorkflowTemplate step/slot types. P2-6 editor conversion: preserve it across single<->multi conversion (removeAgent + primarySlot), matching disabledSkillIds. P2-7 delivery-path guard: wrap resolveNodeAgents in slotResetsContextForSession with try/catch so a corrupt/empty-agents node degrades to 'no clear' instead of dropping the handoff. P2-3 ACP: documented that the feature is SDK-only (ACP never sets sdkSessionId, so the guard short-circuits and there is no resumable context to clear). bun run check clean; space-runtime shards + workflow/export/web tests pass.
VeXQA — input-kind too broad: agent.message.inject (external-event digests) and notifySourceSession (hook-failure notices) are synthetic but NOT node→node handoffs, so the 'isSynthetic => task' derivation cleared reset-enabled slots on them, violating the 'only task inputs clear' contract. Added an explicit inputKind override param to injectSubSessionMessage and pass 'system' from both callers; handoff paths (flushPendingMessagesForTarget, send_message delivery, kickoff) keep defaulting to 'task'. VeXQU — ACP support: clearConversationContext now clears the provider- appropriate resumable pointer — sdkSessionId for SDK slots, and acpSessionId + the acpInstructionsSent flag for ACP slots (mirroring the normal reset path's clearAcpSessionStateForReset) — and the injection guard accepts prior context from either pointer. ACP slots with resetContextPerTurn now get fresh context too instead of silently no-oping. Tests: synthetic non-handoff inject does not clear; ACP session state is cleared for ACP-provider slots. bun run check clean; space-runtime shards pass.
…ew P2) VfIz0 — avoid mass restamp: nodeAgentResetContext is now omitted from the workflow fingerprint when no slot has the flag (rather than serialized as []). This keeps the hash stable for every built-in without a reset-enabled slot, so the upgrade does NOT mass-restamp Plan/Research/Review-Only/Fullstack workflows (which would reapply template autonomy/hooks/prompts and overwrite operator edits). Only templates that actually set the flag (Coding Workflow) drift and restamp. VfIz9 — serialize the clear: added a per-session promise-chain mutex around injectSubSessionMessageWithOrigin so a resetContextPerTurn clear (stop → wipe → restart) cannot interleave with a concurrent inject to the same session. injectMessageIntoSession is not re-entrant, so no self-deadlock; holds release in finally. Regression test parks one inject in an async clear and asserts a second concurrent inject waits on the lock instead of delivering. VfIzw (busy-handoff) is addressed by reply, not code — see the review thread: clearing only at turn-start is the spec'd contract, a busy session is mid-turn, and the proposed defer fix would require modifying the shared deferred-replay path that bypasses the injection layer. bun run check clean; space-runtime shards + inject-heavy tests pass.
… flag (review) VfSc3 (P1) — the prior idle-race fix was incomplete: lifecycleManager.stop() awaits the query promise, whose runQuery finally block calls stateManager.setIdle() (generation still current, not cleaning up), publishing a completion-eligible session.updated. Added isClearingConversationContext() on AgentSession, set for the duration of the clear's stop(); the SDK QueryRunner finally (query-runner.ts) and the ACP runner finally (acp-query-runner.ts) now skip setIdle() while it is set. Test asserts the flag is armed precisely during stop() (the prior test mocked stop and hid this). VfSc- (P2) — clearConversationContext now rolls metadata.lastSdkCost into costBaseline (and resets lastSdkCost) before stopping, mirroring reset(), so session cost totals stay accurate across fresh-context turns. VfSdB (P2) — SpaceWorkflowManager.validateNodeAgentRef rejects non-boolean resetContextPerTurn values (network JSON is not type-protected), so persisted config, the editor, and the strict-=== runtime cannot disagree. bun run check clean; agent-layer, ACP, and space-runtime shards pass.
withSessionInjectLock now deletes its map entry when no later caller chained onto it, so sessionInjectLocks doesn't retain one resolved promise per historical session ID (and per rejected nonexistent ID) and grow without bound over a long-running daemon. Standard self-cleaning mutex: each holder compares the map's current tail to its own before deleting. Vfc_I (post-restart FIFO ordering) and Vfc_S (shared-session slot resolution) are addressed by reply, not code — see the threads.
… P2/P3) Replaces the isClearingConversationContext boolean flag with a query-generation bump in clearConversationContext before stop(). The old query's runQuery finally then sees a stale generation and skips its setIdle() via the EXISTING isStaleQuery guard (query-runner.ts:1376 / acp-query-runner.ts:756) — the same mechanism restart relies on. This resolves both review items: - P2 (load-bearing but untested): the suppression is now the existing, already-tested isStaleQuery guard (query-runner.test.ts:1217 asserts a stale query skips the entire finally cleanup block that contains setIdle, so removing the guard would fail that test). clearConversationContext's contribution — bumping the generation before stop — is asserted directly. - P3 (timing window): robust. Even if the subprocess exits after stop()'s termination timeout, the late finally still observes the stale generation and suppresses the idle, unlike a flag that reset on stop()'s return. Reverts the flag (QueryRunnerContext method, AgentSession field/setter, the finally checks in both runners, and the test-harness stubs). bun run check clean; agent-layer, ACP, and space-runtime shards pass.
…ate (review) Round-3 follow-ups on the resetContextPerTurn feature: - SlotSkillsToggle: when a shorthand single-agent node is promoted to an explicit agents[] slot via the skills toggle, copy step.resetContextPerTurn onto the created slot (the last shorthand→explicit conversion path that dropped the flag). - ACP clear: also clear metadata.acpContextUsageEstimate. AcpQueryAdapter seeds a new conversation's usage from it when the provider emits no usage_update, so leaving it started each fresh turn with the prior turn's token total and inflated reported usage. Added acpContextUsageEstimate to SessionMetadata. - Canvas dirty signature: include resetContextPerTurn (shorthand + per-slot) in buildTemplateCanvasSignature so toggling it marks the canvas dirty and the 'Replace current canvas?' confirmation is no longer bypassed. (The generation-bump idle-suppression refactor in the prior commit resolves the round-3 P2/P3 on the load-bearing guard and timing window.) bun run check clean; agent-layer, ACP, and web tests pass.
…VfZKr) The generation-bump that suppresses the clear-time idle also makes the old query's finally skip its originalEnvVars restore (that cleanup only runs for non-stale queries). Without restoring, the cleared provider's env (base URL, credentials, daemon-port vars) leaks into the next query's originalEnvVars snapshot and contaminates later provider setup. clearConversationContext now restores the daemon's original env (and clears the snapshot) right after stop(), before the fresh query applies its own env. Test spies restoreEnvVars to assert it's called with the prior snapshot.
…on (review P2) P2-1 — null validation: dropped the !== null exemption in SpaceWorkflowManager.validateNodeAgentRef so resetContextPerTurn: null is rejected (typeof null !== 'boolean'). This matches the import Zod schema (z.boolean().optional() accepts undefined, not null), closing the validate-ok / export-ok / import-reject inconsistency. P2-2 — catch setIdle during a clear: the generation bump suppresses the finally's setIdle but not the catch's. When stop() kills the subprocess during a clear, the surfaced error can be a transient connection error (not AbortError), hitting a catch retry/terminal branch that calls setIdle with no generation check — firing the completion callback before the cleared handoff is enqueued. Added getQueryGeneration() === queryGeneration guards to every catch setIdle/retry site (SDK: transient/startup-timeout/message-not-found/provider retries + the terminal-catch setIdle; ACP: the retry branch + the terminal catch block). Normal retries are unaffected (the guard holds for non-stale queries). New query-runner test asserts a stale query neither sets idle nor retries from the catch on a transient error. (The optional P3 _lastConsumedUserMessage clear is left as-is — not a live bug, per the review.) bun run check clean; agent-layer, ACP, and both space-runtime shards pass.
The generation bump that suppresses the clear-time idle also makes the old query's finally skip clearing _lastConsumedUserMessage. If the fresh query then hits a retryable error before consuming the new handoff (e.g. a startup timeout), its retry could re-enqueue the previous turn's message into the fresh context. Added clearLastConsumedUserMessage() to both QueryRunner and AcpQueryRunner; clearConversationContext calls it next to messageQueue.clear(). Test verifies the runner's lastConsumedUserMessage is null after a clear.
…e mutation (review) Replaces the scattered per-site getQueryGeneration() guards on the catch's retry/setIdle sites with a single early-return at the top of both runners' catch handlers (right after the isCleaningUp() check). A stale query — one whose generation was bumped by a resetContextPerTurn clear before stop() — now does NOTHING in the catch: no retry, no messageQueue.clear(), no idle, no error surfacing. The error is from the intentional stop; the newer query owns the queue, env, and completion lifecycle. This closes the gap the per-site guards missed (the terminal messageQueue.clear() that could delete a fresh handoff enqueued by the replacement query after a stop() timeout) and is simpler than the scattered guards. The finally's existing isStaleQuery check still handles cleanup uniformly. Error logging is preserved (it runs before the early-return). bun run check clean; agent-layer, ACP, and both space-runtime shards pass.
…restore The clear-env-restore test spied on getProviderService().restoreEnvVars. The test imports provider-service via a deep-relative .ts specifier while the source uses '../provider-service'; a spy on the singleton is fragile to module-identity differences across Bun versions/platforms, which produced a deterministic CI-only 'spy was not called' failure (the test/code under test was byte-identical to the green pre-merge run). restoreEnvVars mutates process.env, a single process global regardless of singleton identity, so observe the effect there. Production behavior is unchanged (it always mutates the real process.env).
…one write Merge the prior-turn cost rollup into the single final updateSession payload alongside the sdkSessionId/ACP pointer clears, instead of a separate early metadata write. The early write was redundant with the final write whenever both fired, and on a mid-clear DB failure it could leave the session with cost rolled but the resume pointer not cleared (a half-applied clear). Also clarifies the deliberate startStreamingQuery + caller ensureQueryStarted redundancy with a comment (Greptile P2). Adds a test asserting the cost roll and pointer clear land in one atomic updateSession call.
…+restart Replace clearConversationContext's stop→wipe→restart dance with the SDK's /clear command, issued in-stream as an internal control message ahead of the triggering handoff. The SDK's pull-based generator serializes /clear before the handoff (it won't pull the next message until the /clear turn completes), so the handoff always runs in the fresh conversation. This drops the most fragile parts of the previous implementation: there is no query stop, so no generation-bump idle-race to suppress (the P1-1 machinery), no provider-env restore, no messageQueue/runner clear, and no manual restart. Empirically verified against the bundled CLI (2.1.179) that /clear in streaming- input mode clears context for the next message and rotates the SDK session id (online regression test in tests/online/lifecycle/). The SDK rotates sdkSessionId itself on /clear; handleSystemInit now captures it on every init where the id changes (was: only when unset), so daemon-restart resume points at the live conversation, not a stale pre-clear one. The prior id is appended to a capped metadata.pastSdkSessionIds trace for audit. Cost handling is unchanged: the prior turn's lastSdkCost is still rolled into costBaseline before the rotation (the result handler's restart-detection is unreliable across /clear — the post-clear cost can be >= the prior). SDK-only: /clear is a Claude-Code command, so resetContextPerTurn on an ACP (codex) slot is now a documented no-op (gated on sdkSessionId). Drops the orphaned clearLastConsumedUserMessage from both query runners.
The in-stream /clear redesign removed the generation-bump idle-race suppression the stop→wipe→restart version had, but /clear's own result re-introduced the race via a different path: /clear is internal so the generator skips setProcessing, leaving the session idle through the /clear turn; its result then hits result→setIdle AND finishTurn, both of which publish processingState.idle unconditionally (idle→idle). That publish matches the completion-callback predicate and fires it before the cleared handoff is reviewed, prematurely marking the execution idle. Fix: clearConversationContext arms a one-shot idle-suppression token on the message handler before enqueuing /clear. The result handler skips both idle publishes (the result→setIdle branch and finishTurn) for that one result, then consumes the token; the handoff's own genuine processing→idle completes the turn. The token is released (not consumed) if the /clear enqueue fails, so the handoff's result still completes normally. Targeted to /clear's turn (zero blast radius on other state consumers); the broader idempotent-setState fix is left as a separate follow-up. Regression test: /clear's result calls setIdle 0 times; the next result completes normally. Plus clearConversationContext arms/releases the token. Also refreshes two stale comments that still described stop→wipe→restart.
dev added resolveNodeExecutionForSubSession (a cancelled/archived task guard) at the top of injectSubSessionMessageWithOrigin. The rebase onto dev replayed this test with its original mock, which only stubbed getByAgentSessionId — so the guard threw listByAgentSessionId is not a function. Stub it to return [] so the guard finds no execution and skips the terminal-task rejection (these tests exercise the clear path, not the guard). getById is never reached: SESSION_ID doesn't parse to an embedded exec id.
54083d7 to
12777ea
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Adds a per-slot
resetContextPerTurnflag (data-driven, no role-name lookup) that wipes an agent slot's SDK model context at the start of each node→node handoff, so each turn starts fresh while NeoKai keeps one continuous UI thread (sdk_messagesare untouched). Enabled on the built-in Coding Workflow Reviewer; coder stays persistent.How the clear works
AgentSession.clearConversationContext()issues the SDK's/clearcommand in-stream as an internal control message ahead of the triggering handoff. The SDK's pull-based streaming-input generator serializes/clearbefore the handoff (it won't pull the next message until the/clearturn completes), so the handoff always runs in a fresh conversation.This replaces an earlier stop→wipe→restart approach and drops its most fragile machinery — there is no query stop, so no generation-bump idle-race to suppress, no provider-env restore, no message-queue/runner clears, and no manual restart. Verified empirically against the bundled CLI (2.1.179) that
/clearin streaming-input mode clears context for the next message and rotates the SDK session id (online regression test intests/online/lifecycle/).The SDK rotates the SDK-internal
sdkSessionIditself on/clear;handleSystemInitnow captures the new id on every init where it changes (was: only when unset), so daemon-restart resume points at the live conversation, not a stale pre-clear one. The prior id is appended to a cappedmetadata.pastSdkSessionIdstrace for audit. NeoKai keys UI threading on its own session id (notsdkSessionId), so the rotation is transparent to the UI.Gating
Cleared only for
taskinputs (node→node handoffs). Never clears for: human input, system recovery nags, the first turn (no priorsdkSessionId), a busy session, or flag-less slots. SDK-only —/clearis a Claude-Code command, so the flag is a documented no-op on ACP (codex) slots until ACP grows an equivalent.Cost is preserved across the rotation: the prior turn's
lastSdkCostis rolled intocostBaselinebefore/clear(the result handler's restart-detection is unreliable here — the post-/clearcost can be ≥ the prior).Tests
AgentSession clear primitive (enqueues
/clear, rolls cost, records audit trace, does not stop/restart),handleSystemInitsession-id rotation, TaskAgentManager gating (handoff clears; human/recovery/first-turn/busy/no-flag do not), editor + serialization round-trip for the toggle, and an online test proving/clearclears context in streaming-input mode.bun run checkclean; daemon shards pass.