feat: harness with session manager - #251
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR shifts console conversation state, streaming, and compaction to session-manager-backed session reads and writes. Harness turn orchestration, compaction, and session persistence are rewritten around the same API, and the removed session-tree surface is reflected in tests, docs, and permissions. ChangesSession-manager migration
Sequence Diagram(s)sequenceDiagram
participant ChatView
participant TurnOrchestrator
participant SessionManager
participant ContextCompaction
ChatView->>TurnOrchestrator: run::start with message_id
TurnOrchestrator->>SessionManager: session::ensure / session::append / session::set_status
TurnOrchestrator->>SessionManager: session::messages / session::update_message
ContextCompaction->>SessionManager: session::messages / session::append / session::update_message
SessionManager-->>ChatView: transcript snapshots and status changes
Changes🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
skill-check — worker0 verified, 15 skipped (no docs/).
Four for four. Nicely done. |
sergiofilhowz
marked this pull request as ready for review
June 12, 2026 12:13
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
console/web/src/components/chat/ChatView.tsx (1)
339-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
fcallMapcan point at rows that no longer exist.Once the session-events reconciler replaces the locally appended function-call row with the entry-derived segment,
messagesRef.currentstops containing the olduid(). Lines 391-423 still prefer the stalefcallMaphit, sofcall-end/fcall-approval-clearedcan patch a deleted id and leave the visible row without its output update or withpendingApprovalstill set. Validate that the mapped id still exists before using it, or rewrite the map when reconciliation swaps ids.🤖 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 `@console/web/src/components/chat/ChatView.tsx` around lines 339 - 423, The fcallMap may contain stale message ids after the session-events reconciler replaces locally appended function-call rows, causing handlers like the 'fcall-end' and 'fcall-approval-cleared' cases to patch nonexistent ids; update the logic in the 'fcall-end' and 'fcall-approval-cleared' branches to first verify that any id found in fcallMap actually exists in messagesRef.current (or re-resolve the current id by searching messagesRef.current for the same functionCallId) before calling onPatchMessage, and when reconciliation replaces a local uid with an entry-derived id, update fcallMap (or remove the stale mapping) so fcallId / fcallMap always point to an existing message id.harness/docs/workers/turn-orchestrator.md (1)
133-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the documented dependency name to
session-manager.Line 135 still tells readers this worker depends on
session ^0.2.0, but the rest of this migration rewires the orchestrator to the externalsession-managerworker. Following this section as written would wire the wrong dependency for standalone bring-up. Based on PR objectives: the session backend was migrated fromsession-tree/in-harness session handling tosession-manager.🤖 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 `@harness/docs/workers/turn-orchestrator.md` around lines 133 - 136, The documentation still references the old dependency name "session ^0.2.0"; update the dependency listing in the turn-orchestrator documentation to "session-manager ^0.2.0" so the orchestrator is wired to the external session-manager worker as intended—replace the literal `session ^0.2.0` entry in the dependency list/markdown block (the same block that also lists `provider-anthropic ^0.2.0` and `provider-openai ^0.2.0`) with `session-manager ^0.2.0` and run a quick spell-check to ensure no other occurrences remain.harness/src/index.ts (1)
36-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
start:allno longer brings up the session backend the harness now requires.
WORKERSdrops the in-process session worker, but the migrated runtime paths now callsession::ensure,session::append,session::messages, andsession::set_statuson the happy path. That meansnode dist/index.js/pnpm dev:allcan come up successfully and still fail every transcript operation unless a separatesession-managerworker is already running on the same bus.Please either register
session-managerin this composite runner too, or add a startup-time dependency check that fails fast with a clear error instead of leaving the local stack half-wired.🤖 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 `@harness/src/index.ts` around lines 36 - 116, The composite WORKERS list no longer includes the in-process session manager so runtime calls to session::ensure/append/messages/set_status can silently fail; fix by either adding a session-manager entry to WORKERS (e.g., add an object with name 'session-manager' and register: (iii, ctx) => registerSessionManager(iii, ctx) or registerSessionManager(iii) depending on your API) so the harness brings up the session backend, or add a startup dependency check in the bootstrap (where WORKERS is consumed / app initialization runs) that calls the session RPCs (session::ensure or a lightweight health-check like session::ping) and fails fast with a clear error if no session-manager is present.harness/tests/context-compaction/e2e/full-session.test.ts (1)
347-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
deletewhen restoringCOMPACT_RESERVED_TOKENS.When
prevwas unset,process.env.COMPACT_RESERVED_TOKENS = undefineddoes not reliably remove the variable for the rest of the Vitest process. That leaks config into later cases and can make overflow tests order-dependent.Suggested fix
} finally { - if (prev === undefined) process.env.COMPACT_RESERVED_TOKENS = undefined; + if (prev === undefined) delete process.env.COMPACT_RESERVED_TOKENS; else process.env.COMPACT_RESERVED_TOKENS = prev; }🤖 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 `@harness/tests/context-compaction/e2e/full-session.test.ts` around lines 347 - 369, The test restores process.env.COMPACT_RESERVED_TOKENS incorrectly by assigning undefined which leaves the env var set; update the cleanup in the finally block of the test (involving prev and process.env.COMPACT_RESERVED_TOKENS) to delete process.env.COMPACT_RESERVED_TOKENS when prev is undefined and otherwise restore the original value (prev) so the environment is fully cleared for subsequent tests.harness/docs/workers/context-compaction.md (1)
121-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the sync-flow docs with the later replay section.
This section now says
/compactskips replay, while the later replay section sayscompact_nowkeeps the last user message on the active path and does not reinject it. The earlier sequence at Line 75 still sayscompact_now"reinjects user message", so the doc now describes two different sync flows.🤖 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 `@harness/docs/workers/context-compaction.md` around lines 121 - 132, Update the earlier sync-flow description so it matches the later replay section: change the sequence wording that currently states "reinjects user message" for compact_now to explicitly state that compact_now (and the /compact sync path which calls handleSync with projected_tokens: 999_999 and last_user_message_id: '') does NOT reinject the last user message and instead performs unconditional compaction; also note that compact_now returns the same CompactNowResult shape but with auto_continued always false and no synthetic "Continue…" prompt. Ensure you reference compact_now, /compact, handleSync, projected_tokens, last_user_message_id, and CompactNowResult in the updated text so both sections describe the same behavior.
🧹 Nitpick comments (7)
harness/src/context-compaction/summarize.ts (1)
87-102: ⚡ Quick winAvoid reading the full active path twice in one compaction pass.
summarizeAndAppend()callsloadActiveWithIds()andloadCompactionEntries()back-to-back, and each helper paginatesreadActivePath()to exhaustion. Compaction runs on the biggest sessions, so this doubles the hot-pathsession::messagestraffic and latency. Read once withinclude_custom: true, then derive both the message rows and compaction rows from that shared result.Also applies to: 132-134
🤖 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 `@harness/src/context-compaction/summarize.ts` around lines 87 - 102, The code currently calls readActivePath() twice by using loadActiveWithIds() and loadCompactionEntries() in summarizeAndAppend(), which doubles pagination and I/O; change the flow so summarizeAndAppend() calls readActivePath(iii, session_id, { include_custom: true }) once, store the returned items, then call messageItemsFromPath(items) to produce the MessageWithEntryId[] and compactionRowsFromPath(items) (or map its output) to produce the CompactionEntryLike[] instead of invoking loadActiveWithIds() and loadCompactionEntries(); update or remove those helpers accordingly and apply the same single-read refactor for the similar code around lines 132–134.harness/src/context-compaction/handler-async.ts (1)
77-90: ⚡ Quick winReuse the session-backed model lookup instead of duplicating the reverse scan.
This block now mirrors
resolveModelFromSessioninharness/src/context-compaction/model-resolver.ts: samereadActivePath(..., { roles: ['assistant'] }), same reverse walk, same provider/model extraction. Keeping both copies in sync will be easy to miss the next time the session entry shape changes. Pull the shared scan intomodel-resolver.tsand keep this function focused on the threadedmodel_limitfast path.🤖 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 `@harness/src/context-compaction/handler-async.ts` around lines 77 - 90, This block duplicates the reverse-scan logic from resolveModelFromSession; remove the inline readActivePath + reverse walk and instead call the shared resolver (resolveModelFromSession) to populate providerID and modelID, preserving the existing fast-path behavior for the threaded model_limit; ensure you pass the same context identifiers (iii and session_id or whatever parameters resolveModelFromSession expects) and only fall back to the threaded model_limit logic after using the shared resolver.harness/tests/turn-orchestrator/preflight.test.ts (1)
16-40: ⚡ Quick winAdd a regression test for
last_user_message_idderivation.These tests were updated to stub
session::messages, but they never assert thatrunPreflight()forwards the newest userentry_idintocontext-compaction::compact_now. A bug in the newreadActivePath(..., { roles: ['user'] })path would still leave this suite green.Example test to add
+ it('passes the newest user entry id to compact_now', async () => { + const { iii, calls } = makeIii({ + modelsGetResult: { context_window: 10, max_output_tokens: 0 }, + sessionTreeResult: { + messages: [ + { entry_id: 'a1', message: { role: 'assistant', content: [], timestamp: 0 } }, + { entry_id: 'u1', message: { role: 'user', content: [], timestamp: 0 } }, + { entry_id: 'u2', message: { role: 'user', content: [], timestamp: 0 } }, + ], + }, + compactNowResult: { status: 'ok' }, + }); + + await runPreflight(iii, 'my-session', [smallMessage], 'openai', 'gpt-4o'); + + const compactCall = calls.find((c) => c.function_id === 'context-compaction::compact_now'); + const p = compactCall?.payload as Record<string, unknown>; + expect(p.last_user_message_id).toBe('u2'); + });Also applies to: 160-174
🤖 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 `@harness/tests/turn-orchestrator/preflight.test.ts` around lines 16 - 40, Add a regression test to verify runPreflight forwards the newest user entry_id into the context-compaction::compact_now call: update the test harness created by makeIii to stub session::messages with multiple messages (including user messages with entry_id values), call runPreflight, then assert that calls includes a context-compaction::compact_now invocation whose payload contains last_user_message_id equal to the newest user message entry_id; specifically exercise the readActivePath(..., { roles: ['user'] }) path by stubbing session::messages and checking the compact_now payload to prevent the regression.harness/tests/context-compaction/integration/flow-sync.test.ts (1)
78-81: ⚡ Quick winAssert that the synthetic continue nudge is parented to the compaction entry.
This mock always returns
parent_id: null, and the test never checksnudge.parent_id, so a regression that appends the continue prompt at the session root would still pass. That would break the active-path shape the PR is introducing.Based on PR objectives, the sync flow should append the continuation nudge beneath the compaction entry.
Also applies to: 175-188
🤖 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 `@harness/tests/context-compaction/integration/flow-sync.test.ts` around lines 78 - 81, The mock append handler returns parent_id: null which lets regressions place the synthetic "continue" nudge at the session root; fix by assigning the continue nudge a parent_id that points to the compaction entry: when handling function_id === 'session::append' capture the assigned entry id (the `appended-${++appendSeq}` value, e.g. store it in lastCompactionEntryId when the payload is a compaction append added to compactionAppends) and return that id as parent_id for the synthetic continue nudge payload; apply the same change in the other mock block referenced (the block around lines 175-188) so continue nudges are parented to the compaction entry instead of null.harness/tests/context-compaction/integration/flow-async.test.ts (1)
77-79: ⚡ Quick winMake the
session::appendstub honor caller-suppliedentry_ids.Line 77 currently returns a fresh synthetic id for every append, so this test never exercises the idempotent-entry behavior the migration relies on.
harness/tests/_helpers/fakeSessionManager.tspreserves requested ids and dedupes repeats; mirroring that contract here would keep the async compaction test capable of catching duplicate replay-entry regressions.🤖 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 `@harness/tests/context-compaction/integration/flow-async.test.ts` around lines 77 - 79, The session::append stub currently always returns a synthetic id using appendSeq, so change the return logic in the session::append branch to honor a caller-supplied entry_id if present in the payload: check (payload as { entry_id?: string }).entry_id and, if defined, return that value as entry_id; otherwise fall back to `appended-${++appendSeq}`. Keep the existing compactPayloads push and the rest of the returned shape (parent_id and timestamp) so the test can exercise idempotent-entry behavior like fakeSessionManager.ts.harness/src/types/agent-event.ts (1)
67-80: ⚡ Quick winRefresh the
compaction_donedocblock to match the migrated console contract.The text around Line 68 still says this event lets the UI insert the compaction marker, but
console/web/src/lib/backend/translate.tsnow ignorescompaction_donefor marker rendering and relies on the session custom entry instead. Tightening that comment will keep the exported event contract aligned with actual downstream behavior.🤖 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 `@harness/src/types/agent-event.ts` around lines 67 - 80, Update the docblock for the 'compaction_done' event in harness/src/types/agent-event.ts to reflect the migrated console contract: remove the line stating the UI inserts a compaction marker and instead document that this event is emitted after a successful flat-state rewrite to provide metadata (mode, summary_text, tokens_before, and the session-manager compaction entry_id) for consumers; also note that marker rendering in the console/web frontend is now driven by the session custom entry rather than this event (reference the 'compaction_done' type and the 'summary_text' and 'tokens_before' fields to keep the contract clear).harness/tests/context-compaction/e2e/full-session.test.ts (1)
339-341: ⚡ Quick winThe e2e never asserts the reconstructed window is back under the usable budget.
Assertion 3 only proves the summarizer got a non-empty head. A regression that still leaves
loadContextView(...)over budget would pass this test even though the file header says budget compliance is guaranteed. Please recompute the post-compaction size fromafterView(or the exact provider input) and assert it is<= usable.🤖 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 `@harness/tests/context-compaction/e2e/full-session.test.ts` around lines 339 - 341, The test currently only checks providerInvocations non-empty but does not verify the reconstructed window fits the usable budget; update the assertion to recompute the post-compaction size from the provider input (use afterView if available or providerInvocations[0] / the exact input passed to loadContextView) using the same serialization/byte-size helper used elsewhere in the test suite and add an assertion that this computed size is <= usable (replace or augment the existing expect(providerInvocations[0]?.length)... checks). Ensure you reference the same helper function used for sizing so the comparison matches production logic.
🤖 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 `@console/README.md`:
- Around line 97-103: Update the README Features section and any earlier
persistence references so they no longer claim conversations persist in
localStorage; instead state that localStorage is used only for UI state and that
chat transcripts and durable conversation storage are handled by the new backend
workers `session-manager` (durable store for transcripts, sidebar, live token
rendering) and `harness` (turn orchestration). Edit the explanatory sentence(s)
and the example command block (the `iii worker add harness session-manager`
snippet) so both the Features summary and the persistence notes are consistent
about `session-manager` being the source of truth for transcripts and
localStorage being ephemeral UI-only state.
In `@console/web/PLAYGROUND.md`:
- Around line 508-518: Update the outdated backend guidance in
console/web/PLAYGROUND.md and console/web/README.md to describe the
session-manager-backed implementation: remove references that portray
src/lib/backend/real.ts as a stub/seam to replace, and instead document that
transcript text/thought tokens are produced from session-manager events
(session::message_updated snapshots) reconciled by use-conversations and
lib/sessions/entry-mapper, while real.ts only emits ephemeral StreamEvent
turn-state (approvals, function-call lifecycle, stop-reason, agent_end); ensure
ChatView, mock scenario behavior, and persistence notes (conversations persisted
by session-manager worker, not localStorage) are consistent between both files.
In `@console/web/src/hooks/use-conversations.ts`:
- Around line 330-390: The delayed transcript snapshots can overwrite newer
streamed messages because only message_updated checks revisions; update
onMessageAdded and the hydration merge to respect per-entry
revisions/timestamps: in onMessageAdded (handler) consult
revisionsFor(sessionId) and ignore/apply only when event.revision > existing
revision (or use event.timestamp > existing.updatedAt) before calling
patchConversation/applyEntryUpsert; similarly, in the hydration block (after
transcriptToMessages) deduplicate by id by comparing each fetched item's
revision/updatedAt against c.messages' corresponding entry using
revisionsFor(sessionId) and keep the newer one (or call applyEntryUpsert for
each fetched item so the same upsert logic/revision check is reused), then set
hydrated true.
In `@console/web/src/lib/backend/types.ts`:
- Around line 88-95: The comment for messageId is unclear about when optimistic
in-place reconciliation works; update the doc on messageId in types.ts to state
that the console can only reconcile the optimistic user entry
`<message_id>-user-0` in place if the caller supplies the same messageId (e.g.,
via ChatStreamOptions.messageId used by ChatView), because StreamEvent does not
expose a backend-minted message_id (see real.ts) and the backend may mint one
when opts.messageId is omitted; mention session::message_added to show how the
harness derives the session-manager entry id.
In `@console/web/src/lib/sessions/entry-mapper.ts`:
- Around line 54-60: The current textOf(ContentBlock[]) implementation drops
non-text blocks causing UserMessage reconstruction to lose images; update the
rebuild path so image blocks are preserved as attachments instead of being
discarded: modify textOf (or add a new helper) to return both the concatenated
text and an attachments list extracted from blocks (preserving blocks with type
'image' and relevant metadata), then update the UserMessage hydration logic that
currently only consumes text to also set message.attachments (or pass
attachments into the UserMessage constructor) so image-only prompts render as
attachments; ensure the same change is applied to the other hydration site that
mirrors this logic (the other block-handling code around the UserMessage
rebuild).
In `@console/web/src/lib/sessions/events.ts`:
- Around line 110-136: subscribeSessionTranscript creates two trigger bindings
with the hardcoded scope 'live', causing handler id collisions across multiple
subscriptions; change the scope passed to bind for the session::message_added
and session::message_updated bindings to a session-specific value (e.g., derive
a unique scope string from sessionId like `session_${sessionId}`) so the bind
calls in subscribeSessionTranscript (the two bind(...) entries referencing
handlers.onMessageAdded and handlers.onMessageUpdated, config and guard) use
that unique scope for both subscription and later cleanup.
In `@harness/README.md`:
- Around line 33-43: Update the advertised worker count from "Fourteen" (14) to
"Twelve" (12) in both harness README and architecture docs so the header matches
the enumerated list; search for any other occurrences of "14" or "Fourteen
workers" in those docs and change them to "12" or "Twelve workers", and verify
the surrounding sentence/heading text still reads correctly with the updated
number.
In `@harness/src/context-compaction/prune.ts`:
- Around line 76-100: The loop in prune.ts marks every queued entry as pruned
even if sessionUpdateMessage throws or returns {updated: false}; update logic so
you only count an entry toward pruned_tokens/pruned_parts after
sessionUpdateMessage actually succeeds. In the for...of queue loop (where
compacted_at is set and sessionUpdateMessage is called), call
sessionUpdateMessage, check its result (and treat thrown errors as failures),
and only then increment whatever accumulators use q.token_count / q.part_count
(or similar pruned counters) and consider the entry pruned; on failure log/skip
as currently done but do not update the pruned_* totals. Ensure the details
merge and content replacement remain unchanged when you gate the counter updates
on success.
In `@harness/src/turn-orchestrator/assistant-streaming/ports.ts`:
- Around line 164-168: The strict path in updateAssistantContent drops the
{updated, revision} result from sessionUpdateMessage, so change
updateAssistantContent to capture the returned result from
sessionUpdateMessage({ session_id, entry_id, content, origin: opts?.origin }),
check the returned updated flag and propagate failure when updated is false
(e.g., throw or return a clear non-updated signal) so finalizeAssistantTurn /
the FSM can detect a no-op; preserve the existing tolerant behavior
(opts?.tolerant) only for the catch path and ensure you forward revision when
available.
In `@harness/src/turn-orchestrator/function-awaiting-approval/run.ts`:
- Around line 82-83: When readDecision() returns a decision but the
corresponding prepared call is missing, remove or fail the stale
awaiting_approval entry before continuing so the batch doesn't stay blocked;
locate the check using callId and work.prepared.find(...) in run.ts, and before
the existing "if (!current) continue" delete (or mark failed) the matching
awaiting entry (the entry keyed by callId in the awaiting approvals collection)
so routeAfterApprovalProcessing() no longer sees an incomplete batch for that
callId.
In `@harness/src/turn-orchestrator/run-start.ts`:
- Around line 58-77: The session is set to 'working' before durable writes
complete, risking a stuck state if subsequent calls fail; move the await
sessionSetStatus(iii, session_id, 'working') to after the durable start write(s)
(e.g., after await store.saveRunRequest(...) and await
store.appendMessages(...), and any saveRecord call), or alternatively wrap
saveRunRequest/appendMessages/saveRecord in a try/catch that on error resets the
session via sessionSetStatus(iii, session_id, 'error' | previousStatus) and
rethrows; update calls referencing sessionSetStatus, store.saveRunRequest,
store.appendMessages, and saveRecord accordingly so the session only flips to
'working' after durable persistence or is rolled back on failure.
In `@harness/src/turn-orchestrator/run-transition.ts`:
- Around line 73-76: Move the session status update so it cannot be
short-circuited by awaited external emits: call sessionSetStatus(iii,
rec.session_id, 'error', rec.error.message) before awaiting emit(...) (or wrap
emits in a non-throwing/fire-and-forget block) so that even if message_complete
or agent_end throws after saveRecord(...), the session store is updated to
'error'; update the sequence around emit and sessionSetStatus in
run-transition.ts accordingly.
In `@harness/src/turn-orchestrator/state-runtime/context-view.ts`:
- Around line 49-60: The code currently computes tail_start_id against the
filtered `usable` array which drops empty-assistant placeholders, causing
compactions anchored on placeholder entries to be missed; instead, locate
`compaction.tail_start_id` in the original `messages` array (e.g., use
`messages.findIndex(m => m.entry_id === compaction.tail_start_id)`) and then
convert that raw index into the corresponding index in the filtered `usable`
sequence by counting non-placeholder entries up to that raw index (use
`isEmptyAssistant` to detect placeholders). Use that resulting index for
`tailStart` (fall back to 0 if not found) before slicing `usable` and returning
the summary + tail messages; keep the rest of the function (`latestCompaction`,
`buildSummaryMessage`, etc.) unchanged.
In `@harness/src/turn-orchestrator/state-runtime/ports.ts`:
- Around line 35-40: finishStatus currently only checks
last_assistant.stop_reason and can return {status:'done'} even when the
TurnStateRecord indicates failure; update finishStatus to first inspect
rec.state and rec.error on the TurnStateRecord and return {status:'error',
reason: rec.error ?? 'turn failed'} when rec.state === 'failed' or rec.error is
present, before falling back to checking last_assistant
(last_assistant.stop_reason === 'error' ? use last_assistant.error_message).
Ensure the function signature and return shape remain unchanged and reference
finishStatus, TurnStateRecord, rec.state, rec.error, and last_assistant when
making the change.
In `@harness/src/turn-orchestrator/state-runtime/store.ts`:
- Around line 204-218: The appendMessages loop (function appendMessages) can
produce duplicate transcript rows on retry because non-deterministic entry ids
(from defaultEntryId) allow partial commits; make this helper enforce
deterministic ids for every message before calling sessionAppendMessage: require
opts.entryIdFor be provided (or compute a deterministic id for each message) and
validate each entry_id is stable and non-null, throwing an error if any message
would use defaultEntryId, or alternatively narrow the helper to only accept a
single retriable logical step (change signature to appendMessage for one
message) so multi-message batches cannot be partially applied; update call sites
accordingly.
In `@harness/src/turn-orchestrator/state.ts`:
- Around line 106-121: parseTurnStateRecord currently uses a generic
TurnStateRecordSchema that accepts records with state: 'function_execute' (and
other batch states) even when required batch-specific fields are missing,
causing downstream crashes; change TurnStateRecordSchema into a discriminated
union on the "state" field (use z.discriminatedUnion or z.union of .refine
variants keyed by state) so that entries with state === 'function_execute' (and
other batch-related states) must include the batch fields work, last_assistant,
and awaiting_approval, while other states keep their own minimal shape; update
parseTurnStateRecord to use this new schema (same function name) so malformed
persisted batch records are rejected up front.
In `@harness/tests/_helpers/fakeSessionManager.ts`:
- Around line 181-201: The messages(p) helper currently iterates s.entries in
append order; instead compute the active-path entry_ids by starting at s.root_id
and repeatedly selecting the most recently appended child whose parent_id equals
the current node (until no child exists), collect that chain's entry_ids, then
filter s.entries to only include entries whose entry_id is in that active-path
set (still applying the existing role and include_custom checks). Update the
messages(p) function to build that active-path set (using s.entries' parent_id
and entry_id fields) and then produce out from only those entries.
In `@harness/tests/turn-orchestrator/assistant.test.ts`:
- Around line 220-238: The test currently only asserts the persisted entry's
content; extend it to also assert the persisted metadata (stop_reason and
error_message) is preserved on the session transcript. In the test that uses
assistant({ stop_reason: 'error', error_message: 'auth failed' }), after
obtaining entries = sessions.messageEntries('s1') add assertions that
entries[0]?.message?.details?.stop_reason === 'error' and
entries[0]?.message?.details?.error_message === 'auth failed' (or the equivalent
metadata field used by sessions.messageEntries), so the fake session-manager
path is validated to keep the error metadata.
In `@harness/tests/turn-orchestrator/run-transition.test.ts`:
- Around line 102-112: The writes array currently types payload as unknown which
causes unsafe property access in later assertions; update the writes declaration
or the push site in the fake III implementation (the iii.trigger function) to
use a narrowed payload type (e.g., an interface with scope, key, value, data
fields) or cast the payload to that shape before pushing so downstream
assertions like w.payload.scope / w.payload.value / w.payload.data are
type-safe; change the declaration of writes (and/or the push in iii.trigger) to
use that narrowed payload type to align with expected test usage.
---
Outside diff comments:
In `@console/web/src/components/chat/ChatView.tsx`:
- Around line 339-423: The fcallMap may contain stale message ids after the
session-events reconciler replaces locally appended function-call rows, causing
handlers like the 'fcall-end' and 'fcall-approval-cleared' cases to patch
nonexistent ids; update the logic in the 'fcall-end' and
'fcall-approval-cleared' branches to first verify that any id found in fcallMap
actually exists in messagesRef.current (or re-resolve the current id by
searching messagesRef.current for the same functionCallId) before calling
onPatchMessage, and when reconciliation replaces a local uid with an
entry-derived id, update fcallMap (or remove the stale mapping) so fcallId /
fcallMap always point to an existing message id.
In `@harness/docs/workers/context-compaction.md`:
- Around line 121-132: Update the earlier sync-flow description so it matches
the later replay section: change the sequence wording that currently states
"reinjects user message" for compact_now to explicitly state that compact_now
(and the /compact sync path which calls handleSync with projected_tokens:
999_999 and last_user_message_id: '') does NOT reinject the last user message
and instead performs unconditional compaction; also note that compact_now
returns the same CompactNowResult shape but with auto_continued always false and
no synthetic "Continue…" prompt. Ensure you reference compact_now, /compact,
handleSync, projected_tokens, last_user_message_id, and CompactNowResult in the
updated text so both sections describe the same behavior.
In `@harness/docs/workers/turn-orchestrator.md`:
- Around line 133-136: The documentation still references the old dependency
name "session ^0.2.0"; update the dependency listing in the turn-orchestrator
documentation to "session-manager ^0.2.0" so the orchestrator is wired to the
external session-manager worker as intended—replace the literal `session ^0.2.0`
entry in the dependency list/markdown block (the same block that also lists
`provider-anthropic ^0.2.0` and `provider-openai ^0.2.0`) with `session-manager
^0.2.0` and run a quick spell-check to ensure no other occurrences remain.
In `@harness/src/index.ts`:
- Around line 36-116: The composite WORKERS list no longer includes the
in-process session manager so runtime calls to
session::ensure/append/messages/set_status can silently fail; fix by either
adding a session-manager entry to WORKERS (e.g., add an object with name
'session-manager' and register: (iii, ctx) => registerSessionManager(iii, ctx)
or registerSessionManager(iii) depending on your API) so the harness brings up
the session backend, or add a startup dependency check in the bootstrap (where
WORKERS is consumed / app initialization runs) that calls the session RPCs
(session::ensure or a lightweight health-check like session::ping) and fails
fast with a clear error if no session-manager is present.
In `@harness/tests/context-compaction/e2e/full-session.test.ts`:
- Around line 347-369: The test restores process.env.COMPACT_RESERVED_TOKENS
incorrectly by assigning undefined which leaves the env var set; update the
cleanup in the finally block of the test (involving prev and
process.env.COMPACT_RESERVED_TOKENS) to delete
process.env.COMPACT_RESERVED_TOKENS when prev is undefined and otherwise restore
the original value (prev) so the environment is fully cleared for subsequent
tests.
---
Nitpick comments:
In `@harness/src/context-compaction/handler-async.ts`:
- Around line 77-90: This block duplicates the reverse-scan logic from
resolveModelFromSession; remove the inline readActivePath + reverse walk and
instead call the shared resolver (resolveModelFromSession) to populate
providerID and modelID, preserving the existing fast-path behavior for the
threaded model_limit; ensure you pass the same context identifiers (iii and
session_id or whatever parameters resolveModelFromSession expects) and only fall
back to the threaded model_limit logic after using the shared resolver.
In `@harness/src/context-compaction/summarize.ts`:
- Around line 87-102: The code currently calls readActivePath() twice by using
loadActiveWithIds() and loadCompactionEntries() in summarizeAndAppend(), which
doubles pagination and I/O; change the flow so summarizeAndAppend() calls
readActivePath(iii, session_id, { include_custom: true }) once, store the
returned items, then call messageItemsFromPath(items) to produce the
MessageWithEntryId[] and compactionRowsFromPath(items) (or map its output) to
produce the CompactionEntryLike[] instead of invoking loadActiveWithIds() and
loadCompactionEntries(); update or remove those helpers accordingly and apply
the same single-read refactor for the similar code around lines 132–134.
In `@harness/src/types/agent-event.ts`:
- Around line 67-80: Update the docblock for the 'compaction_done' event in
harness/src/types/agent-event.ts to reflect the migrated console contract:
remove the line stating the UI inserts a compaction marker and instead document
that this event is emitted after a successful flat-state rewrite to provide
metadata (mode, summary_text, tokens_before, and the session-manager compaction
entry_id) for consumers; also note that marker rendering in the console/web
frontend is now driven by the session custom entry rather than this event
(reference the 'compaction_done' type and the 'summary_text' and 'tokens_before'
fields to keep the contract clear).
In `@harness/tests/context-compaction/e2e/full-session.test.ts`:
- Around line 339-341: The test currently only checks providerInvocations
non-empty but does not verify the reconstructed window fits the usable budget;
update the assertion to recompute the post-compaction size from the provider
input (use afterView if available or providerInvocations[0] / the exact input
passed to loadContextView) using the same serialization/byte-size helper used
elsewhere in the test suite and add an assertion that this computed size is <=
usable (replace or augment the existing
expect(providerInvocations[0]?.length)... checks). Ensure you reference the same
helper function used for sizing so the comparison matches production logic.
In `@harness/tests/context-compaction/integration/flow-async.test.ts`:
- Around line 77-79: The session::append stub currently always returns a
synthetic id using appendSeq, so change the return logic in the session::append
branch to honor a caller-supplied entry_id if present in the payload: check
(payload as { entry_id?: string }).entry_id and, if defined, return that value
as entry_id; otherwise fall back to `appended-${++appendSeq}`. Keep the existing
compactPayloads push and the rest of the returned shape (parent_id and
timestamp) so the test can exercise idempotent-entry behavior like
fakeSessionManager.ts.
In `@harness/tests/context-compaction/integration/flow-sync.test.ts`:
- Around line 78-81: The mock append handler returns parent_id: null which lets
regressions place the synthetic "continue" nudge at the session root; fix by
assigning the continue nudge a parent_id that points to the compaction entry:
when handling function_id === 'session::append' capture the assigned entry id
(the `appended-${++appendSeq}` value, e.g. store it in lastCompactionEntryId
when the payload is a compaction append added to compactionAppends) and return
that id as parent_id for the synthetic continue nudge payload; apply the same
change in the other mock block referenced (the block around lines 175-188) so
continue nudges are parented to the compaction entry instead of null.
In `@harness/tests/turn-orchestrator/preflight.test.ts`:
- Around line 16-40: Add a regression test to verify runPreflight forwards the
newest user entry_id into the context-compaction::compact_now call: update the
test harness created by makeIii to stub session::messages with multiple messages
(including user messages with entry_id values), call runPreflight, then assert
that calls includes a context-compaction::compact_now invocation whose payload
contains last_user_message_id equal to the newest user message entry_id;
specifically exercise the readActivePath(..., { roles: ['user'] }) path by
stubbing session::messages and checking the compact_now payload to prevent the
regression.
🪄 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: 5c87f548-8fb7-4099-874e-a97331ce0f9e
📒 Files selected for processing (103)
.gitignoreREADME.mdconsole/README.mdconsole/web/PLAYGROUND.mdconsole/web/README.mdconsole/web/src/components/chat/ChatView.tsxconsole/web/src/components/sidebar/ConversationRow.tsxconsole/web/src/hooks/use-conversations.tsconsole/web/src/lib/backend/auto-accept-policy.test.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/translate.test.tsconsole/web/src/lib/backend/translate.tsconsole/web/src/lib/backend/types.tsconsole/web/src/lib/conversations-context.tsxconsole/web/src/lib/functions.tsconsole/web/src/lib/session-id.tsconsole/web/src/lib/sessions/api.tsconsole/web/src/lib/sessions/entry-mapper.test.tsconsole/web/src/lib/sessions/entry-mapper.tsconsole/web/src/lib/sessions/events.tsconsole/web/src/lib/sessions/types.tsconsole/web/src/lib/storage.tsconsole/web/src/types/chat.tsharness/README.mdharness/docs/architecture.mdharness/docs/workers/context-compaction.mdharness/docs/workers/session.mdharness/docs/workers/turn-orchestrator.mdharness/iii.worker.yamlharness/package.jsonharness/src/context-compaction/handler-async.tsharness/src/context-compaction/handler-sync.tsharness/src/context-compaction/iii.worker.yamlharness/src/context-compaction/main.tsharness/src/context-compaction/model-resolver.tsharness/src/context-compaction/prune.tsharness/src/context-compaction/register.tsharness/src/context-compaction/replay.tsharness/src/context-compaction/selection.tsharness/src/context-compaction/summarize.tsharness/src/index.tsharness/src/models-catalog/state.tsharness/src/runtime/models-discovery.tsharness/src/runtime/session.tsharness/src/session/iii.worker.yamlharness/src/session/main.tsharness/src/session/register.tsharness/src/session/tree/operations.tsharness/src/session/tree/register.tsharness/src/session/tree/store.tsharness/src/session/tree/types.tsharness/src/turn-orchestrator/assistant-streaming/ports.tsharness/src/turn-orchestrator/assistant-streaming/run.tsharness/src/turn-orchestrator/function-awaiting-approval/run.tsharness/src/turn-orchestrator/function-execute/ports.tsharness/src/turn-orchestrator/function-execute/run.tsharness/src/turn-orchestrator/iii.worker.yamlharness/src/turn-orchestrator/preflight.tsharness/src/turn-orchestrator/run-start.tsharness/src/turn-orchestrator/run-transition.tsharness/src/turn-orchestrator/state-runtime/context-view.tsharness/src/turn-orchestrator/state-runtime/ports.tsharness/src/turn-orchestrator/state-runtime/store.tsharness/src/turn-orchestrator/state-runtime/transcript.tsharness/src/turn-orchestrator/state.tsharness/src/types/agent-event.tsharness/tests/_helpers/fakeSessionManager.tsharness/tests/context-compaction/compact-session-registered.test.tsharness/tests/context-compaction/compact-session.test.tsharness/tests/context-compaction/compaction-done-emit.test.tsharness/tests/context-compaction/e2e/full-session.test.tsharness/tests/context-compaction/handler-sync.test.tsharness/tests/context-compaction/integration/backward-compat.test.tsharness/tests/context-compaction/integration/flow-async.test.tsharness/tests/context-compaction/integration/flow-prune.test.tsharness/tests/context-compaction/integration/flow-sync.test.tsharness/tests/context-compaction/lease.test.tsharness/tests/context-compaction/prune.test.tsharness/tests/context-compaction/selection.test.tsharness/tests/context-compaction/strip-media.test.tsharness/tests/context-compaction/summarize.test.tsharness/tests/integration/parallel-approval-harness.tsharness/tests/provider-lmstudio/stream.test.tsharness/tests/session/operations.test.tsharness/tests/session/tree-append-synthetic.test.tsharness/tests/session/tree-compact-tail-start.test.tsharness/tests/session/tree-compactions.test.tsharness/tests/session/tree-update-part.test.tsharness/tests/session/tree/store.test.tsharness/tests/session/tree/types.test.tsharness/tests/turn-orchestrator/_helpers/mockTurnStore.tsharness/tests/turn-orchestrator/assistant-streaming.test.tsharness/tests/turn-orchestrator/assistant.test.tsharness/tests/turn-orchestrator/coalesce-deltas.test.tsharness/tests/turn-orchestrator/context-view.test.tsharness/tests/turn-orchestrator/function-execute.test.tsharness/tests/turn-orchestrator/functions.test.tsharness/tests/turn-orchestrator/get-state.test.tsharness/tests/turn-orchestrator/preflight.test.tsharness/tests/turn-orchestrator/run-start.test.tsharness/tests/turn-orchestrator/run-transition.test.tsharness/tests/turn-orchestrator/store.test.tsiii-permissions.yaml
💤 Files with no reviewable changes (18)
- harness/tests/session/tree/store.test.ts
- harness/tests/session/tree-compactions.test.ts
- harness/src/session/main.ts
- harness/src/session/iii.worker.yaml
- harness/tests/session/tree/types.test.ts
- harness/tests/session/tree-compact-tail-start.test.ts
- harness/src/turn-orchestrator/state-runtime/transcript.ts
- harness/docs/workers/session.md
- harness/tests/session/tree-append-synthetic.test.ts
- harness/tests/session/operations.test.ts
- harness/src/session/tree/store.ts
- harness/src/session/register.ts
- harness/src/session/tree/register.ts
- harness/src/session/tree/types.ts
- harness/src/turn-orchestrator/function-execute/ports.ts
- harness/tests/session/tree-update-part.test.ts
- harness/tests/turn-orchestrator/_helpers/mockTurnStore.ts
- harness/src/session/tree/operations.ts
Comment on lines
+97
to
+103
| Chat needs two more workers on the engine: `harness` (turn orchestration) | ||
| and `session-manager` (the durable conversation store the sidebar, | ||
| transcripts, and live token rendering are backed by): | ||
|
|
||
| ```bash | ||
| iii worker add harness session-manager | ||
| ``` |
There was a problem hiding this comment.
Update the persistence docs in this README to match the new backend.
This new note correctly says chat transcripts now depend on session-manager, but the earlier Features section still says conversations persist in localStorage. That leaves the README internally contradictory after this migration. Based on the PR summary, localStorage is now UI-state-only.
🤖 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 `@console/README.md` around lines 97 - 103, Update the README Features section
and any earlier persistence references so they no longer claim conversations
persist in localStorage; instead state that localStorage is used only for UI
state and that chat transcripts and durable conversation storage are handled by
the new backend workers `session-manager` (durable store for transcripts,
sidebar, live token rendering) and `harness` (turn orchestration). Edit the
explanatory sentence(s) and the example command block (the `iii worker add
harness session-manager` snippet) so both the Features summary and the
persistence notes are consistent about `session-manager` being the source of
truth for transcripts and localStorage being ephemeral UI-only state.
Comment on lines
+508
to
+518
| - **The real backend's transcript path.** [`real.ts`](src/lib/backend/real.ts) | ||
| no longer streams transcript content through `StreamEvent`s — text/thought | ||
| tokens render from session-manager events (`session::message_updated` | ||
| snapshots) reconciled by `use-conversations` + `lib/sessions/entry-mapper`. | ||
| The real backend's stream carries only ephemeral turn state (approvals, | ||
| function-call lifecycle, stop-reason notices, agent_end). Mock scenario | ||
| backends still exercise the full `StreamEvent` surface below, and ChatView | ||
| keeps rendering all of it — that is exactly what these stories pin. | ||
| - **Persisting playground conversations.** They're ephemeral by design; | ||
| the real chat surface persists conversations in the session-manager | ||
| worker (not localStorage). |
There was a problem hiding this comment.
Unify the post-migration backend documentation across console/web/PLAYGROUND.md and console/web/README.md.
Both files now explain parts of the new harness/session-manager flow, but they still retain older sections that describe src/lib/backend/real.ts as a stub/provider seam to replace. The shared root cause is a partial docs migration: contributors can read either file and come away with two different production-backend models. Please update the older stub/replacement guidance in console/web/PLAYGROUND.md and console/web/README.md to match the new session-manager-backed implementation.
🧰 Tools
🪛 LanguageTool
[style] ~515-~515: Consider an alternative for the overused word “exactly”.
Context: ...w keeps rendering all of it — that is exactly what these stories pin. - **Persisting ...
(EXACTLY_PRECISELY)
🤖 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 `@console/web/PLAYGROUND.md` around lines 508 - 518, Update the outdated
backend guidance in console/web/PLAYGROUND.md and console/web/README.md to
describe the session-manager-backed implementation: remove references that
portray src/lib/backend/real.ts as a stub/seam to replace, and instead document
that transcript text/thought tokens are produced from session-manager events
(session::message_updated snapshots) reconciled by use-conversations and
lib/sessions/entry-mapper, while real.ts only emits ephemeral StreamEvent
turn-state (approvals, function-call lifecycle, stop-reason, agent_end); ensure
ChatView, mock scenario behavior, and persistence notes (conversations persisted
by session-manager worker, not localStorage) are consistent between both files.
Comment on lines
+330
to
+390
| onMessageAdded: (event) => { | ||
| patchConversation(sessionId, (c) => ({ | ||
| ...c, | ||
| messages: applyEntryUpsert( | ||
| c.messages, | ||
| { | ||
| entry_id: event.entry_id, | ||
| message: event.message, | ||
| custom: event.custom, | ||
| }, | ||
| { sessionId }, | ||
| ), | ||
| updatedAt: event.timestamp, | ||
| })) | ||
| }, | ||
| onMessageUpdated: (event) => { | ||
| const revs = revisionsFor(sessionId) | ||
| const prev = revs.get(event.entry_id) ?? -1 | ||
| if (event.revision <= prev) return | ||
| revs.set(event.entry_id, event.revision) | ||
| patchConversation(sessionId, (c) => ({ | ||
| ...c, | ||
| messages: applyEntryUpsert( | ||
| c.messages, | ||
| { entry_id: event.entry_id, message: event.message }, | ||
| { sessionId, streaming: c.status === 'working' }, | ||
| ), | ||
| updatedAt: event.timestamp, | ||
| })) | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| return () => { | ||
| if (persistRef.current) cancelAnimationFrame(persistRef.current) | ||
| cancelled = true | ||
| off?.() | ||
| } | ||
| }, [conversations]) | ||
| }, [activeIsServerBacked, activeId, patchConversation]) | ||
|
|
||
| /* Migrate persisted model ids once catalog-backed keys are known. Gated on | ||
| catalogReady so we don't rewrite catalog-only picks (e.g. claude-haiku-4-5) | ||
| against a stale placeholder catalog during the brief window before the real | ||
| catalog fetch resolves. Also reconciles the persisted last-model slot. */ | ||
| /* Hydrate the active conversation's transcript once (read-back, then the | ||
| live subscription above keeps it current). Folding through | ||
| applyEntryUpsert makes the read idempotent against events that raced in | ||
| while the fetch was in flight. */ | ||
| useEffect(() => { | ||
| if (!activeIsServerBacked || !activeId) return | ||
| const conv = conversations.find((c) => c.id === activeId) | ||
| if (!conv || conv.hydrated) return | ||
| const sessionId = activeId | ||
| let cancelled = false | ||
| void fetchTranscript(sessionId) | ||
| .then((items) => { | ||
| if (cancelled) return | ||
| patchConversation(sessionId, (c) => { | ||
| let messages = transcriptToMessages(items, sessionId) | ||
| // Re-apply anything the live feed already reconciled on top. | ||
| for (const m of c.messages) { | ||
| if (!messages.some((existing) => existing.id === m.id)) { | ||
| messages = [...messages, m] | ||
| } | ||
| } | ||
| return { ...c, messages, hydrated: true } |
There was a problem hiding this comment.
Older transcript snapshots can still roll the UI backward.
revisionsRef only protects message_updated. On Lines 330-343 a delayed message_added snapshot is always applied, and on Lines 382-390 hydration keeps the fetched row whenever ids collide. If the newer streamed snapshot arrived first, either path can replace it with the older placeholder copy for the same entry id and regress the visible transcript.
🤖 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 `@console/web/src/hooks/use-conversations.ts` around lines 330 - 390, The
delayed transcript snapshots can overwrite newer streamed messages because only
message_updated checks revisions; update onMessageAdded and the hydration merge
to respect per-entry revisions/timestamps: in onMessageAdded (handler) consult
revisionsFor(sessionId) and ignore/apply only when event.revision > existing
revision (or use event.timestamp > existing.updatedAt) before calling
patchConversation/applyEntryUpsert; similarly, in the hydration block (after
transcriptToMessages) deduplicate by id by comparing each fetched item's
revision/updatedAt against c.messages' corresponding entry using
revisionsFor(sessionId) and keep the newer one (or call applyEntryUpsert for
each fetched item so the same upsert logic/revision check is reused), then set
hydrated true.
Comment on lines
+88
to
+95
| /** | ||
| * Per-send message id (`msg-<uuid>`). The harness derives the user | ||
| * message's session-manager entry id from it (`<message_id>-user-0`), so | ||
| * the console's optimistic user message reconciles in place when the | ||
| * `session::message_added` snapshot arrives. The real backend mints one | ||
| * when omitted. | ||
| */ | ||
| messageId?: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Chat backend stream call sites =="
rg -n -C2 '\.stream\s*\(' console/web/src -g '!**/*.test.*'
echo
echo "== Explicit messageId plumbing =="
rg -n -C2 'messageId\s*:' console/web/src -g '!**/*.test.*'
echo
echo "== Session/message id helpers in likely send paths =="
fd 'ChatView.tsx|use-conversations.ts|session-id.ts' console/web/src -x sh -c '
echo "### $1"
sed -n "1,260p" "$1"
' sh {}Repository: iii-hq/workers
Length of output: 22559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect backend.stream call in ChatView.tsx =="
rg -n "backend\.stream\s*\(" console/web/src/components/chat/ChatView.tsx
echo
# show around the call
sed -n '250,380p' console/web/src/components/chat/ChatView.tsx
echo
echo "== Inspect messageId types in console/web/src/lib/backend/types.ts (around mentioned lines 88-95) =="
sed -n '1,170p' console/web/src/lib/backend/types.ts | sed -n '70,130p'
echo
echo "== Inspect StreamEvent shape in backend/types.ts =="
rg -n "export type StreamEvent|interface StreamEvent|type StreamEvent|StreamEvent" console/web/src/lib/backend/types.ts
sed -n '1,260p' console/web/src/lib/backend/types.ts | sed -n '1,220p'
echo
echo "== Inspect real backend stream implementation (console/web/src/lib/backend/real.ts) =="
ls -1 console/web/src/lib/backend | sed -n '1,200p'
sed -n '1,260p' console/web/src/lib/backend/real.tsRepository: iii-hq/workers
Length of output: 21106
Clarify messageId-dependent optimistic reconciliation
StreamEvent doesn’t expose the backend-chosen message_id; the real backend can mint one internally when opts.messageId is omitted (real.ts), but the console can’t predict/match the optimistic user entry id (<message_id>-user-0) without the caller supplying the same messageId (as ChatView does). Update the comment to state optimistic in-place reconciliation only works when ChatStreamOptions.messageId is provided by the caller.
🤖 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 `@console/web/src/lib/backend/types.ts` around lines 88 - 95, The comment for
messageId is unclear about when optimistic in-place reconciliation works; update
the doc on messageId in types.ts to state that the console can only reconcile
the optimistic user entry `<message_id>-user-0` in place if the caller supplies
the same messageId (e.g., via ChatStreamOptions.messageId used by ChatView),
because StreamEvent does not expose a backend-minted message_id (see real.ts)
and the backend may mint one when opts.messageId is omitted; mention
session::message_added to show how the harness derives the session-manager entry
id.
Comment on lines
+54
to
+60
| function textOf(blocks: ContentBlock[]): string { | ||
| let out = '' | ||
| for (const block of blocks) { | ||
| if (block.type === 'text') out += block.text | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Persisted image turns currently hydrate as blank/text-only user messages.
textOf() drops every non-text block, and the user branch only uses that string. After this migration, a stored user turn that contains { type: 'image' } blocks loses its attachment chips on reload, and an image-only prompt becomes an empty bubble. Preserve image blocks in attachments (or another visible fallback) when rebuilding UserMessage.
Also applies to: 107-114
🤖 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 `@console/web/src/lib/sessions/entry-mapper.ts` around lines 54 - 60, The
current textOf(ContentBlock[]) implementation drops non-text blocks causing
UserMessage reconstruction to lose images; update the rebuild path so image
blocks are preserved as attachments instead of being discarded: modify textOf
(or add a new helper) to return both the concatenated text and an attachments
list extracted from blocks (preserving blocks with type 'image' and relevant
metadata), then update the UserMessage hydration logic that currently only
consumes text to also set message.attachments (or pass attachments into the
UserMessage constructor) so image-only prompts render as attachments; ensure the
same change is applied to the other hydration site that mirrors this logic (the
other block-handling code around the UserMessage rebuild).
Comment on lines
+204
to
+218
| /* | ||
| * Sequential appends without explicit parent_id: each append chains from | ||
| * the active leaf and moves it, preserving order. Deterministic entry ids | ||
| * make redelivered steps no-ops (existing entry returned, nothing fired). | ||
| */ | ||
| async appendMessages(session_id, msgs, opts) { | ||
| for (const [index, message] of msgs.entries()) { | ||
| const entry_id = opts?.entryIdFor?.(message, index) ?? defaultEntryId(message); | ||
| await sessionAppendMessage(iii, { | ||
| session_id, | ||
| message, | ||
| ...(entry_id ? { entry_id } : {}), | ||
| ...(opts?.origin ? { origin: opts.origin } : {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Sequential appends are not retry-safe unless every message has a deterministic entry_id.
This loop can persist a prefix and then fail mid-batch. On retry, only function_result is deduped by default; any earlier user/assistant/system message without opts.entryIdFor(...) is appended again as a new transcript row. That turns a transient session::append error into duplicate history.
Either enforce deterministic ids for every retriable message passed through appendMessages, or narrow this helper so one logical retried step never spans multiple non-idempotent appends.
🤖 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 `@harness/src/turn-orchestrator/state-runtime/store.ts` around lines 204 - 218,
The appendMessages loop (function appendMessages) can produce duplicate
transcript rows on retry because non-deterministic entry ids (from
defaultEntryId) allow partial commits; make this helper enforce deterministic
ids for every message before calling sessionAppendMessage: require
opts.entryIdFor be provided (or compute a deterministic id for each message) and
validate each entry_id is stable and non-null, throwing an error if any message
would use defaultEntryId, or alternatively narrow the helper to only accept a
single retriable logical step (change signature to appendMessage for one
message) so multi-message batches cannot be partially applied; update call sites
accordingly.
Comment on lines
+106
to
+121
| /** Minimal structural guard for persisted turn_state — nested fields pass through. */ | ||
| const TurnStateRecordSchema = z | ||
| .object({ | ||
| session_id: z.string(), | ||
| state: z.enum(TURN_STATES), | ||
| turn_count: z.number().catch(0), | ||
| function_results: z.array(z.unknown()).catch([]), | ||
| turn_end_emitted: z.boolean().catch(false), | ||
| started_at_ms: z.number().catch(0), | ||
| updated_at_ms: z.number().catch(0), | ||
| }) | ||
| .passthrough(); | ||
|
|
||
| export function parseTurnStateRecord(raw: unknown): TurnStateRecord | null { | ||
| const result = TurnStateRecordSchema.safeParse(raw); | ||
| return result.success ? (result.data as TurnStateRecord) : null; |
There was a problem hiding this comment.
The recovery parser accepts invalid batch-state records as valid.
parseTurnStateRecord() currently treats { state: 'function_execute', ... } as a TurnStateRecord even when work, last_assistant, or awaiting_approval are missing. That pushes malformed persisted state deeper into recovery and turns it into a later crash on rec.work.prepared instead of rejecting it up front. Make the schema discriminated by state so batch states require their batch fields.
🤖 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 `@harness/src/turn-orchestrator/state.ts` around lines 106 - 121,
parseTurnStateRecord currently uses a generic TurnStateRecordSchema that accepts
records with state: 'function_execute' (and other batch states) even when
required batch-specific fields are missing, causing downstream crashes; change
TurnStateRecordSchema into a discriminated union on the "state" field (use
z.discriminatedUnion or z.union of .refine variants keyed by state) so that
entries with state === 'function_execute' (and other batch-related states) must
include the batch fields work, last_assistant, and awaiting_approval, while
other states keep their own minimal shape; update parseTurnStateRecord to use
this new schema (same function name) so malformed persisted batch records are
rejected up front.
Comment on lines
+181
to
+201
| private messages(p: Record<string, unknown>) { | ||
| const s = this.session(String(p.session_id)); | ||
| const roles = Array.isArray(p.roles) ? (p.roles as string[]) : null; | ||
| const include_custom = p.include_custom === true; | ||
| const out: Array<{ | ||
| entry_id: string; | ||
| message?: AgentMessage; | ||
| custom?: { custom_type: string; data: unknown }; | ||
| }> = []; | ||
| for (const e of s.entries) { | ||
| if (e.kind === 'message' && e.message) { | ||
| if (roles && !roles.includes(e.message.role)) continue; | ||
| out.push({ entry_id: e.entry_id, message: e.message }); | ||
| } else if (e.kind === 'custom' && e.custom) { | ||
| if (roles) continue; | ||
| if (!include_custom) continue; | ||
| out.push({ entry_id: e.entry_id, custom: e.custom }); | ||
| } | ||
| } | ||
| // Single page: tests stay below the 500-row cap. | ||
| return { messages: out }; |
There was a problem hiding this comment.
messages() is returning insertion order, not active-path order.
This fake advertises path-ordered session::messages, but it ignores parent_id and streams every stored entry in append order. As soon as a test appends under a non-leaf parent, stale siblings will leak into reads and the helper stops matching the real transcript contract.
Suggested fix
private messages(p: Record<string, unknown>) {
const s = this.session(String(p.session_id));
const roles = Array.isArray(p.roles) ? (p.roles as string[]) : null;
const include_custom = p.include_custom === true;
+ const byId = new Map(s.entries.map((e) => [e.entry_id, e] as const));
+ const activePath: FakeEntry[] = [];
+ for (
+ let cursor = s.entries.at(-1);
+ cursor;
+ cursor = cursor.parent_id ? byId.get(cursor.parent_id) : undefined
+ ) {
+ activePath.push(cursor);
+ }
+ activePath.reverse();
const out: Array<{
entry_id: string;
message?: AgentMessage;
custom?: { custom_type: string; data: unknown };
}> = [];
- for (const e of s.entries) {
+ for (const e of activePath) {
if (e.kind === 'message' && e.message) {
if (roles && !roles.includes(e.message.role)) continue;
out.push({ entry_id: e.entry_id, message: e.message });
} else if (e.kind === 'custom' && e.custom) {
if (roles) continue;🤖 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 `@harness/tests/_helpers/fakeSessionManager.ts` around lines 181 - 201, The
messages(p) helper currently iterates s.entries in append order; instead compute
the active-path entry_ids by starting at s.root_id and repeatedly selecting the
most recently appended child whose parent_id equals the current node (until no
child exists), collect that chain's entry_ids, then filter s.entries to only
include entries whose entry_id is in that active-path set (still applying the
existing role and include_custom checks). Update the messages(p) function to
build that active-path set (using s.entries' parent_id and entry_id fields) and
then produce out from only those entries.
Comment on lines
+220
to
+238
| it('stops on an error assistant, keeping the error content on the session entry', async () => { | ||
| const finalMsg = assistant({ stop_reason: 'error', error_message: 'auth failed' }); | ||
| const rec: TurnStateRecord = { ...newRecord('s1'), state: 'assistant_streaming' }; | ||
| const { iii } = fakeIiiWithDone(finalMsg); | ||
| const { iii, sessions } = fakeIiiWithDone(finalMsg); | ||
|
|
||
| const store = mockStreamingStore(); | ||
| mockStreamingStore(); | ||
| vi.spyOn(preflightModule, 'runPreflight').mockResolvedValue('ok'); | ||
| const appendSpy = store.appendMessages; | ||
|
|
||
| await handleStreaming(iii, rec); | ||
|
|
||
| expect(rec.state).toBe('finishing'); | ||
| expect(rec.turn_end_emitted).toBe(true); | ||
| expect(appendSpy).not.toHaveBeenCalled(); | ||
| const entries = sessions.messageEntries('s1'); | ||
| expect(entries).toHaveLength(1); | ||
| expect(entries[0]?.message?.content).toEqual(finalMsg.content); | ||
|
|
||
| await handleFinishing(iii, rec); | ||
| expect(sessions.session('s1').status).toBe('error'); | ||
| expect(sessions.session('s1').status_reason).toBe('auth failed'); |
There was a problem hiding this comment.
Assert the persisted entry keeps the error metadata too.
This only verifies content, so it still passes if the session transcript drops stop_reason / error_message and only preserves the text blocks. That contract matters now that session-manager is the shared transcript source, and the fake session-manager’s updateMessage path only mutates content/details.
Suggested assertion tightening
const entries = sessions.messageEntries('s1');
expect(entries).toHaveLength(1);
- expect(entries[0]?.message?.content).toEqual(finalMsg.content);
+ expect(entries[0]?.message).toMatchObject({
+ role: 'assistant',
+ content: finalMsg.content,
+ stop_reason: 'error',
+ error_message: 'auth failed',
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('stops on an error assistant, keeping the error content on the session entry', async () => { | |
| const finalMsg = assistant({ stop_reason: 'error', error_message: 'auth failed' }); | |
| const rec: TurnStateRecord = { ...newRecord('s1'), state: 'assistant_streaming' }; | |
| const { iii } = fakeIiiWithDone(finalMsg); | |
| const { iii, sessions } = fakeIiiWithDone(finalMsg); | |
| const store = mockStreamingStore(); | |
| mockStreamingStore(); | |
| vi.spyOn(preflightModule, 'runPreflight').mockResolvedValue('ok'); | |
| const appendSpy = store.appendMessages; | |
| await handleStreaming(iii, rec); | |
| expect(rec.state).toBe('finishing'); | |
| expect(rec.turn_end_emitted).toBe(true); | |
| expect(appendSpy).not.toHaveBeenCalled(); | |
| const entries = sessions.messageEntries('s1'); | |
| expect(entries).toHaveLength(1); | |
| expect(entries[0]?.message?.content).toEqual(finalMsg.content); | |
| await handleFinishing(iii, rec); | |
| expect(sessions.session('s1').status).toBe('error'); | |
| expect(sessions.session('s1').status_reason).toBe('auth failed'); | |
| it('stops on an error assistant, keeping the error content on the session entry', async () => { | |
| const finalMsg = assistant({ stop_reason: 'error', error_message: 'auth failed' }); | |
| const rec: TurnStateRecord = { ...newRecord('s1'), state: 'assistant_streaming' }; | |
| const { iii, sessions } = fakeIiiWithDone(finalMsg); | |
| mockStreamingStore(); | |
| vi.spyOn(preflightModule, 'runPreflight').mockResolvedValue('ok'); | |
| await handleStreaming(iii, rec); | |
| expect(rec.state).toBe('finishing'); | |
| expect(rec.turn_end_emitted).toBe(true); | |
| const entries = sessions.messageEntries('s1'); | |
| expect(entries).toHaveLength(1); | |
| expect(entries[0]?.message).toMatchObject({ | |
| role: 'assistant', | |
| content: finalMsg.content, | |
| stop_reason: 'error', | |
| error_message: 'auth failed', | |
| }); | |
| await handleFinishing(iii, rec); | |
| expect(sessions.session('s1').status).toBe('error'); | |
| expect(sessions.session('s1').status_reason).toBe('auth failed'); |
🤖 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 `@harness/tests/turn-orchestrator/assistant.test.ts` around lines 220 - 238,
The test currently only asserts the persisted entry's content; extend it to also
assert the persisted metadata (stop_reason and error_message) is preserved on
the session transcript. In the test that uses assistant({ stop_reason: 'error',
error_message: 'auth failed' }), after obtaining entries =
sessions.messageEntries('s1') add assertions that
entries[0]?.message?.details?.stop_reason === 'error' and
entries[0]?.message?.details?.error_message === 'auth failed' (or the equivalent
metadata field used by sessions.messageEntries), so the fake session-manager
path is validated to keep the error metadata.
Comment on lines
+102
to
+112
| const writes: Array<{ function_id: string; payload: unknown }> = []; | ||
| const iii = { | ||
| trigger: vi.fn(async ({ function_id, payload }: any) => { | ||
| trigger: vi.fn(async ({ function_id, payload }: { function_id: string; payload: unknown }) => { | ||
| writes.push({ function_id, payload }); | ||
| if ( | ||
| function_id === 'state::get' && | ||
| payload.scope === TURN_STATE_SCOPE && | ||
| payload.key === 's1' | ||
| ) { | ||
| const p = payload as Record<string, unknown>; | ||
| if (function_id === 'state::get' && p.scope === TURN_STATE_SCOPE && p.key === 's1') { | ||
| return record; | ||
| } | ||
| return null; | ||
| }), | ||
| } as any; | ||
| } as unknown as ISdk; |
There was a problem hiding this comment.
Fix fakeIii() payload typing to match downstream assertions
writes.payload is typed as unknown, but later assertions access w.payload.scope, w.payload.value, and w.payload.data; TypeScript will flag this if the test file is typechecked. Store a narrowed payload shape in writes (or cast/narrow at the writes.push site) so assertions don’t rely on unsafe property access. This likely won’t fail harness’s current tsc -b --noEmit since harness/tsconfig.json excludes tests/**/*.
🤖 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 `@harness/tests/turn-orchestrator/run-transition.test.ts` around lines 102 -
112, The writes array currently types payload as unknown which causes unsafe
property access in later assertions; update the writes declaration or the push
site in the fake III implementation (the iii.trigger function) to use a narrowed
payload type (e.g., an interface with scope, key, value, data fields) or cast
the payload to that shape before pushing so downstream assertions like
w.payload.scope / w.payload.value / w.payload.data are type-safe; change the
declaration of writes (and/or the push in iii.trigger) to use that narrowed
payload type to align with expected test usage.
ytallo
approved these changes
Jun 12, 2026
andersonleal
approved these changes
Jun 12, 2026
7 tasks
This was referenced Jun 13, 2026
Merged
This was referenced Jul 3, 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
3 participants
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.
Summary
Replaces the in-harness
session-treeworker with the external session-manager worker (session::*). The harness becomes a driver of durable, reactive transcripts; the console reads and reconciles the same store directly.Key changes
Harness
sessionworker (session-tree::*, tree store/operations, related tests/docs).harness/src/runtime/session.ts— typed client forsession::ensure,append,update_message, paginatedmessages,set_status, and compaction custom entries.session-manageras a harness dependency (iii.worker.yaml).entry_ids.update_messagesnapshots (live surface issession::message_updated, notagent::eventsdeltas).working→done/error) on run finish.custom_type: "compaction"entries to session-manager instead of tree compactions.Console
console/web/src/lib/sessions/module (api,entry-mapper,events,types).use-conversationsis server-backed: sidebar fromsession::list, transcript fromsession::messages, live updates from session trigger events.session::ensure); entry mapper reconciles optimistic user rows and assistant block segments byentry_id/revision.Permissions & tests
iii-permissions.yaml: allow/deny rules forsession::*(store bypass blocked; harness-owned writes allowed).FakeSessionManagertest helper; turn-orchestrator and compaction tests updated; session-tree tests removed.Breaking / migration notes
session-tree::*or locally persisted console transcripts must move tosession::*and event-driven reconciliation.Test plan
pnpm testinharness/)entry-mapper.test.ts)Summary by CodeRabbit
New Features
Documentation
Chores