Skip to content

fix(cli): keep a finished turn's endedAt across teardown finalize - #409

Open
audichuang wants to merge 1 commit into
LodyAI:mainfrom
audichuang:audichuang/fix-260-worked-for-duration
Open

fix(cli): keep a finished turn's endedAt across teardown finalize#409
audichuang wants to merge 1 commit into
LodyAI:mainfrom
audichuang:audichuang/fix-260-worked-for-duration

Conversation

@audichuang

Copy link
Copy Markdown
Contributor

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 endedAt is rewritten exactly once, at teardown, so the displayed
number jumps the next time the session is loaded.

The renderer derives the duration from endedAt - timestamp
(packages/components/src/lib/session-history-duration.ts
resolveSessionHistoryDurationMs), so the inflation comes from endedAt being
rewritten after the turn is over.

finalizeACPState (apps/cli/src/lib/message-handler.ts) has a no-turnId
overload. Without a turnId its history loop matches the last assistant entry
regardless of whether that entry already finished, and unconditionally writes
endedAt = Date.now(). On app close, sessionManager.cleanUp() raises
exit/terminated for every live session, and both handlers call that overload
after checking only this.store.has(sessionId) — no pending-work check. A
session whose turn ended an hour earlier is re-stamped with the close-time clock.

(flushAllACPUpdates() is guarded by hasPendingTurnWork and skips an idle
turn with an empty buffer, so it is not the unconditional trigger; the two
teardown handlers are.)

apps/cli/src/session/AGENTS.md already documents the intended contract — the
no-turnId overload "stamps finished=true/endedAt on the in-progress
entry". The implementation did not honour that.

Summary

  • New apps/cli/src/lib/assistant-turn-finalize.ts exports
    markAssistantTurnFinished(history, { turnId, endedAt, permissionWaitMs }):
    the finalize history loop, verbatim, plus one guard — an entry already marked
    finished is left untouched.
  • finalizeACPState now calls it instead of inlining the loop, so the guard
    covers every no-turnId call site, not just the teardown pair above.
  • The guard tests finished === true rather than filling in a missing
    endedAt: createAssistantImageGroupEntry and createAssistantFileEntry
    publish assistant entries with finished: true and no endedAt, so an
    endedAt ??= guard would still stamp close time on them. Leaving endedAt
    absent makes resolveSessionHistoryDurationMs return null and the row shows
    no duration, which is the honest answer.
  • A turn interrupted mid-flight is not finished, so its teardown stamp still
    lands; resume still clears the footprint through writeAssistantEntryForTurn's
    reopen branch. Behaviour there is unchanged.
  • apps/cli/src/session/AGENTS.md records that the overload is a no-op on an
    already-finished entry.

Before / after

Before After
Every no-turnId finalize rewrote the last assistant entry's endedAt to Date.now(), so closing the app inflated a finished turn's "Worked for X" by the time the app had been open. The first terminal stamp is final; a later teardown finalize for an already-finished turn writes nothing and the displayed duration stays put.
The finalize history loop was inline in finalizeACPState, with nothing asserting that a second finalize is harmless. The rule is one named function, covered directly and through finalizeACPState itself.

Test plan

  • New apps/cli/tests/message-handler-turn-duration.test.ts drives the real
    finalizeACPState against a real SessionDocument, with Date.now stubbed
    rather than any clock being waited on: finalizing a turn, then finalizing the
    same session an hour later (what app close does), leaves the first endedAt
    in place; a turn that was never finished still gets stamped at teardown.
  • New apps/cli/src/lib/assistant-turn-finalize.test.ts covers the rule itself
    (6 cases, two explicit endedAt values standing in for "turn ended" and "app
    closed"): open entry is stamped; a second stamp does not move the first; an
    entry with finished: true and no endedAt gains none; an explicit turnId
    is honoured past a trailing image entry; permissionWaitMs is recorded; a
    trailing non-assistant entry is never stamped.
  • Both suites fail without the guard (3 failures) and pass with it (8 passed).
  • corepack pnpm format — clean; all three new files pass prettier --check.
  • corepack pnpm check does not complete on this machine: it aborts in its
    first step (typecheck -> lody prepare:acp-adapters) on a pre-existing
    acp-extension-dsh build error, src/adapter.ts(1649,7) TS2352 on
    ReadableStream generics. That submodule is untouched here
    (git diff --submodule and git status are clean for it) and the local
    toolchain is Node v24.16.0, so this may be a local @types/node lib
    mismatch rather than something CI sees.
  • Every other check stage was run individually: recursive typecheck with
    acp-extension-dsh excluded (covers lody, all packages, and site-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.
  • Rebased onto current main and re-verified there: applies with no conflict,
    apps/cli typechecks clean, both new suites pass (8 cases).
  • corepack pnpm test:ci on the pre-rebase base: apps/cli 248 files / 2474
    tests and every package pass. That count predates both new suites; a full
    apps/cli run on the rebased branch reports 250 files / 2489 tests, the delta
    being the two new suites plus suites main gained during the rebase window.
    One apps/electron file, src/main/services/loro-data-plane-relay.test.mjs,
    fails on Error: Electron failed to install correctly — the Electron binary
    is missing from this machine's install, not a code failure, and nothing here
    touches apps/electron.
  • That test:ci run predates both new suites, so they were also run together
    with the neighbouring message-handler-* suites (5 files, 29 tests, all pass).
    apps/cli typecheck was re-run afterwards; note its tsconfig excludes test
    files, so that step covers assistant-turn-finalize.ts but not either new
    test file — those are typechecked only by vitest.
  • Not done: no end-to-end app-close run. The teardown path itself
    (exit/terminated -> finalizeACPState) is unchanged; only the history
    write it performs is guarded.

Context handoff

Instructions for reviewing agents

  • Review focus: apps/cli/src/lib/assistant-turn-finalize.ts (one guard, extracted verbatim otherwise) and the finalizeACPState call site in message-handler.ts.
  • Decisions to challenge: guarding on finished === true instead of a missing endedAt, and extracting the loop into its own module rather than leaving it inline.
  • Plausible failures / evidence gaps: An adjacent bug is untouched: image/file entries are pushed after the turn entry, so a no-turnId finalize on a session ending with one matches that entry and breaks, and the real turn entry is never finalized at all — previously it got the wrong entry stamped, now neither is. Fixing it means scanning past finished entries, which would stamp now on 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 later permissionWaitMs is dropped.

Authoring context

  • User goal / directives: Fix issue [Bug] Completed turn duration grows after the app is quit and reopened #260 ("Worked for" duration grows while the app stays open) at its root cause, with a regression test and no unrelated changes.
  • Constraints / non-goals: No behaviour change for interrupted turns or resume; no touching the teardown call sites; no fix for the adjacent image/file-entry ordering bug; tests use no real sleeps or wall-clock races per AGENTS.md.
  • Risk-bearing decisions: The guard makes the first terminal stamp final for an assistant entry, so any future producer that pre-marks a live turn finished: true would suppress its real timing; all current finished: true writers in apps/cli/src were checked and write system/user entries or separate assistant image/file entries.
  • Destructive or irreversible behavior: None. No migration, deletion, or rewrite of stored history; the change only declines a write that previously overwrote a durable field.
  • Deliberately not done or tested: The adjacent finalize-target bug described above, and a manual app-close reproduction — finalizeACPState is driven directly against a real SessionDocument instead, which is the boundary the bug lives at.
  • Unknowns / confidence: High confidence in the root cause and the guard; the residual unknown is how often sessions end with an image/file entry, which decides how much of [Bug] Completed turn duration grows after the app is quit and reopened #260 the untouched adjacent bug still accounts for.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Completed turn duration grows after the app is quit and reopened

1 participant