Conversation
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 19d45d02c9bc2dbc96c46e1ba64c452048f5b84b (11 files, +343/−15). A transcript Turn whose projection exceeds the presentation limits used to fail the whole read; this change makes the reader emit one synthetic system_note for that Turn and leaves the neighbouring Turns readable, and makes Desktop startup tolerate a Session whose transcript cannot seed.
No P0–P2. Two minor (P3) notes below.
What the change does
- Every presentation-limit throw in this reader is now a tagged
TranscriptProjectionLimitError(packages/runtime-host/src/server/session-transcript-reader.ts:66; throwing sites at 586, 592, 615, 663, 680, 682). - Folding a Turn catches exactly two things — that class and
RuntimeTranscriptOversizedTurnErrorfrom storage — marks the Turnoversizedand continues (:257-269); anything else still propagates.projectTurnthen returns one note instead of the projection (:180,:185). omittedTranscriptRecord(:632-646) builds a deterministic row: kindtranscript_omitted, an id derived from the invocation and its first ordinal,tsfrom the invocation's open time, andsequence: firstOrdinal * EVENT_SEQUENCE_STRIDE— the same anchor the paging window already uses (:302-303) and that Turn landmarks use (:423-424), which is what keeps both paging directions advancing.transcript_omittedjoins the user-visible note policy (packages/core/src/session.ts:1259), and the Desktop label and copy land inpackages/ui/src/materialize.ts:183plus all three locales.- Desktop: a failed transcript seed is reported to the renderer and excluded from the "failed to restore" throw, so one bad Session no longer blocks startup (
apps/desktop/src/main/runtime-host-desktop-candidate.ts:770-783).
What I checked and found correct
- The catch is narrow: only storage's explicit oversized signal and this reader's own limits become an omission, so a genuinely broken transcript still fails loudly. That is the property that matters most here, and it holds.
- The omitted row is page-stable. Its sequence sits exactly at the Turn's window start, so
newer,olderand the bootstrap page all include it, and the id depends only on the invocation and first ordinal, so re-reads agree. The new reader test asserts the same shape for all three reads and that the neighbours survive. - An oversized Turn cannot reach the ordinal-mapping code that throws on a missing ordinal (
:220-224), becauseprojectTurnreturns before it, so a partial fold cannot produce “Durable transcript message has no source RuntimeEvent”. - The per-Turn permission-outcome byte limit is evaluated inside
finishfor that Turn's own request ids (:684-690), so folding it into the same omission is consistent rather than masking a session-wide limit. - This depends on storage's per-invocation event budget; the change pins that invariant with a test and no storage source change.
- Base and revision: this revision is one commit behind
main, andmain's newer commit edits the samepackages/ui/src/conversation-copy.ts. A synthetic merge is clean and keeps both sides, and CI ran on the merge, so there is nothing to resolve here.
P3 — the new note is invisible on the CLI surface. The kind is user-visible by policy now, so it flows through the shared transcript projection into the pages clients receive (packages/runtime-host/src/server/shared-session-transcript.ts:158-160). Desktop renders it, but the CLI's note-label switch has no case for it (packages/cli/src/pi-transcript.ts:1348-1397) and falls off the end returning undefined, and its caller drops an entry with no text (:1288-1290). Trigger: attach the TUI to a Session whose page contains an omitted Turn — the neighbouring Turns appear and the oversized one disappears, without the explanation Desktop shows. A case 'transcript_omitted', or a fallback for unknown-but-visible kinds, closes it.
P3 — the Desktop classifies the seed failure by message text. See the inline comment: the tolerance matches the Host's exact sentence, so a rewording silently restores the hard startup failure this change exists to avoid.
What I could not judge
- I did not run the suites here. The
testcheck is green on this revision; the eval job in that run is skipped. - I did not reproduce a real Turn beyond the limits outside the test's synthetic writes.
- The CLI finding rests on my reading of the path (pager → continuity coordinator → client channel). If the TUI never consumes Host-produced pages, that note is latent rather than user-visible.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
| function isTranscriptSeedFailure(error: unknown): boolean { | ||
| return ( | ||
| error instanceof RuntimeHostOperationError && | ||
| error.operation === 'subscription.open' && | ||
| error.code === 'persistence_failed' && | ||
| error.message === 'Session transcript is unavailable' | ||
| ); | ||
| } |
There was a problem hiding this comment.
This tolerance is keyed on the Host's exact sentence, which makes it a contract that only the two source files and a test can see. The Host builds that message in several places (packages/runtime-host/src/server/session-continuity-coordinator.ts:978, :1011, :1168), and the test here hard-codes the same string — so a reword on either side, or any wrapper that prefixes it along the way, silently turns the tolerance off and restores the hard startup failure this change exists to avoid. Nothing in the type system or in CI connects the two.
operation and code narrow it usefully, but persistence_failed is too broad to be the discriminator on its own. A dedicated code or reason on the error — something like transcript_unavailable — would let both sides agree without depending on wording, and the Desktop could keep matching on the operation plus that code.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up deep review at this head by independent AI reviewers from three model families. The reader-side placeholder is reasonable and nothing is lost in storage (raw events remain and the session bundle export includes them), but this found one P1 and two further P2s, so we would hold the merge. Details inline.
- P1 — WorkHub delegation gets a silent wrong result.
createTurnResultReader→readLatestAssistantForTurnreuses the sameprojectTurn, so an oversized delegated turn now yields an omitted note and no assistant message; the result is an empty string, or text from an earlier run of that turn.workhub-result-coordinator.ts:199-205then marks it delivered and never retries. Before this PR the task stayed pending and logged an error; now the delegator is told it completed with the wrong content. - P2 — the Desktop tolerance cannot keep the replacement Host alive. Confirmed independently by two reviewers (inline).
- P2 — compatibility epoch not bumped.
transcript_omittedis a new system-note kind on the wire (packages/core/src/session.ts), but the epoch stays at 192; older clients on 192 will not decode the placeholder. The repo's epoch-106 note says new note kinds need a bump. - P2 — live catch-up drops rows silently. Desktop's live catch-up starts mid-run and filters the placeholder (
runtime-transcript-query.ts:313with the reader at:310), so once a running turn passes the limit its later rows stop appearing with no omission note.
Earlier P3s still hold: the TUI does not render the note, so the whole turn disappears there; and the Desktop check matches the Host's exact error text. The new storage test also passes on the old code.
Not verified: a real 9.2k-event user store, packaged Desktop, and an end-to-end Host → Desktop drain test.
Automated review notice: This review was posted by an automated review agent operated by Astro-Han. It combines independent reviews from several AI models. It is not an independent human review and does not replace one.
| const failedSessionIds = sessionObservations | ||
| .observedSessionIds() | ||
| .filter((sessionId) => !restoredSessionIdSet.has(sessionId)); | ||
| .filter((sessionId) => !restoredSessionIdSet.has(sessionId) && !transcriptSeedFailures.has(sessionId)); |
There was a problem hiding this comment.
P2 — Excluding transcript-seed failures here assumes the Host keeps serving after one, but it does not: in session-continuity-coordinator.ts:991-1013 a failed subscription.open bootstrap calls onPublicationFailure(error) before returning persistence_failed, and production wiring (execution-composition.ts:987-992) connects that to requestDrain, after which host-kernel.ts refuses new ready/operations. Other sessions may restore briefly, but the replacement Host is already draining. The new Desktop test uses a fake connection that only throws and does not model the drain. Either give isolatable transcript-seed errors a no-drain recovery boundary on the Host (with a Host → Desktop replacement-start test), or keep this failure fatal.
| 'context_reported_window_exceeded', | ||
| 'context_overflow_after_compaction', | ||
| 'step_limit', | ||
| 'transcript_omitted', |
There was a problem hiding this comment.
P2 — Adding transcript_omitted to the runtime note kinds changes what can appear on the wire, but the protocol compatibility epoch is unchanged at 192. A client on 192 that predates this kind will fail to decode the placeholder. Please bump the epoch as the repo's note-kind rule requires.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
The issue is real and reachable: in base, an invocation exceeding the transcript source budget (>8,192 events) made storage throw RuntimeTranscriptOversizedTurnError (packages/storage/src/runtime-transcript-query.ts:431-443), which propagated through the reader's readRun into createSessionTranscriptBootstrap, so subscription.open failed with persistence_failed (packages/runtime-host/src/server/session-continuity-coordinator.ts:1005-1014) and createDesktopRuntimeHostCandidate threw, killing Host replacement for all observed sessions. The fix is at the right layers: the reader emits one bounded placeholder anchored at firstOrdinal * EVENT_SEQUENCE_STRIDE and paging advances by stored ordinal extent (session-transcript-reader.ts:321), so both directions terminate without duplicates; the desktop tolerates per-session seed failures. Tests genuinely exercise the fix (reverting the reader catch or the desktop filter fails them). CI is green.
Findings
- [P3]
apps/desktop/src/main/runtime-host-session-observation-registry.ts:101-108—isTranscriptSeedFailurematches the exact human string'Session transcript is unavailable'. The coordinator emits that message from four paths with two codes (session-continuity-coordinator.ts:978, 1011, 1122, 1168) and maps any bootstrap error to it, so desktop now tolerates every transcript bootstrap failure (broader than "oversized"), and a future copy edit silently reverts to the old one-bad-session-fails-everything behavior. Pinned by tests on both sides today; a dedicated error code would be robust. - [P3]
packages/cli/src/pi-transcript.ts:1348-1394—systemNoteTexthas notranscript_omittedcase and no default; since the kind now passes theisRuntimeSystemNoteKindguard (:1351),systemNoteToTranscriptEntry(:1287-1297) returnsundefinedand the CLI transcript silently renders nothing where history was omitted. Desktop UI and guest projection handle it; CLI does not. - [P3]
packages/core/src/session.ts:1259—transcript_omittedis added toRUNTIME_SYSTEM_NOTE_KINDS, the closed list of notes the runtime writes, though this note is reader-synthesized and never persisted. Nothing writes it today, but writer-side consumers such asruntime-event-backfill.ts:374would now accept it as a runtime-authored note if one were ever stored.
Verdict
merge-ready — correct fix at the right layers with real regression coverage; three minor follow-ups (error-code signal, CLI label, kind-list hygiene) don't block.
|
One additional pagination edge case on I reproduced this by removing the preceding normal invocation from the existing oversized-invocation test. With a bounded page size, the older page returns
中文对照在 我移除了现有超限 invocation 测试中前面的正常 invocation,复现了这个问题。限制页面大小后,向前翻阅历史的页面返回
|
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
…kinds Generated-by: OpenAI Codex
|
Thanks for the detailed review. The new commits address the reported failure paths:
Focused Runtime Host and core regressions, Desktop candidate tests (27/27), affected builds/typechecks, and changed-file lint passed on the updated branch. I have not run the packaged Desktop against the reported 9.2k-event store or a full Host-to-Desktop drain integration test. Automated update prepared and posted by OpenAI Codex under the contributor's direction. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head. I found no additional substantiated P0–P3 issue in the changed paths. The fixes address the earlier review findings: delegated result reads reject an omitted turn rather than returning empty/stale text (packages/runtime-host/src/server/session-transcript-reader.ts:333-353); a failed transcript bootstrap is isolated to its subscription (packages/runtime-host/src/server/session-continuity-coordinator.ts:1000-1016) and Desktop identifies it by code (apps/desktop/src/main/runtime-host-session-observation-registry.ts:103-108); the omission anchors to the first actual event in a traversed run (packages/storage/src/runtime-transcript-query.ts:309-329), the CLI renders it (packages/cli/src/pi-transcript.ts:1398-1399), and the wire epoch is 198 (packages/runtime-host/src/protocol/index.ts:107-109). The synthetic note is not included in runtime-authored note kinds (packages/core/src/session.ts:1253-1273).
I checked the full 20-file PR diff against its base, the repair increment, the Desktop restore and WorkHub retry paths, and existing reviews. Node 24 npm ci, build:test, and 219 focused tests passed. The current-head CI test and package checks are green; a static merge-tree against freshly fetched main was clean and git diff --check passed. No schema/migration change. I did not exercise a packaged Desktop against the reported 9.2k-event store or a real Host-to-Desktop drain/reconnect, so those remain validation gaps. This is a commented review, not an approval or merge decision.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Review at 5902baa5 (Claude lineage). Verdict: 1 × P2, 3 × P3. The earlier P1 (a wrong WorkHub result being delivered) is gone, but the replacement "throw and keep retry" path drains the whole Host instead of retrying.
Fixed:
- Host transcript bootstrap errors are isolated to their subscription, with a test.
- Desktop recovers other Sessions on
transcript_unavailable. - The omission note is anchored at the first actual event. A mutation back to
firstOrdinalfails the new test. - The CLI shows the note.
- The epoch is 197→198.
P2: the omitted delegated result drains the Host on every sweep (inline).
P3s:
- Duplicate omission notes in the live view. While an oversized invocation is still running, each watermark advance appends another omission note with a new id (
session-transcript-reader.ts:635-647,runtime-transcript-query.ts:312-329). Reproduced on a real SQLite store: the live view showslarge:8003andlarge:8303, while a fresh read shows onlylarge:1. Rows rendered before the limit was crossed also remain. A likely side effect, from reading the code: Desktop mark-read ids stop matching the Host's until the Session is reopened. decodeRuntimeEventnow acceptsnote: 'transcript_omitted'as a stored event.isSystemNoteKind(session.ts:1309-1315) now includes the synthetic kind, soruntime-event.ts:1128accepts it; base rejected it. This contradicts the comment atsession.ts:1281. No writer produces it today.- The CLI omission label (
pi-transcript.ts:1398-1399) has no test. The new storage test also passes on the old code.
Epoch (coordination only): 198 is also claimed by #4751, #5709 and #5670, and #5394 uses 200. Whichever merges later must renumber.
Verified in a scratch copy (Node 22): the 3 affected runtime-host files pass 79/79, plus the new core and storage tests. Two mutations confirm the new tests catch the old behavior. The Desktop and CLI suites were not run.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
| if (run.invocation.turnId === turnId) { | ||
| for (const { message } of await projectTurn(run)) { | ||
| if (message.type === 'system_note' && message.kind === 'transcript_omitted') { | ||
| throw new Error(`Delegated turn ${turnId} has an omitted transcript result`); |
There was a problem hiding this comment.
P2. This throws a plain Error. It is raised from prepareContent inside root-turn-coordinator.ts:1820's runCommand(...). runCommand (:3461-3472) calls requestHostDrain() for any error that is not a hosted-root conflict, unavailable, admission-gate or shutdown error, and production wires that to context.requestDrain (execution-composition.ts:1748).
So when a delegated Turn with an oversized (>8,192-event) invocation is swept, the whole Host drains instead of the result being retried. Each new Host hits it again on startup (:3131), so the Host keeps draining in a loop. This contradicts the "keeps retry" intent.
workhub-result-runtime.test.ts stubs startWorkHubResult, so this is untested. Suggested fix: throw a typed error that runCommand excludes from drain (or resolve 'obsolete'/a terminal failure for the result), and add a coordinator-level test.
There was a problem hiding this comment.
Good catch. readLatestAssistantForTurn now throws OmittedTurnResultError for this case, and runCommand excludes that typed error from Host drain. HostWorkHubResultCoordinator still catches it and schedules a retry, so no result is marked delivered. I added a root-coordinator regression that asserts the error propagates without draining; it compiles, but the local sandbox blocks the test fixture at Maka’s account-level state-root lock directory (EPERM), so I cannot claim a local pass for that test. The two oversized reader regressions pass; the full build, lint, format check, and typecheck pass on the updated branch. The merge with current main also moved the compatibility epoch to 199.
Automated update prepared and posted by OpenAI Codex under the contributor’s direction.
hqhq1025
left a comment
There was a problem hiding this comment.
Correction to my earlier review at this same head (#pullrequestreview-5339145169): I withdraw its “no additional P0–P3” conclusion and merge-readiness implication. The independent Claude-lineage review at #pullrequestreview-5339286402 identified a P2 that I missed; I have now checked the relevant production call path myself.
[P2] An omitted delegated result still drains the whole Host. packages/runtime-host/src/server/session-transcript-reader.ts:345-347 throws an ordinary Error when a WorkHub result's transcript is omitted. packages/runtime-host/src/server/workhub-result-runtime.ts:189 routes delivery through startWorkHubResult, which wraps preparation in runCommand (packages/runtime-host/src/server/root-turn-coordinator.ts:1816-1840). Its catch at :3461-3473 calls requestHostDrain() for this error. The WorkHub coordinator's per-assignment catch can mark the item for retry, but it cannot keep this Host serving that retry. For a persistently oversized delegated turn, replacement Hosts encounter the same error again. This PR is not ready for merge until this failure is handled without a Host-wide drain while preserving the no-wrong-result guarantee.
My earlier Node 24 build and 219 focused tests passed, but they did not cover this Host lifecycle interaction; that passing result does not negate this finding. I did not dynamically reproduce the Host restart loop. I did not approve or merge.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Generated-by: OpenAI Codex
…anscript-recovery # Conflicts: # packages/runtime-host/src/protocol/index.ts
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed head 00725958efc58b5a6bd23523259064c277adbaf1. The prior Host-drain issue is addressed: an omitted delegated transcript result now raises OmittedTurnResultError, and RootTurnCoordinator.runCommand propagates that error without requesting a Host drain. The WorkHub result coordinator retains its retry path. The new root-coordinator regression exercises the error path and asserts that drain is not requested. I found no additional substantiated P0–P3 issue in this delta.
Node 24 build:test and focused tests passed locally (the new root-coordinator case 1/1, transcript-reader and Desktop candidate tests 42/42). Current-head CI test, package/platform validation, and audit checks passed. A static merge against current main (2f322055) and git diff --check were clean; the Host compatibility epoch is 199. I did not run a packaged Desktop against the reported large user database or a full Host-to-Desktop drain/reconnect exercise. GitHub still reports the PR as blocked; this is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review at 00725958 (Claude lineage). Our P2 is fixed; no new P0–P2 found in the fix.
session-transcript-reader.ts:70-73,349-355now throws a dedicatedOmittedTurnResultError.root-turn-coordinator.ts:3465-3478excludes that error fromrequestHostDrain(), so an oversized delegated result no longer drains the Host.workhub-result-coordinator.ts:166-177still catches the error and retries.- A new root-coordinator test pins the no-drain behavior.
The epoch is now 199. That follows main, which moved to 198 with #5670.
Our earlier P3s (repeated omission notes in the live view while an oversized invocation is still running, decodeRuntimeEvent accepting the synthetic note kind, and no CLI label test) were not re-verified in this pass.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
Summary
Recover readable session history when one valid invocation exceeds transcript source or presentation limits. The reader emits a bounded omission note while keeping adjacent turns pageable. The note now anchors to an actual stored event ordinal, so backward and forward cursors agree even when the oversized invocation starts the session; live catch-up beginning inside the invocation also shows the omission.
A delegated WorkHub turn whose transcript is omitted now fails its result read instead of delivering an empty or stale answer. A transcript bootstrap failure is isolated to its subscription: the Host logs it without entering drain, and Desktop recognizes the dedicated
transcript_unavailablecode while restoring other observations. The CLI shows the omission note. The wire change advances the Runtime Host compatibility epoch to 199 after merging currentmain. An omitted delegated result now raises a retryable typed error without draining the Host.Fixes #5750
Verification
mainmerge, including independent restoration after one transcript seed failure.@maka/core,@maka/storage,@maka/runtime,@maka/runtime-host,@maka/ui, Desktop main, and CLI builds passed. Desktop and CLI typechecks passed. Biome lint passed for the changed files.EPERM). CI is expected to run that regression.EPERM). The focused oversized-invocation tests passed.A packaged Desktop build with the reported 9.2k-event user database and a full Host-to-Desktop drain integration test were not run locally.
AI use
Tool(s) and scope: OpenAI Codex authored the implementation and tests under the contributor's direction. Authored commits contain
Generated-by: OpenAI Codextrailers.Checklist
Does this PR entail a change in behavior?