Repository navigation
fix(runtime): classify handoff-pause terminal RuntimeEvents as handed off - #5853
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the exact head. The new handed_off terminal classification matches the status-less event written at AgentRun.reachHandoffBoundary, and restores RuntimeReadModel.getSessionView for that ledger shape. However, a selected handoff run still fails the conversation-copy preflight before the new clone branch can run (P2 below), so branch/copy and revisions that retain that turn remain unavailable. The new error logging also drops the structured diagnostic identity (P3 below).
Validation: Node 24 clean install and build:test; 172 focused runtime/Host tests, Biome, ASF headers, app-shell hooks, diff-check, and merge-tree against current main b3933043 passed. Hosted test and label passed on this head. I did not reproduce against the reported production state root or exercise a live Desktop handoff/revision flow. No schema migration is introduced. These findings need resolution before treating the user-facing copy/revision path as fixed.
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.
…identity Review feedback on apache#5853: - P2: the handed_off skip in the copy commit loop was unreachable — the copy preflight (assertConversationRuntimeLedgerCopySupported) deliberately rejects the whole handoff lineage (pause events, continuation-start, non-fresh openings) with branch_runtime_fact_rewrite_unsupported before cloning. Remove the silent skip and keep only a defensive guard that documents the unreachable boundary and satisfies the widened runStatus union. Typed identity rewriting of handoff lineage is follow-up work, so copies retaining a handed-off turn now fail with a deliberate operation_unavailable instead of the accidental read-model error. - P3: describeConversationCopyFailure logs a bounded, redacted summary of RuntimeReadModelError.diagnostics (code + eventId/runId/turnId) instead of only the generic error message, so a recurring regression identifies the offending event. - Refresh the runtime-handoff test comment to document the deliberate scope boundary.
|
Thanks for the review — both findings addressed in P2 (dead clone path / copy preflight): Confirmed — P3 (diagnostic identity in logs): Both catch sites now log via |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 7f9e464. The follow-up removes the unreachable handed-off clone skip and explicitly limits support to read-model recovery and revision slices that exclude the handoff lineage. Copies or revisions retaining that lineage still return operation_unavailable by design; typed identity rewriting remains a follow-up, not a fix in this PR. The two coordinator failure paths now include a bounded, redacted summary of RuntimeReadModelError diagnostic codes and event/run/turn IDs. I found no new P0–P3 issue in this incremental change.
Validation: Node 24 build:test, 144 focused runtime/Host tests, Biome on the three changed files, ASF headers, and git diff --check passed. I did not run the reported production state root or a packaged Desktop handoff/revision flow. No hosted checks are reported for this head. Against current main 0aa2707, merge-tree has a content conflict in packages/runtime-host/src/server/session-revision-coordinator.ts (GitHub also reports CONFLICTING), so the PR is not merge-ready; resolve it and revalidate the resulting head.
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.
… off A handoff pause seals a run with actions.endInvocation plus actions.handoffPause and deliberately no terminal status — completed, failed and aborted would each lie about a run that was none of them. The terminal-fact classifier refused the shape, so any session containing a paused run became unreadable through RuntimeReadModel.getSessionView: edit & resend (session.revision.create), branch and copy failed with persistence_failed, and the blanket catch in the revision coordinator hid the real error. - classifyRuntimeEventTerminalFact now classifies a handoff-pause terminal event as a distinguishable handed_off fact; the turn's terminal state stays with the successor run - conversation copy keeps a handed-off run's pause terminal as imported instead of committing it (no committable status by design) - recovery skips handed-off runs: the handoff claim saga owns them - canonical turn snapshot treats a handed-off tip as still running - the revision coordinator logs the error it swallows (with diagnostics) Verified against a copy of a production state root: the one session that previously failed getSessionView (of 41) now reads clean, with no data migration. Fixes apache#5836
jackwener
left a comment
There was a problem hiding this comment.
Reviewed head 7f9e46408760c0ad8b8317dce74e25b2c71a437b. No P0–P2 in the code, but the PR is CONFLICTING with main and has no CI results, so it needs a rebase before approval.
The reader-side fix is the right shape. The ledger is immutable, so classifying the existing endInvocation + handoffPause event (with no status) as its own handed_off fact heals existing sessions without a migration. I traced every consumer of the widened union:
RuntimeReadModel(runtime-read-model.ts:134): a terminal invocation with a handoff pause now yields a fact instead of throwing, which is the reported bug.classifyTerminalRuntimeLedger: this now returnsfactrather thannonefor the pause. Its callers handle that:canonical-turn-snapshotmaps it torunning, since the successor owns the turn.conversation-copystill rejects handoff lineage inassertConversationRuntimeLedgerCopySupportedbefore the terminal check, andcloneConversationRuntimeLedgerkeeps a defensive throw.- The recovery loop only uses it for the
corruptcheck.
- Recovery: handed-off runs are already skipped earlier by the
runtimeHandoffPause(run.terminalEvent)guard, andclassifyRuntimeEventRecoveryalso returnsundefinedfor them. TheclassifyAgentRunRecoveryfallback is therefore unreachable for these runs, and no outcome is manufactured. - Diagnostics: the two blanket
catchsites in the revision coordinator now log a bounded, redacted summary of theRuntimeReadModelErrordiagnostics instead of swallowing them.
P3 (behavior change, worth a test): runtime-resume.ts:668 also consumes classifyRuntimeEventTerminalFact. When a continuation target run has itself paused for handoff (a chained handoff), the plan used to park as continuation_claim_repair_required ("invalid or ambiguous terminal fact"). It now parks as continuation_already_exists. That reads as more accurate, since the continuation did start and moved on, but it is an unexercised change in resume planning. A regression test for the chained case would pin it down.
Not run locally.
…identity Review feedback on apache#5853: - P2: the handed_off skip in the copy commit loop was unreachable — the copy preflight (assertConversationRuntimeLedgerCopySupported) deliberately rejects the whole handoff lineage (pause events, continuation-start, non-fresh openings) with branch_runtime_fact_rewrite_unsupported before cloning. Remove the silent skip and keep only a defensive guard that documents the unreachable boundary and satisfies the widened runStatus union. Typed identity rewriting of handoff lineage is follow-up work, so copies retaining a handed-off turn now fail with a deliberate operation_unavailable instead of the accidental read-model error. - P3: describeConversationCopyFailure logs a bounded, redacted summary of RuntimeReadModelError.diagnostics (code + eventId/runId/turnId) instead of only the generic error message, so a recurring regression identifies the offending event. - Refresh the runtime-handoff test comment to document the deliberate scope boundary.
7f9e464 to
443e911
Compare
…eady_exists Review feedback (jackwener, apache#5853): runtime-resume classifyExistingClaim consumes classifyRuntimeEventTerminalFact, so a continuation target run that itself paused for handoff now classifies as handed_off and parks as continuation_already_exists instead of continuation_claim_repair_required ("invalid or ambiguous terminal fact"). The new outcome is more accurate — the continuation did start and moved on to the next claim in the chain — and the regression test drives the full chained flow through the real machinery (run paused, successor resumed, successor paused again) to pin it down.
|
Thanks for the reviews — both outstanding items are resolved in Rebase (jackwener's blocker): The branch is rebased onto current main ( P3 (chained-handoff resume behavior): Pinned with a regression test, Current head |
jackwener
left a comment
There was a problem hiding this comment.
Approving at 912180eb1e3be3a0ce4412db47f3d88bb337ff69: no P0–P2, and CI test is green on this head.
- The range-diff against
7f9e4640(reviewed in #5853 (review)) shows the first two commits rebased onto main with only import and context changes insession-revision-coordinator.ts, and no logic change. 912180eadds aruntime-handoffregression that pins the chained-handoff resume plan ascontinuation_already_exists, which closes my P3.
Not run locally.
Applies PR apache#5965's head state for review, then ablates: drops redundant matchHistoryCompactCheckpointPrefix calls in planHistoryCompaction and cloneAgentRunEvent (consuming currency.match instead), removes the isHistoryCompactContentEvent re-export, and extracts archiveTransitionFor plus EMPTY_PROJECTION_SNAPSHOT into history-compact-test-fixtures.ts. Also merges two identical-setup currency tests and drops a apache#5853 hunk that does not compile on this base. Checkpoint: peOxw_Ag_DxY_uEPXiz8l
Summary
Fixes #5836.
A handoff pause seals a run with
actions.endInvocation+actions.handoffPauseand deliberately no terminalstatus. The terminal-fact classifier refused that shape, so any session containing a paused run became unreadable throughRuntimeReadModel.getSessionView— transcript-derived reads failed, andsession.revision.create(edit & resend) failed deterministically withpersistence_failed, surfaced as a generic "operation failed" toast.Root cause
Writer/reader asymmetry:
AgentRun.reachHandoffBoundarywrites the pause event without a terminal status (none of completed/failed/aborted/cancelled describes "paused for handoff"), whileclassifyRuntimeEventTerminalFactrequires one and refuses to classify, making the read model throwRuntimeReadModelErrorfor the whole session. The revision coordinator's blanketcatchthen hid the real error behindpersistence_failed.Because the ledger is immutable, already-written events will never gain a status — so this is a reader-side fix; it also heals existing affected sessions with no data migration.
Changes
runtime-event-read-model.ts:classifyRuntimeEventTerminalFactclassifies a handoff-pause terminal event as a distinguishablehanded_offfact (newrunStatus/turnStatusunion member); the turn's terminal state stays with the successor run. The projection path already special-casedhandoffPause; only the classifier was missing the branch.session-manager.ts: recovery skips handed-off runs — the handoff claim saga owns their continuation; generic restart repair must not manufacture an outcome.canonical-turn-snapshot.ts: a handed-off tip reads asrunning(the logical execution continues in the successor).conversation-copy.ts: a defensive guard at the terminal commit keeps the widenedrunStatusunion honest; handed-off runs never reach it because the copy preflight rejects handoff lineage (see Scope).session-revision-coordinator.ts: both blanketcatchsites now log the swallowed error including a bounded, redacted summary ofRuntimeReadModelError.diagnostics(code + eventId/runId/turnId).endInvocation: true+handoffPause, no status); refreshed the runtime-handoff test comment to document the scope boundary.Scope
getSessionView-based read of a session containing a handed-off run — includingsession.revision.createwhose slice excludes the handed-off turn (e.g. revising the handed-off turn itself, the reported scenario).branch_runtime_fact_rewrite_unsupported→operation_unavailablefrom the copy preflight instead of the accidental read-model error. Typed identity rewriting of handoff/continuation lineage (boundary digests, claim identity, successor references) is follow-up work.Verification
handed_offwith no diagnostics.runtime-event-read-model(80),runtime-handoff+conversation-copy(46, in the combined 126-test run),session-manager-terminal-ledger(44),session-revision-diagnostics+session-revision-graph-references(18); typecheck clean for@maka/runtimeand@maka/runtime-host.getSessionView; with this fix all 41 read clean — including the handoff-paused one — with no data migration.Follow-ups intentionally out of scope (per #5836): typed identity rewriting of handoff lineage in conversation copy, writer-side hardening (explicit
pausedstatus — must roll out after this reader fix is widespread), models.dev drift log level.Developed with Maka.