-
Notifications
You must be signed in to change notification settings - Fork 4
fix: make agent delivery receipts trustworthy #404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,8 @@ export interface AgentDeliveryReceipt { | |
| submission_started_at?: string | null; | ||
| /** Earliest wall-clock time at which a known pre-mutation rejection may retry. */ | ||
| next_attempt_at?: string | null; | ||
| /** The receiving TUI visibly accepted this into its own queue; never replay it. */ | ||
| composer_accepted?: boolean; | ||
| } | ||
|
|
||
| /** A known pre-mutation delivery rejection that is safe to retry. */ | ||
|
|
@@ -210,7 +212,11 @@ export class RetryableDeliveryError extends Error { | |
|
|
||
| type DeliverySubmitter = ( | ||
| receipt: AgentDeliveryReceipt, | ||
| ) => Promise<{ retry_count: number; submit_verified: boolean | null }>; | ||
| ) => Promise<{ | ||
| retry_count: number; | ||
| submit_verified: boolean | null; | ||
| delivery?: "submitted" | "queued"; | ||
| }>; | ||
|
|
||
| export interface SpawnAgentParams { | ||
| repo: string; | ||
|
|
@@ -5057,7 +5063,8 @@ export class AgentEngine { | |
| }; | ||
| if ( | ||
| receipt.delivery_state === "queued" && | ||
| receipt.submission_started_at | ||
| receipt.submission_started_at && | ||
| receipt.composer_accepted !== true | ||
| ) { | ||
| receipt.delivery_state = "failed"; | ||
| receipt.terminal = true; | ||
|
|
@@ -5127,6 +5134,37 @@ export class AgentEngine { | |
| return { ...receipt }; | ||
| } | ||
|
|
||
| acceptComposerQueue(input: { | ||
| delivery_id: string; | ||
| agent_id: string; | ||
| text: string; | ||
| press_enter: boolean; | ||
| source_event: DeliveryEventType; | ||
| retry_count: number; | ||
| }): AgentDeliveryReceipt { | ||
| const acceptedAt = new Date().toISOString(); | ||
| const receipt: AgentDeliveryReceipt = { | ||
| ...input, | ||
| delivery_state: "queued", | ||
| terminal: false, | ||
| created_at: acceptedAt, | ||
| resolved_at: null, | ||
| submit_verified: null, | ||
| error: null, | ||
| submission_started_at: acceptedAt, | ||
| next_attempt_at: null, | ||
| composer_accepted: true, | ||
| }; | ||
| this.deliveryReceipts.set(receipt.delivery_id, receipt); | ||
| try { | ||
| this.persistDeliveryReceipts(); | ||
| } catch (error) { | ||
| this.deliveryReceipts.delete(receipt.delivery_id); | ||
| throw error; | ||
| } | ||
| return { ...receipt }; | ||
| } | ||
|
|
||
| resolveDelivery( | ||
| input: Omit<AgentDeliveryReceipt, "created_at" | "resolved_at"> & { | ||
| created_at?: string; | ||
|
|
@@ -5157,6 +5195,7 @@ export class AgentEngine { | |
| try { | ||
| for (const receipt of this.deliveryReceipts.values()) { | ||
| if (receipt.delivery_state !== "queued") continue; | ||
| if (receipt.composer_accepted === true) continue; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an agent is removed after its TUI visibly accepted a message into its internal queue, this early AGENTS.md reference: AGENTS.md:L11-L16 Useful? React with 👍 / 👎. |
||
| const agent = this.getAgentState(receipt.agent_id); | ||
| if (!agent) { | ||
| receipt.delivery_state = "failed"; | ||
|
|
@@ -5195,13 +5234,21 @@ export class AgentEngine { | |
| ]).finally(() => { | ||
| if (timeout) clearTimeout(timeout); | ||
| }); | ||
| receipt.delivery_state = "submitted"; | ||
| receipt.terminal = true; | ||
| receipt.resolved_at = new Date().toISOString(); | ||
| receipt.retry_count += result.retry_count; | ||
| receipt.submit_verified = result.submit_verified; | ||
| receipt.error = null; | ||
| receipt.next_attempt_at = null; | ||
| if (result.delivery === "queued") { | ||
| receipt.delivery_state = "queued"; | ||
| receipt.terminal = false; | ||
| receipt.resolved_at = null; | ||
| receipt.submit_verified = null; | ||
| receipt.composer_accepted = true; | ||
| } else { | ||
| receipt.delivery_state = "submitted"; | ||
| receipt.terminal = true; | ||
| receipt.resolved_at = new Date().toISOString(); | ||
| receipt.submit_verified = result.submit_verified; | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof RetryableDeliveryError) { | ||
| receipt.submission_started_at = null; | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium Line 303 in bb4ce3f
- else if (info.submit_verified === null)
+ else if (info.submit_verified === null && !info.submit_attempted)🚀 Reply "fix it for me" or copy this AI Prompt for your agent: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -266,6 +266,8 @@ export function formatDelivery( | |
| // instead of the delivered/failed binary, so the line never contradicts a | ||
| // pending status. | ||
| pending?: boolean; | ||
| typed?: boolean; | ||
| submit_attempted?: boolean; | ||
| submit_verified?: boolean | null; | ||
| }, | ||
| ): string { | ||
|
|
@@ -280,8 +282,15 @@ export function formatDelivery( | |
| ); | ||
| const parens = meta.length > 0 ? ` (${meta.join(" \u00b7 ")})` : ""; | ||
| let head: string; | ||
| if (info.pending) { | ||
| if (info.typed) { | ||
| head = `typed into ${label}${parens} (not submitted)`; | ||
| } else if (info.pending) { | ||
| head = `delivering to ${label}${parens}`; | ||
| } else if ( | ||
| info.submit_attempted && | ||
| info.submit_verified === null | ||
| ) { | ||
| head = `submission attempted to ${label}${parens} (not verified)`; | ||
|
Comment on lines
+285
to
+293
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Keep null verification wording consistent. When Proposed fix- else if (info.submit_verified === null)
- submit = " · submit_verified=null (not attempted)";
+ else if (info.submit_verified === null)
+ submit = info.submit_attempted
+ ? " · submit_verified=null (not verified)"
+ : " · submit_verified=null (not attempted)";🤖 Prompt for AI Agents
Comment on lines
+290
to
+293
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When AGENTS.md reference: AGENTS.md:L41-L42 Useful? React with 👍 / 👎. |
||
| } else if (info.delivered) { | ||
| head = `delivered to ${label}${parens}`; | ||
| } else { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High
src/agent-engine.ts:5162acceptComposerQueuedeletes the receipt and throws whenpersistDeliveryReceipts()fails, even though the TUI has already accepted the input; this removes the no-replay marker and lets a caller or restart submit the same delivery again. The same issue occurs whendrainDeliveryQueuepersists queue acceptance: an in-memorycomposer_acceptedreceipt is skipped forever, while a restart reloads the stale receipt and marks the accepted delivery as uncertain. Preserve and report the accepted-but-not-durable outcome, and retry or otherwise durably record the marker before treating persistence as complete.🚀 Reply "fix it for me" or copy this AI Prompt for your agent: