Repository navigation
LOU-C: Human-in-the-loop & durable execution - #4
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ault Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resumeAfterApproval() rebuilds messages from an ExecutionSnapshot, which already includes the original system message, then passes them as `input` to AgentExecutor.execute() without threading through a checkpoint/sessionId. Because no checkpoint is found, execute() fell into its "build from scratch" fallback in buildMessages(), which unconditionally prepends a fresh system message from agent.prompt - producing two system messages for any agent with agent.prompt set. Add ExecuteOptions.skipSystemPromptInjection, respected by buildMessages() on the fallback path only (the checkpoint-rehydration path already never re-injects a system message). resume.ts now always passes skipSystemPromptInjection: true, since the messages it builds from the snapshot are already complete. Add a regression test in resume.test.ts that pauses an agent with agent.prompt set on a needsApproval tool, resumes it, and asserts the resumed message array contains exactly one system message matching the original content. Confirmed the test fails on the pre-fix code (2 system messages) and passes after the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AgentExecutor.execute() never called checkpointStore.delete(), so a later execute() call reusing the same sessionId would silently rehydrate from the old, already-finished checkpoint and ignore fresh input. Clear the checkpoint once the run reaches a terminal finish reason (normal stop or maxSteps exhaustion), while still leaving it in place across the mid-flight 'awaiting-approval' early return so existing checkpoint-save/resume behavior (LOU-C9/C10) is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resume.ts called the deferred tool's execute() with no try/catch,
unlike AgentExecutor.executeToolCall which wraps the identical call.
Since ApprovalGate.resolve() deletes the pending-approval record as
soon as it's read, a thrown error (e.g. a real network/payment
failure) left resumeAfterApproval's promise rejecting with no retry
path and the approval record already gone. Now a thrown error is
caught and turned into a graceful {error}-shaped tool-result message,
mirroring AgentExecutor's (error as Error).message convention and this
file's own rejection-branch message shape, so the resume still
resolves.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resume.ts captured snapshot.steps in ExecutionSnapshot but never forwarded it to the follow-up AgentExecutor.execute() call, so every resumed run silently restarted step-budget accounting at 0 and got a full fresh maxSteps allowance instead of continuing from where the paused run left off. Adds initialSteps to ExecuteOptions (same internal-option pattern already used for skipSystemPromptInjection), used only on the "build from scratch" path - a loaded checkpoint's stepIndex remains the source of truth when one exists. resume.ts now passes snapshot.steps as initialSteps and keeps it out of the public ResumeExecuteOptions surface via the existing Omit list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
load() read/checked the checkpoint file without acquiring the same lock save()/delete() use, unlike StorageServiceApprovalStore.resolve() which does lock its read. Wraps load() in the same acquireLock/finally-releaseLock pattern for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tate resumeAfterApproval() (LOU-C6) fully reconstructs messages/steps from the ApprovalGate's ExecutionSnapshot, but ResumeExecuteOptions only omitted skipSystemPromptInjection/initialSteps from pass-through options - not sessionId/checkpointStore. A checkpoint saved under the paused session (from durable-execution steps taken before the pause, LOU-C9/C10) is stale by construction, since checkpoints are only written after a tool result and a pause happens before the deferred tool runs. If a caller passed sessionId+checkpointStore through, AgentExecutor's checkpoint rehydration branch would silently override the resume's reconstructed messages/steps with that stale pre-pause state, dropping the deferred tool's just-executed result even though the tool really ran. Fix: ResumeExecuteOptions now also omits sessionId/checkpointStore, and resumeAfterApproval forces both to undefined on the follow-up execute() call at runtime (the Omit alone only guards type-checked callers, not an `as any` or plain-JS caller). Also threads a sessionId onto ExecutionSnapshot and, when a CheckpointStore is passed to resumeAfterApproval, proactively deletes any stale checkpoint for that session so a later, unrelated execute() call against the same sessionId+checkpointStore can't rehydrate it either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Implements all 10 tickets from LOU-C (Phase 1), adding an approval-gate primitive and durable checkpoint/resume to
AgentExecutor:needsApproval?: boolean | ((args) => boolean | Promise<boolean>)added toToolDescriptor.PendingApproval/ApprovalDecision/ExecutionSnapshottypes +ApprovalStoreinterface with aStorageService-backed default (src/execution/ApprovalGate.ts), reusingStorageService's existing lock pattern.AgentExecutorevaluatesneedsApprovalbefore invoking a tool; when true, persists a snapshot and resolves withfinishReason: 'awaiting-approval'instead of throwing.resumeAfterApproval()(src/execution/resume.ts) reconstructs message history from a saved snapshot, runs the deferred tool (or pushes a rejection message) and re-entersAgentExecutor.execute().Checkpoint/CheckpointStoretypes, kept backend-agnostic, with aLocalStorageCheckpointStoredefault implementation.AgentExecutorsaves a checkpoint after each tool result (fully opt-in viasessionId/checkpointStore) and rehydrates from an existing checkpoint on entry, resuming the loop (and step counter) from where it left off.Codebase adaptations (vs. literal ticket snippets)
AgentExecutoris fully static (AgentExecutor.execute(options), not instance-based), so approval/checkpoint dependencies were threaded through as optionalExecuteOptionsfields rather than constructor deps.resumeAfterApprovaltakes anLLMProvider, not an executor instance.ToolDescriptorwraps a Vercel AI SDKtool, so the approval check gatestoolDesc.tool.execute. IDs use the codebase's existingnanoid()convention rather thancrypto.randomUUID().Validation results
A fresh QA agent independently re-verified every ticket from a clean state, including writing its own scratch tests beyond what the implementer wrote (concurrent-save corruption check for LOU-C3, and a stronger deep-equal + step-counter assertion for LOU-C10's crash/resume scenario — both passed).
QA caught one genuine bug not covered by any ticket's own test:
resumeAfterApproval()was duplicating the system prompt for any agent withagent.promptset, because the resume path fell intoAgentExecutor's "build messages from scratch" fallback instead of treating the snapshot's messages as complete. Fixed with a newskipSystemPromptInjectionoption, and a regression test that reproduces the exact failure (2 system messages) and confirms the fix (1 system message, matching content).npx vitest run: 476 passing. 3 known pre-existing provider-suite failures (missing optional peer deps) — unrelated to this epic.npx tsc --noEmit: clean.Minor note
LOU-C7's AC ("zero storage-backend imports above/around the interface declarations") is technically not met —
checkpoint.tsimportsStorageServiceat the top of the file because the concreteLocalStorageCheckpointStoreimplementation lives alongside the interface in the same file (same pattern asApprovalGate.ts). The interfaces themselves reference no storage types and stay backend-agnostic; this is a spec-literalism gap, not a functional issue — not worth splitting into a separate file for this epic.🤖 Generated with Claude Code