fix(cli): keep a finished turn's endedAt across teardown finalize - #409
Open
audichuang wants to merge 1 commit into
Open
fix(cli): keep a finished turn's endedAt across teardown finalize#409audichuang wants to merge 1 commit into
audichuang wants to merge 1 commit into
Conversation
The renderer derives "Worked for X" from `endedAt - timestamp`, so the displayed duration of a long-finished turn grew by however long the app stayed open after it. `finalizeACPState`'s no-turnId overload matched the last assistant entry regardless of state and wrote `endedAt = Date.now()` unconditionally. At app close `sessionManager.cleanUp()` raises `exit`/`terminated` for every live session, and both handlers run that overload after checking only that transient state exists, so a session whose turn ended an hour earlier was re-stamped with the close-time clock. The history loop moves to `assistant-turn-finalize.ts` with one guard: an entry already marked `finished` is left alone, so the first terminal stamp is final for every no-turnId caller. Guarding on `finished` rather than filling in a missing `endedAt` is deliberate — image-group and file entries publish `finished: true` with no `endedAt`, and an `endedAt`-only guard would still stamp close time on them. An interrupted turn is not finished, so its teardown stamp still lands, and resume still reopens the entry through `writeAssistantEntryForTurn`. Closes LodyAI#260 Model: claude-opus-5
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
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.
Related issue
Closes #260
Problem / pressure
A finished assistant turn's "Worked for X" grew by however long the app stayed
open after that turn ended. It does not tick up while the app is running: the
stored
endedAtis rewritten exactly once, at teardown, so the displayednumber jumps the next time the session is loaded.
The renderer derives the duration from
endedAt - timestamp(
packages/components/src/lib/session-history-duration.tsresolveSessionHistoryDurationMs), so the inflation comes fromendedAtbeingrewritten after the turn is over.
finalizeACPState(apps/cli/src/lib/message-handler.ts) has a no-turnIdoverload. Without a
turnIdits history loop matches the last assistant entryregardless of whether that entry already finished, and unconditionally writes
endedAt = Date.now(). On app close,sessionManager.cleanUp()raisesexit/terminatedfor every live session, and both handlers call that overloadafter checking only
this.store.has(sessionId)— no pending-work check. Asession whose turn ended an hour earlier is re-stamped with the close-time clock.
(
flushAllACPUpdates()is guarded byhasPendingTurnWorkand skips an idleturn with an empty buffer, so it is not the unconditional trigger; the two
teardown handlers are.)
apps/cli/src/session/AGENTS.mdalready documents the intended contract — theno-turnId overload "stamps
finished=true/endedAton the in-progressentry". The implementation did not honour that.
Summary
apps/cli/src/lib/assistant-turn-finalize.tsexportsmarkAssistantTurnFinished(history, { turnId, endedAt, permissionWaitMs }):the finalize history loop, verbatim, plus one guard — an entry already marked
finishedis left untouched.finalizeACPStatenow calls it instead of inlining the loop, so the guardcovers every no-turnId call site, not just the teardown pair above.
finished === truerather than filling in a missingendedAt:createAssistantImageGroupEntryandcreateAssistantFileEntrypublish assistant entries with
finished: trueand noendedAt, so anendedAt ??=guard would still stamp close time on them. LeavingendedAtabsent makes
resolveSessionHistoryDurationMsreturnnulland the row showsno duration, which is the honest answer.
finished, so its teardown stamp stilllands; resume still clears the footprint through
writeAssistantEntryForTurn'sreopen branch. Behaviour there is unchanged.
apps/cli/src/session/AGENTS.mdrecords that the overload is a no-op on analready-finished entry.
Before / after
endedAttoDate.now(), so closing the app inflated a finished turn's "Worked for X" by the time the app had been open.finalizeACPState, with nothing asserting that a second finalize is harmless.finalizeACPStateitself.Test plan
apps/cli/tests/message-handler-turn-duration.test.tsdrives the realfinalizeACPStateagainst a realSessionDocument, withDate.nowstubbedrather than any clock being waited on: finalizing a turn, then finalizing the
same session an hour later (what app close does), leaves the first
endedAtin place; a turn that was never finished still gets stamped at teardown.
apps/cli/src/lib/assistant-turn-finalize.test.tscovers the rule itself(6 cases, two explicit
endedAtvalues standing in for "turn ended" and "appclosed"): open entry is stamped; a second stamp does not move the first; an
entry with
finished: trueand noendedAtgains none; an explicitturnIdis honoured past a trailing image entry;
permissionWaitMsis recorded; atrailing non-assistant entry is never stamped.
corepack pnpm format— clean; all three new files passprettier --check.corepack pnpm checkdoes not complete on this machine: it aborts in itsfirst step (
typecheck->lody prepare:acp-adapters) on a pre-existingacp-extension-dshbuild error,src/adapter.ts(1649,7) TS2352onReadableStreamgenerics. That submodule is untouched here(
git diff --submoduleandgit statusare clean for it) and the localtoolchain is Node v24.16.0, so this may be a local
@types/nodelibmismatch rather than something CI sees.
checkstage was run individually: recursivetypecheckwithacp-extension-dshexcluded (coverslody, all packages, andsite-docs)— pass;
corepack pnpm lint(oxlint type-aware) — 0 errors;corepack pnpm lint:i18n,check:code-collab-imports,check:platform-boundaries,check:public-boundary— all pass.mainand re-verified there: applies with no conflict,apps/clitypechecks clean, both new suites pass (8 cases).corepack pnpm test:cion the pre-rebase base:apps/cli248 files / 2474tests and every package pass. That count predates both new suites; a full
apps/clirun on the rebased branch reports 250 files / 2489 tests, the deltabeing the two new suites plus suites
maingained during the rebase window.One
apps/electronfile,src/main/services/loro-data-plane-relay.test.mjs,fails on
Error: Electron failed to install correctly— the Electron binaryis missing from this machine's install, not a code failure, and nothing here
touches
apps/electron.test:cirun predates both new suites, so they were also run togetherwith the neighbouring
message-handler-*suites (5 files, 29 tests, all pass).apps/clitypecheckwas re-run afterwards; note its tsconfig excludes testfiles, so that step covers
assistant-turn-finalize.tsbut not either newtest file — those are typechecked only by
vitest.(
exit/terminated->finalizeACPState) is unchanged; only the historywrite it performs is guarded.
Context handoff
Instructions for reviewing agents
apps/cli/src/lib/assistant-turn-finalize.ts(one guard, extracted verbatim otherwise) and thefinalizeACPStatecall site inmessage-handler.ts.finished === trueinstead of a missingendedAt, and extracting the loop into its own module rather than leaving it inline.nowon genuinely abandoned older turns; that trade-off is yours, not this PR's. Also intended but worth confirming: when a teardown finalize precedes a turnId finalize, the first stamp wins and the laterpermissionWaitMsis dropped.Authoring context
AGENTS.md.finished: truewould suppress its real timing; all currentfinished: truewriters inapps/cli/srcwere checked and writesystem/userentries or separate assistant image/file entries.finalizeACPStateis driven directly against a realSessionDocumentinstead, which is the boundary the bug lives at.