feat: keep replay session active on request - #1554
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Review verdict: not merge-ready at [P1] Missing required provider-backed integration coverage. The new Please add a provider scenario through the real daemon router/session store— The functional implementation otherwise looks coherent: the public projections are complete, only a terminal close is suppressed, interior/close-less behavior is covered, and test/Maestro lifecycle ownership is preserved. No live-device evidence is needed beyond the missing provider-route proof because no platform backend behavior changes. |
|
Addressed in Added provider-backed coverage through the real SDK → daemon route for a native Validation passed locally: |
|
Addressed the follow-up review in
Counterfactual proof: with the old raw-final-index logic and no postcondition check restored locally, both new regressions fail — the trailing-marker case dispatches Validation passed: full |
|
Re-review verdict: code-clean at The real SDK → daemon scenario proves The only red check is an unrelated Android emulator capture stall ( |
|
Thermo-nuclear review verdict: changes requested for structure. [P1] The new replay-terminal lifecycle policy must be extracted before merge. This PR adds the keep-session/repair terminal-close decision, the postcondition, and the dispatched-action count to session-replay-runtime.ts, taking an already oversized 864-line implementation to 901 lines. Its matching test file grows from 566 to 719 lines. That violates this repository’s explicit rule to extract before adding behavior beyond 500 lines, and leaves lifecycle policy entangled with parsing, execution, diagnostics, repair, and response formatting. There is a direct code-judo move here: introduce a focused replay-terminal-lifecycle module that owns terminal executable-action resolution and its outcome (suppressed close index, live-session requirement, and executed-action count). Keep session-replay-runtime.ts as orchestration only, and mirror the new unit tests in a session-replay-terminal-lifecycle.test.ts file. This removes the new mode-specific plumbing from the busy runtime and gives the keep-session + repair seam one canonical home instead of extending the existing god module. I found no second high-conviction structural issue in the public projections, raw test guard, or provider-route coverage. The prior functional findings remain resolved; this is the remaining merge blocker. |
…#1554 review) session-replay-runtime.ts was already over the 500-line extract-before-adding-behavior tripwire before this PR; the keep-session/repair terminal-close decision, its live-session postcondition, and the dispatched-action count pushed it further past budget. Move that policy into a focused session-replay-terminal-lifecycle.ts (isExecutableReplayAction, resolveSuppressedTerminalCloseIndex, countExecutedReplayActions, requireLiveSessionForKeepSession) so the runtime file stays orchestration-only, and mirror its PR-added unit tests into session-replay-terminal-lifecycle.test.ts. Pure extraction: no assertions changed.
|
Extracted the keep-session/repair terminal-close policy into What moved (verbatim, function extraction only):
LOC
For reference, Gates (all green, foreground, on 01f284a):
Generated by Claude Code |
… into the ad-replay engine Rebasing p5/extract-ad-replay onto main pulled in #1554's --keep-session feature, which had grown its own daemon-side terminal-close-suppression predicate (session-replay-terminal-lifecycle.ts's resolveSuppressedTerminalCloseIndex/countExecutedReplayActions) independently of this branch's own engine-side one (step-loop.ts's isRepairArmedTerminalCloseAction). Both are the same decision family — replay --keep-session and an active --save-script repair now share ONE structural resolution (resolveSuppressedTerminalCloseIndex, generalized to "terminal among EXECUTABLE actions" rather than the old physical-last-index check) and one suppression check inside runAdReplay, gated on keepSession OR runtime.isRepairArmed(). AdReplayRunRequest grew a keepSession field; the neutral 'replayed' count in AdReplayRunOutcome is now computed inline in the loop instead of the daemon's old actions.length - entryIndex approximation. requireLiveSessionForKeepSession (the --keep-session live-session postcondition) stays daemon-side, inlined into session-replay-runtime.ts, since it inspects SessionStore state the engine never sees. The daemon-only session-replay-terminal-lifecycle.ts this arrived with is deleted entirely — its isExecutableReplayAction was a duplicate of the engine's own. runReplayScriptFile's Maestro-format routing (including the new --keep-session Maestro rejection) was extracted into routeMaestroReplay to keep the function under fallow's complexity threshold after re-threading keepSession through it. Added packages/ad-replay/src/internal/__tests__/step-loop.test.ts covering the unified suppression decision (both keepSession and repair-armed) directly against runAdReplay, including the terminal-among-executable-actions case with a trailing nested replay marker. The daemon-level integration tests (6 tests in session-replay-terminal-lifecycle.test.ts, exercising the same behavior through runReplayScriptFile) and the SDK provider-scenario test (active-session-script-publication.test.ts) needed no changes and pass unmodified.
… into the ad-replay engine Rebasing p5/extract-ad-replay onto main pulled in #1554's --keep-session feature, which had grown its own daemon-side terminal-close-suppression predicate (session-replay-terminal-lifecycle.ts's resolveSuppressedTerminalCloseIndex/countExecutedReplayActions) independently of this branch's own engine-side one (step-loop.ts's isRepairArmedTerminalCloseAction). Both are the same decision family — replay --keep-session and an active --save-script repair now share ONE structural resolution (resolveSuppressedTerminalCloseIndex, generalized to "terminal among EXECUTABLE actions" rather than the old physical-last-index check) and one suppression check inside runAdReplay, gated on keepSession OR runtime.isRepairArmed(). AdReplayRunRequest grew a keepSession field; the neutral 'replayed' count in AdReplayRunOutcome is now computed inline in the loop instead of the daemon's old actions.length - entryIndex approximation. requireLiveSessionForKeepSession (the --keep-session live-session postcondition) stays daemon-side, inlined into session-replay-runtime.ts, since it inspects SessionStore state the engine never sees. The daemon-only session-replay-terminal-lifecycle.ts this arrived with is deleted entirely — its isExecutableReplayAction was a duplicate of the engine's own. runReplayScriptFile's Maestro-format routing (including the new --keep-session Maestro rejection) was extracted into routeMaestroReplay to keep the function under fallow's complexity threshold after re-threading keepSession through it. Added packages/ad-replay/src/internal/__tests__/step-loop.test.ts covering the unified suppression decision (both keepSession and repair-armed) directly against runAdReplay, including the terminal-among-executable-actions case with a trailing nested replay marker. The daemon-level integration tests (6 tests in session-replay-terminal-lifecycle.test.ts, exercising the same behavior through runReplayScriptFile) and the SDK provider-scenario test (active-session-script-publication.test.ts) needed no changes and pass unmodified.
Summary
Add
replay --keep-sessionand SDKkeepSessionsupport for native.adreplay.closeso an agent can continue interactively.testand Maestro YAML reject the option because those runtimes own cleanup/lifecycle.main; command help and user docs carry the planning guidance.Validation
pnpm check(5,105 unit tests and 35 smoke tests passed; 3 live lanes skipped by environment selectors)