Repository navigation
Conversation
baron
left a comment
There was a problem hiding this comment.
Reviewed 9a7a9b1a9f6b289cb4f8e30d783e76e0aa95848b against its merge base.
No actionable correctness defect identified in the oversized-resume fix. I checked preserved conversation identity, controller retirement, blocking repeated attempts for the same conversation/rollout pair, allowing a changed target, /compact and watchdog recovery refusal, and retained error projection through transcript compaction. The tests assert that no replacement thread or user turn is started and that the terminal snapshot carries resume_history_too_large.
The observed CI run for this exact head passed: https://github.com/repoprompt/repoprompt-ce/actions/runs/39811205618. Review here was static source/test inspection, not a local macOS run.
Integration note: #1457 changes the same resume fallback and diagnostic paths to stop on timeouts. This branch still retains the old timeout-to-fresh policy, while #1457 still retains the old oversized-to-fresh policy. Integrate them deliberately and run both sets of regressions; their independent green checks do not prove the combined behavior.
Refs #1377.
Scope and root cause
Merged #1379 (
27d13e762) already fixed recovery-notice retention. The remaining oversized-resume path on the task'sorigin/mainbase (ab84d47e6) still automatically started a fresh thread, then delivered a follow-up without the prior task. This PR stops that unsafe replacement; it is part of #1377, not the complete history-free-resume / explicit recovery-choice solution described in its maintainer comment.Changes, including adversarial-review response
session not ready; terminal settlement does not append a duplicate generic error. Failure notifications use the settled transcript error when the provider has already reported it. A real terminal-barrier regression asserts one error per user turn, exact send/notification text, and canonical failure publication.failure_reason: resume_history_too_large; run snapshots, lane-board mapping, display text, and CLI help support that reason. The blocker is session-scoped like resume-timeout accounting; changed references can resume, and a restart/restoration may make one new attempt before an overflow is remembered again.Refs #1377, not an auto-closing keyword./compact, including repeat attempts, no replacement/turn/compact dispatch, preserved identity, and successful unblocking when the rollout reference changes.latestErrorTextconsults retained summary diagnostics. Tests assert row kind, exact text, serialization round-trip, and latest-error lookup.resumeStoppedOutcomeandresumeFrameTooLargeReason, and replaced the stale notice-length comment with the actual consumer-budget constraint.The original conversation ID, rollout path, and cleanup handle remain intact. The poisoned controller is retired; the current follow-up is not sent to a replacement thread.
Remaining overlap / deliberately not implemented
thread/readwith full turns can overflow too. This PR does not implement bounded/full-history-free reads or resume reconciliation.Validation
make dev-test FILTER='CodexResumeWedgeCommitTests|AgentTranscriptRecoveryDiagnosticTests|AgentRunLifecycleContractsTests': 57 passed, 0 failures (43 + 10 + 4), ticketf6c39dc3-f9bf-4c5f-94f4-498e5032dd15. Includes a second appended user turn, barrier publication/notification assertions, and the/compact/recovery cases../conductor swift-build --product all --async --request-key issue-1377-review-products: RepoPrompt and repoprompt-mcp both passed, ticket9502bdd8-60a5-4469-9e98-098d229d9b06. Production source was unchanged by the subsequent fixture correction / second-user-turn assertion.make dev-lint: passed, ticketf92f84e6-bc42-4d76-8407-22409d62a185.Iteration evidence: the first review-response test build caught a fixture assignment to read-only
runID(ticketd77f66e8-a265-4a4d-9719-666c07b8c343); it was corrected to useAgentModeProcessRunIdentitybefore the passing run. The original implementation's safety regression failed first against the old automatic-replacement source (ticket3b8f49cb-cdf3-4ac6-9c94-b10b9731a474; its redundant automatic cold retry was canceled). The earlier PR revision passed 51 focused tests, lint, and the App product build, but its no-barrier fixture missed the failure-propagation issue now covered above.Not run: full root/provider suites, full
pr-ready, packaging/release validation, live MCP/UI smoke, live Codex requests, or any visible app launch/stop/relaunch. Regression coverage uses injected controllers, not a live provider.PR remains draft for opposing-model review; the dedicated worktree is retained.