feat: expose durable delivery inbox and ack/fail/defer API#155
Conversation
Add the public engine/API/SDK surface for server-backed durable delivery on top of the existing per-recipient `deliveries` model (issue #154). Engine: - GET /v1/deliveries lists an agent's queued items (accepted + deferred by default, oldest first) with the message payload so offline consumers can replay on reconnect. - POST /v1/deliveries/:id/ack|fail|defer idempotently transition a delivery to delivered / failed (error + retryable) / deferred (available_at + reason), emitting delivery.delivered / delivery.failed / delivery.deferred to the agent. Types: - New Delivery / DeliveryItem / request schemas. - Typed delivery.accepted, delivery.delivered, delivery.deferred, and delivery.failed server events wired into the event unions. SDK: - agent.deliveries() / ackDelivery() / failDelivery() / deferDelivery() and delivery.* event handlers so downstream SDKs can report durable delivery support honestly. Docs: README + openapi.yaml document the new endpoints and schemas. Also align the engine's @relaycast/types dependency to the workspace version so it links the local package instead of a stale published copy.
|
Warning Review limit reached
More reviews will be available in 53 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a complete durable delivery API and SDK surface for Relaycast, exposing the existing internal ChangesDurable Delivery API & SDK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a durable delivery API contract to Relaycast, allowing per-recipient delivery tracking with endpoints to list, acknowledge, fail, and defer deliveries. It includes updates to the OpenAPI specification, TypeScript SDK, database schemas, and comprehensive conformance tests. The review feedback highlights an issue where channel_id is returned empty in several API responses, suggesting a join in getOwnedDelivery to populate it. Additionally, the feedback recommends optimizing database performance by utilizing Drizzle's .returning() clause on updates to eliminate the redundant reloadDelivery helper and reduce database round-trips.
| function serializeDelivery(row: DeliveryRow) { | ||
| return { | ||
| id: row.id, | ||
| message_id: row.messageId, | ||
| channel_id: '', // filled in by callers that have the message row | ||
| agent_id: row.agentId, | ||
| status: row.status as DeliveryStatus, | ||
| mode: row.mode, | ||
| reason: row.reason, | ||
| priority: row.priority, | ||
| retryable: row.retryable ?? null, | ||
| error: row.error, | ||
| available_at: toIso(row.availableAt), | ||
| deadline: toIso(row.deadline), | ||
| created_at: toIso(row.createdAt) ?? new Date(0).toISOString(), | ||
| updated_at: toIso(row.updatedAt), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Empty channel_id in API Responses
The serializeDelivery function currently hardcodes channel_id: ''. While listDeliveries overrides this by joining with the messages table, the ackDelivery, failDelivery, and deferDelivery functions return the serialized delivery directly without overriding it. This results in empty channel_id values in their respective API responses, violating the Delivery schema which requires a non-empty channel_id string.
We can update serializeDelivery to accept an optional channelId property on the input row to preserve this value.
| function serializeDelivery(row: DeliveryRow) { | |
| return { | |
| id: row.id, | |
| message_id: row.messageId, | |
| channel_id: '', // filled in by callers that have the message row | |
| agent_id: row.agentId, | |
| status: row.status as DeliveryStatus, | |
| mode: row.mode, | |
| reason: row.reason, | |
| priority: row.priority, | |
| retryable: row.retryable ?? null, | |
| error: row.error, | |
| available_at: toIso(row.availableAt), | |
| deadline: toIso(row.deadline), | |
| created_at: toIso(row.createdAt) ?? new Date(0).toISOString(), | |
| updated_at: toIso(row.updatedAt), | |
| }; | |
| } | |
| function serializeDelivery(row: DeliveryRow & { channelId?: string }) { | |
| return { | |
| id: row.id, | |
| message_id: row.messageId, | |
| channel_id: row.channelId ?? '', | |
| agent_id: row.agentId, | |
| status: row.status as DeliveryStatus, | |
| mode: row.mode, | |
| reason: row.reason, | |
| priority: row.priority, | |
| retryable: row.retryable ?? null, | |
| error: row.error, | |
| available_at: toIso(row.availableAt), | |
| deadline: toIso(row.deadline), | |
| created_at: toIso(row.createdAt) ?? new Date(0).toISOString(), | |
| updated_at: toIso(row.updatedAt), | |
| }; | |
| } |
| /** Fetch a single delivery owned by the agent, or null. */ | ||
| async function getOwnedDelivery( | ||
| db: Db, | ||
| workspaceId: string, | ||
| agentId: string, | ||
| deliveryId: string, | ||
| ): Promise<DeliveryRow | null> { | ||
| const [row] = await db | ||
| .select() | ||
| .from(deliveries) | ||
| .where( | ||
| and( | ||
| eq(deliveries.id, deliveryId), | ||
| eq(deliveries.workspaceId, workspaceId), | ||
| eq(deliveries.agentId, agentId), | ||
| ), | ||
| ); | ||
| return row ?? null; | ||
| } |
There was a problem hiding this comment.
Fetch channelId via Join
To populate the channel_id field in the response of ack, fail, and defer operations, we can update getOwnedDelivery to perform an innerJoin with the messages table to retrieve the channelId directly.
/** Fetch a single delivery owned by the agent, or null. */
async function getOwnedDelivery(
db: Db,
workspaceId: string,
agentId: string,
deliveryId: string,
): Promise<(DeliveryRow & { channelId: string }) | null> {
const [row] = await db
.select({
delivery: deliveries,
channelId: messages.channelId,
})
.from(deliveries)
.innerJoin(messages, eq(deliveries.messageId, messages.id))
.where(
and(
eq(deliveries.id, deliveryId),
eq(deliveries.workspaceId, workspaceId),
eq(deliveries.agentId, agentId),
),
);
if (!row) return null;
return {
...row.delivery,
channelId: row.channelId,
};
}| async function reloadDelivery(db: Db, deliveryId: string): Promise<DeliveryRow> { | ||
| const [row] = await db.select().from(deliveries).where(eq(deliveries.id, deliveryId)); | ||
| return row; | ||
| } |
There was a problem hiding this comment.
| export async function ackDelivery( | ||
| db: Db, | ||
| workspaceId: string, | ||
| agentId: string, | ||
| deliveryId: string, | ||
| ) { | ||
| const existing = await getOwnedDelivery(db, workspaceId, agentId, deliveryId); | ||
| if (!existing) return null; | ||
|
|
||
| if (existing.status !== 'delivered') { | ||
| await db | ||
| .update(deliveries) | ||
| .set({ status: 'delivered', updatedAt: new Date() }) | ||
| .where(eq(deliveries.id, deliveryId)); | ||
| } | ||
| return serializeDelivery(await reloadDelivery(db, deliveryId)); | ||
| } |
There was a problem hiding this comment.
Optimize ackDelivery with .returning() and Cache Reuse
We can optimize ackDelivery by:
- Reusing the
existingrow if the status is already'delivered', avoiding any database updates or re-queries. - Using
.returning()on the update query to get the updated row directly, avoiding the extrareloadDeliveryquery.
export async function ackDelivery(
db: Db,
workspaceId: string,
agentId: string,
deliveryId: string,
) {
const existing = await getOwnedDelivery(db, workspaceId, agentId, deliveryId);
if (!existing) return null;
if (existing.status !== 'delivered') {
const [updated] = await db
.update(deliveries)
.set({ status: 'delivered', updatedAt: new Date() })
.where(eq(deliveries.id, deliveryId))
.returning();
return serializeDelivery({ ...updated, channelId: existing.channelId });
}
return serializeDelivery(existing);
}There was a problem hiding this comment.
Valid — fixed in 2f423e5 (identified by cubic). ackDelivery now routes through the same resolveTransition helper as fail/defer, so when UPDATE ... RETURNING yields no row (the delivery was deleted between the ownership read and the update) it re-reads and returns the current state, or null if the row is gone, instead of a malformed object built from undefined.
Generated by Claude Code
| export async function failDelivery( | ||
| db: Db, | ||
| workspaceId: string, | ||
| agentId: string, | ||
| deliveryId: string, | ||
| opts: { error?: string; retryable?: boolean } = {}, | ||
| ) { | ||
| const existing = await getOwnedDelivery(db, workspaceId, agentId, deliveryId); | ||
| if (!existing) return null; | ||
|
|
||
| await db | ||
| .update(deliveries) | ||
| .set({ | ||
| status: 'failed', | ||
| error: opts.error ?? null, | ||
| retryable: opts.retryable ?? null, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(deliveries.id, deliveryId)); | ||
| return serializeDelivery(await reloadDelivery(db, deliveryId)); | ||
| } |
There was a problem hiding this comment.
Optimize failDelivery with .returning()
We can use .returning() on the update query to get the updated row directly, avoiding the extra reloadDelivery query.
export async function failDelivery(
db: Db,
workspaceId: string,
agentId: string,
deliveryId: string,
opts: { error?: string; retryable?: boolean } = {},
) {
const existing = await getOwnedDelivery(db, workspaceId, agentId, deliveryId);
if (!existing) return null;
const [updated] = await db
.update(deliveries)
.set({
status: 'failed',
error: opts.error ?? null,
retryable: opts.retryable ?? null,
updatedAt: new Date(),
})
.where(eq(deliveries.id, deliveryId))
.returning();
return serializeDelivery({ ...updated, channelId: existing.channelId });
}There was a problem hiding this comment.
All three findings addressed in 288d9e4 (identified by cubic):
- failDelivery idempotency (P1):
failedis now treated as settled — once a delivery has failed, repeated calls are no-ops that preserve the originalerror/retryableand don't churnupdatedAt. - Duplicate
delivery.*events (P2): transitions now report whether they actually changed state, and the routes fan out only on a real change, so idempotent retries (re-ack, re-fail, re-defer to the same time) emit no duplicate notifications. - Status guard on fail/defer (P1): already in place from an earlier commit — both short-circuit when already
deliveredand addstatus != 'delivered'to the UPDATE WHERE to close the read→write race against a concurrent ack.
Added conformance tests for single-emit ack, fail-metadata preservation, and (separately) read-clears-deferred. Engine build/test/lint green.
Generated by Claude Code
| export async function deferDelivery( | ||
| db: Db, | ||
| workspaceId: string, | ||
| agentId: string, | ||
| deliveryId: string, | ||
| opts: { availableAt: Date; reason?: string }, | ||
| ) { | ||
| const existing = await getOwnedDelivery(db, workspaceId, agentId, deliveryId); | ||
| if (!existing) return null; | ||
|
|
||
| await db | ||
| .update(deliveries) | ||
| .set({ | ||
| status: 'deferred', | ||
| availableAt: opts.availableAt, | ||
| reason: opts.reason ?? existing.reason, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(deliveries.id, deliveryId)); | ||
| return serializeDelivery(await reloadDelivery(db, deliveryId)); | ||
| } |
There was a problem hiding this comment.
Optimize deferDelivery with .returning()
We can use .returning() on the update query to get the updated row directly, avoiding the extra reloadDelivery query.
export async function deferDelivery(
db: Db,
workspaceId: string,
agentId: string,
deliveryId: string,
opts: { availableAt: Date; reason?: string },
) {
const existing = await getOwnedDelivery(db, workspaceId, agentId, deliveryId);
if (!existing) return null;
const [updated] = await db
.update(deliveries)
.set({
status: 'deferred',
availableAt: opts.availableAt,
reason: opts.reason ?? existing.reason,
updatedAt: new Date(),
})
.where(eq(deliveries.id, deliveryId))
.returning();
return serializeDelivery({ ...updated, channelId: existing.channelId });
}There was a problem hiding this comment.
All three concurrency findings fixed in 5fbe6d0 (identified by cubic). Root cause was the same for all: changed was derived from the application-level pre-read, so two concurrent calls could both observe the same pre-state. I moved the transition precondition into each UPDATE's WHERE clause and now derive changed purely from whether RETURNING matched a row — SQLite serializes writes, so the DB decides atomically and only one concurrent call wins:
- ack —
WHERE status != 'delivered'; the loser matches no row →changed: false, sodelivery.deliveredfires once. - fail —
WHERE status NOT IN ('delivered','failed');failedis settled, so concurrent fails can't overwrite the first failure's metadata or re-emit. - defer —
WHERE NOT (already deferred to the same available_at + reason); identical concurrent defers collapse to a single change/event, while a defer to a new time still counts.
Added a test asserting a re-defer to the same available_at emits no second delivery.deferred. Engine build/test/lint green (27 tests).
Generated by Claude Code
…urning() Address review feedback on the durable delivery engine: - getOwnedDelivery now joins messages so ack/fail/defer responses return the delivery's channel_id instead of an empty string (matching the Delivery schema). - Use Drizzle .returning() on the update queries to read back the transitioned row in a single round-trip, removing the reloadDelivery helper. - ackDelivery short-circuits when already delivered. - Tests assert channel_id is populated on ack/fail/defer responses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0ef93119c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await db | ||
| .update(deliveries) | ||
| .set({ | ||
| status: 'deferred', |
There was a problem hiding this comment.
Preserve terminal delivery status in updates
When two consumers race on the same delivery, a stale defer request can overwrite an already-acked record back to deferred because the update only filters by id after reading ownership. This contradicts the lifecycle that marks delivered as terminal (packages/types/src/delivery.ts:3-6) and makes the delivery reappear in the default queue even though another worker successfully acknowledged it; the same stale-update pattern exists in failDelivery, so these transitions should guard the current status in the WHERE clause (or no-op for terminal rows).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 9466059: delivered is now terminal for both fail and defer — they short-circuit when the row is already delivered, and the update WHERE clause adds status != 'delivered' to close the read→write race against a concurrent ack (re-reading current state if the guard rejects the write). Added a conformance test asserting a late defer/fail on a delivered delivery keeps it delivered and out of the default queue.
Generated by Claude Code
A late fail/defer could overwrite an already-acked (delivered) delivery, resurrecting it into the default queue. Guard the transitions so delivered is terminal: short-circuit when already delivered, and add `status != 'delivered'` to the update WHERE clause to close the read->write race against a concurrent ack (re-reading current state when the guard rejects the write).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
312-324: ⚡ Quick winConsider adding a realtime-first code example.
The documentation accurately describes the durable delivery API surface, but developers would benefit from seeing how to use the delivery events in a realtime-first pattern. For example:
// Listen for delivery state changes over WebSocket alice.on.deliveryAccepted((event) => { console.log(`Delivery ${event.deliveryId} queued for message ${event.messageId}`); }); alice.on.deliveryDelivered((event) => { console.log(`Delivery ${event.deliveryId} acknowledged`); }); // On reconnect, replay queued deliveries const pending = await alice.deliveries({ status: 'accepted' }); for (const item of pending) { await processDelivery(item); await alice.ackDelivery(item.id); }This would demonstrate the intended usage pattern and help users avoid polling anti-patterns. As per coding guidelines, quickstart examples should be realtime-first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 312 - 324, Add a realtime-first quickstart snippet to the durable delivery docs showing how to subscribe to delivery events and replay on reconnect: introduce example usage of the client event handlers (e.g., alice.on.deliveryAccepted and alice.on.deliveryDelivered), demonstrate fetching pending deliveries via alice.deliveries({ status: 'accepted' }) and processing them with a helper like processDelivery, then acknowledge using alice.ackDelivery(item.id); place this example near the GET /deliveries description and ensure it highlights realtime event handling + reconnect replay instead of polling.packages/engine/src/routes/delivery.ts (1)
14-17: 💤 Low valueLocal
listQuerySchemaduplicates the exportedListDeliveriesQuerySchema.
@relaycast/typesalready exportsListDeliveriesQuerySchema, but it usesz.number()(no coercion) so it can't validate raw query strings directly. The local schema is a reasonable workaround, but consider deriving from the shared schema to avoid drift — e.g. exporting a coercing variant from types, or having the shared schema usez.coerce.number()so both layers agree on bounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/routes/delivery.ts` around lines 14 - 17, The local listQuerySchema duplicates the exported ListDeliveriesQuerySchema; instead derive from the shared schema and only override the limit to a coercing number to avoid drift. Import ListDeliveriesQuerySchema from `@relaycast/types` and create listQuerySchema by spreading/using its shape but replace the limit with z.coerce.number().int().positive().max(200).optional() (e.g. listQuerySchema = z.object({ ...ListDeliveriesQuerySchema.shape, limit: z.coerce.number().int().positive().max(200).optional() })), or alternatively, update the shared ListDeliveriesQuerySchema to export a coercing variant and use that here so both layers stay consistent.packages/types/src/delivery.ts (1)
60-63: 💤 Low valueUpdate Zod 4 datetime schema to
z.iso.datetime()
z.string().datetime()is deprecated in Zod 4; preferavailable_at: z.iso.datetime()(works with the same optional config patterns like offsets/locales where needed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/types/src/delivery.ts` around lines 60 - 63, The DeferDeliveryRequestSchema uses the deprecated z.string().datetime(); update the available_at field to use Zod 4's ISO datetime helper by replacing z.string().datetime() with z.iso.datetime() (preserving any existing config options if you need offsets/locales), i.e., modify the DeferDeliveryRequestSchema definition so available_at: z.iso.datetime().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/engine/delivery.ts`:
- Around line 139-198: Add the same terminal-state guard used in ackDelivery to
failDelivery and deferDelivery: after retrieving existing via getOwnedDelivery,
return null if not found and if existing.status === 'delivered' skip the update
and just return serializeDelivery(await reloadDelivery(db, deliveryId));
otherwise perform the update as currently written; this prevents reversing a
delivered terminal state and avoids emitting conflicting lifecycle changes in
failDelivery and deferDelivery.
---
Nitpick comments:
In `@packages/engine/src/routes/delivery.ts`:
- Around line 14-17: The local listQuerySchema duplicates the exported
ListDeliveriesQuerySchema; instead derive from the shared schema and only
override the limit to a coercing number to avoid drift. Import
ListDeliveriesQuerySchema from `@relaycast/types` and create listQuerySchema by
spreading/using its shape but replace the limit with
z.coerce.number().int().positive().max(200).optional() (e.g. listQuerySchema =
z.object({ ...ListDeliveriesQuerySchema.shape, limit:
z.coerce.number().int().positive().max(200).optional() })), or alternatively,
update the shared ListDeliveriesQuerySchema to export a coercing variant and use
that here so both layers stay consistent.
In `@packages/types/src/delivery.ts`:
- Around line 60-63: The DeferDeliveryRequestSchema uses the deprecated
z.string().datetime(); update the available_at field to use Zod 4's ISO datetime
helper by replacing z.string().datetime() with z.iso.datetime() (preserving any
existing config options if you need offsets/locales), i.e., modify the
DeferDeliveryRequestSchema definition so available_at: z.iso.datetime().
In `@README.md`:
- Around line 312-324: Add a realtime-first quickstart snippet to the durable
delivery docs showing how to subscribe to delivery events and replay on
reconnect: introduce example usage of the client event handlers (e.g.,
alice.on.deliveryAccepted and alice.on.deliveryDelivered), demonstrate fetching
pending deliveries via alice.deliveries({ status: 'accepted' }) and processing
them with a helper like processDelivery, then acknowledge using
alice.ackDelivery(item.id); place this example near the GET /deliveries
description and ensure it highlights realtime event handling + reconnect replay
instead of polling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58523027-1ead-415e-a35a-f79f1b1a9604
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
README.mdopenapi.yamlpackages/engine/package.jsonpackages/engine/src/__tests__/conformance/delivery.test.tspackages/engine/src/engine.tspackages/engine/src/engine/delivery.tspackages/engine/src/routes/delivery.tspackages/sdk-typescript/src/agent.tspackages/sdk-typescript/src/types.tspackages/types/src/__tests__/types.test.tspackages/types/src/delivery.tspackages/types/src/events.tspackages/types/src/index.ts
- Reuse the shared ListDeliveriesQuerySchema in the engine route (now coercing on limit) instead of a local duplicate, avoiding schema drift. - Modernize DeferDeliveryRequestSchema to Zod 4's z.iso.datetime(). - Add a realtime-first SDK example (live delivery events + reconnect replay) to the durable delivery README section. The terminal-state guard for fail/defer flagged by reviewers was already added in an earlier commit.
|
Reviewed and fixed PR #155 locally. Fixed:
Verified locally:
|
There was a problem hiding this comment.
128) — fixes were not applied to the PR. The notes below are advisory and were not pushed.
Reviewed and fixed PR #155 locally.
Fixed:
- Delivery transition responses (
ack,fail,defer) now return the realchannel_idinstead of"". - Added engine conformance assertions for
channel_idon queued and acked deliveries. - Added SDK tests for
deliveries,ackDelivery,failDelivery, anddeferDelivery, including URL encoding andavailableAt->available_at.
Verified locally:
npm run build --workspace=@relaycast/types && npm run build --workspace=@relaycast/engine && npm run build --workspace=@relaycast/sdknpx turbo test --filter=@relaycast/types --filter=@relaycast/engine --filter=@relaycast/sdknpx turbo lint --filter=@relaycast/types --filter=@relaycast/engine --filter=@relaycast/sdk
Add SDK tests for deliveries(), ackDelivery(), failDelivery(), and deferDelivery() — asserting URL construction, delivery-id URL encoding, query params, response camelization, and the availableAt -> available_at body decamelization.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
If the delivery is deleted between the ownership read and the UPDATE,
RETURNING yields no row and serializeDelivery({ ...undefined }) produced a
malformed object. Route ackDelivery through the same resolveTransition helper
as fail/defer so it re-reads and returns the current state (or null) instead.
There was a problem hiding this comment.
3 issues found across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Reviewed PR #155 and fixed lifecycle issues in the durable delivery API. Changes made:
Local checks passed:
|
There was a problem hiding this comment.
1) — fixes were not applied to the PR. The notes below are advisory and were not pushed.
Reviewed PR #155 and fixed lifecycle issues in the durable delivery API.
Changes made:
- Made
deliveredterminal:failanddefernow return delivered records unchanged instead of reopening them. - Updated read receipts so marking a deferred delivery read removes it from the replay queue.
- Added conformance coverage for both cases.
- Updated README and OpenAPI wording to match the terminal delivered contract.
- Normalized the lockfile via
npm install.
Local checks passed:
npx turbo buildnpx turbo lint --concurrency=1npx turbo test --concurrency=1- Focused delivery test/build/lint rerun after docs update
Address review findings on durable delivery idempotency: - Transitions now report whether they actually changed state; the routes fan out delivery.* events only on a real change, so idempotent retries no longer emit duplicate notifications. - failDelivery treats `failed` as settled: repeated calls are no-ops that preserve the original error/retryable and don't churn updatedAt. - deferDelivery is a no-op when re-deferred to the same available_at/reason. - markRead now clears both accepted and deferred deliveries, so a deferred delivery doesn't linger in the replay queue once the message is read. Adds conformance coverage for single-emit ack, fail-metadata preservation, and read-clears-deferred.
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…conditions
Move the "is this a real transition?" predicate into each UPDATE's WHERE clause
and derive `changed` from whether RETURNING matched a row, so the database
decides atomically instead of the application-level pre-read. SQLite serializes
writes, so concurrent calls can no longer both report changed:
- ack: WHERE status != 'delivered'
- fail: WHERE status NOT IN ('delivered','failed') (failed is settled)
- defer: WHERE not already deferred to the same (available_at, reason)
This closes the read->write races flagged in review: duplicate delivered/failed/
deferred events under concurrent calls, and failure-metadata overwrite. Adds a
test asserting a re-defer to the same time emits no second event.
|
Reviewed PR #155 against the provided diff and traced the delivery changes through engine routes, delivery row creation, SDK casing, shared types, docs, and OpenAPI. I normalized the workspace install/package lock state with Local verification passed:
|
There was a problem hiding this comment.
Reviewed PR #155 against the provided diff and traced the delivery changes through engine routes, delivery row creation, SDK casing, shared types, docs, and OpenAPI.
I normalized the workspace install/package lock state with npm install; this corrected the package-lock metadata drift while keeping the new engine dependency on @relaycast/types@2.0.0.
Local verification passed:
npm run test --workspace=@relaycast/typesnpm run test --workspace=@relaycast/engine -- src/__tests__/conformance/delivery.test.tsnpm run test --workspace=@relaycast/sdk -- src/__tests__/deliveries.test.tsnpm run test --workspace=@relaycast/sdknpx turbo testnpx turbo lint --concurrency=1npx turbo build --concurrency=1 --force
|
Reviewed and fixed PR #155 locally. Key fix: addressed cubic’s concurrency findings in packages/engine/src/engine/delivery.ts by moving ack/fail/defer idempotency guards into the database Added regression coverage in packages/engine/src/tests/conformance/delivery.test.ts to verify identical re-defers only emit one Local checks passed:
Trail recording failed because existing trajectory files in |
|
Reviewed and fixed PR #155 locally. Key fix: addressed cubic’s concurrency findings in packages/engine/src/engine/delivery.ts by moving ack/fail/defer idempotency guards into the database Added regression coverage in packages/engine/src/tests/conformance/delivery.test.ts to verify identical re-defers only emit one Local checks passed:
Trail recording failed because existing trajectory files in |
There was a problem hiding this comment.
Reviewed and fixed PR #155 locally.
Key fix: addressed cubic’s concurrency findings in packages/engine/src/engine/delivery.ts by moving ack/fail/defer idempotency guards into the database UPDATE ... WHERE predicates, so duplicate events and failure metadata overwrites are prevented under concurrent calls.
Added regression coverage in packages/engine/src/tests/conformance/delivery.test.ts to verify identical re-defers only emit one delivery.deferred event.
Local checks passed:
npx vitest run packages/engine/src/__tests__/conformance/delivery.test.tsnpm run typecheck --workspace packages/enginenpm run lint --workspace packages/enginenpx turbo buildnpx turbo lint --concurrency=1npx turbo test --concurrency=1
Trail recording failed because existing trajectory files in .trajectories/active do not validate with the installed agent-trajectories schema.
|
Reviewed PR #155 and ran the durable delivery changes through the affected engine, SDK, and shared types paths. I fixed the lockfile metadata churn left by the PR by regenerating the install state with Local verification run:
All completed successfully locally. |
There was a problem hiding this comment.
Reviewed PR #155 and ran the durable delivery changes through the affected engine, SDK, and shared types paths.
I fixed the lockfile metadata churn left by the PR by regenerating the install state with npm install; the bundled @clack/prompts dependency is back to normal lockfile metadata instead of being marked extraneous.
Local verification run:
npm --workspace @relaycast/types test -- src/__tests__/types.test.tsnpm --workspace @relaycast/types run buildnpm --workspace @relaycast/sdk test -- src/__tests__/deliveries.test.tsnpm --workspace @relaycast/sdk run buildnpm --workspace @relaycast/engine test -- src/__tests__/conformance/delivery.test.tsnpm --workspace @relaycast/engine run typechecknpm --workspace @relaycast/types run lintnpm --workspace @relaycast/sdk run lintnpm --workspace @relaycast/engine run lintnpm --workspace @relaycast/engine run buildnpx turbo test --filter=@relaycast/types --filter=@relaycast/sdk --filter=@relaycast/engine
All completed successfully locally.
|
Reviewed PR #155 and made one fix: added 0014_normalize_delivery_pending.sql to normalize historical Local checks run:
Note: the first parallel |
|
Reviewed PR #155 and made one fix: added 0014_normalize_delivery_pending.sql to normalize historical Local checks run:
Note: the first parallel |
There was a problem hiding this comment.
Reviewed PR #155 and made one fix: added 0014_normalize_delivery_pending.sql to normalize historical deliveries.status = 'pending' rows to accepted, matching the new public delivery status contract so upgraded queues remain visible.
Local checks run:
npx turbo buildpassednpx turbo testpassednpx turbo lint --concurrency=1passed- Focused engine/sdk delivery tests passed
Note: the first parallel npx turbo lint run had @relaycast/mcp killed with exit 137, then the same lint suite passed when rerun with lower concurrency.
- Add migration 0014 to promote any historical deliveries.status = 'pending' rows (the old column default from migration 0010) to 'accepted'. The public DeliveryStatus contract has no 'pending' state and GET /v1/deliveries lists accepted + deferred, so legacy rows would otherwise be invisible and outside the typed enum. Idempotent. - Restore the @clack/prompts bundled dep lockfile metadata to match main (dev, not extraneous) — incidental churn from an earlier reinstall.
|
Reviewed PR #155 against the supplied diff and changed-file list. I fixed the lockfile drift where Local verification run:
All passed locally. Trail recording was attempted, but the existing trajectory files are invalid for the installed |
There was a problem hiding this comment.
Reviewed PR #155 against the supplied diff and changed-file list. I fixed the lockfile drift where npm install corrected the nested @clack/prompts bundled dependency metadata back to dev: true.
Local verification run:
npx turbo build --filter=@relaycast/types --filter=@relaycast/sdk --filter=@relaycast/enginenpx turbo lint --filter=@relaycast/types --filter=@relaycast/sdk --filter=@relaycast/enginenpx turbo test --filter=@relaycast/types --filter=@relaycast/sdk --filter=@relaycast/engine- conflict marker scan with
rg
All passed locally. Trail recording was attempted, but the existing trajectory files are invalid for the installed trail schema, so the CLI refused to append.
|
Reviewed PR #155 and fixed one delivery lifecycle bug: failed deliveries are now terminal for late Local verification passed:
|
There was a problem hiding this comment.
Reviewed PR #155 and fixed one delivery lifecycle bug: failed deliveries are now terminal for late ack and defer calls, matching the existing settled-state behavior in failDelivery. Added conformance coverage for that case in packages/engine/src/__tests__/conformance/delivery.test.ts.
Local verification passed:
npx turbo buildnpx turbo lint --concurrency=1npx turbo test --concurrency=1- Focused engine/types/SDK tests and builds
… recoverable A reviewer suggested making `failed` terminal for late ack/defer. That conflicts with the retryable-failure contract: a retryable failure should be able to recover by acking (retry succeeded) or deferring (retry later). Document the intended lifecycle on DeliveryStatus and add a conformance test that a failed delivery can still be acked. No behavior change.
|
On the recurring suggestion to make A This is now documented on Generated by Claude Code |
|
Reviewed PR #155 and fixed the build issue in packages/types/src/delivery.ts: changed defer payload validation from Local verification passed:
|
There was a problem hiding this comment.
Reviewed PR #155 and fixed the build issue in packages/types/src/delivery.ts: changed defer payload validation from z.iso.datetime() to z.string().datetime() so it works with the repo’s zod surface and keeps TypeScript builds clean.
Local verification passed:
npx turbo build --forcenpx turbo lint --force --concurrency=1npx turbo test --force --concurrency=1
|
Reviewed PR #155 and made one additional fix. I found that Local checks run: All passed. |
There was a problem hiding this comment.
Reviewed PR #155 and made one additional fix.
I found that /v1/agents/:name still exposed only accepted deliveries in pending_deliveries, while the new durable delivery contract treats accepted + deferred as replayable. I updated packages/engine/src/engine/agent.ts to include deferred deliveries and added conformance coverage in delivery.test.ts.
Local checks run:
npx turbo test lint build --filter=@relaycast/types --filter=@relaycast/sdk --filter=@relaycast/engine
All passed.
GET /v1/agents/:name listed only `accepted` rows in pending_deliveries, but the durable delivery contract treats `accepted + deferred` as the replayable queue (matching GET /v1/deliveries). Include deferred so the two surfaces agree. Adds conformance coverage.
|
Reviewed PR #155 against Local verification run:
The initial broad parallel build/lint attempts were killed with exit 137 due to resource pressure, then passed when rerun serially. |
There was a problem hiding this comment.
Reviewed PR #155 against .workforce/pr.diff, changed files, and related engine/SDK/type call sites. I didn’t find source changes that needed patching, and there were no separate bot review files in .workforce.
Local verification run:
npm run build --workspace @relaycast/typesnpm run test --workspace @relaycast/types -- --runnpm run build --workspace @relaycast/sdknpm run test --workspace @relaycast/sdk -- --run src/__tests__/deliveries.test.tsnpm run build --workspace @relaycast/enginenpm run test --workspace @relaycast/engine -- --run src/__tests__/conformance/delivery.test.tsnpx turbo testnpx turbo lint --concurrency=1 --forcenpx turbo build --concurrency=1 --force
The initial broad parallel build/lint attempts were killed with exit 137 due to resource pressure, then passed when rerun serially.
|
CodeAnt AI is reviewing your PR. |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
| const raw = await c.req.json().catch(() => ({})); | ||
| const parsed = FailDeliveryRequestSchema.safeParse(raw ?? {}); |
There was a problem hiding this comment.
Suggestion: Malformed JSON bodies are silently converted to {} in the fail endpoint, so invalid request payloads can still mutate delivery state to failed instead of returning 400. This breaks API contract consistency with other validation failures and can cause accidental state transitions when clients send bad JSON. Do not coerce JSON parse errors into an empty object; treat parse failure as an invalid request. [logic error]
Severity Level: Major ⚠️
- ⚠️ Malformed JSON still marks deliveries failed instead of 400.
- ⚠️ Durable fail endpoint inconsistent with defer validation behavior.
- ⚠️ Clients cannot reliably detect invalid fail payloads.Steps of Reproduction ✅
1. Start the engine so the HTTP routes in `packages/engine/src/routes/delivery.ts` are
active, including `GET /v1/deliveries` (lines 13–41) and `POST /v1/deliveries/:id/fail`
(lines 80–123).
2. Ensure there is a queued delivery for the agent (for example, via the existing message
delivery flow) and obtain its id (e.g., `del_1`) by calling `GET /v1/deliveries` from
`packages/sdk-typescript/src/agent.ts:8–13`, which hits the `/v1/deliveries` route in
`delivery.ts:16–36`.
3. From any HTTP client, call `POST /v1/deliveries/del_1/fail` with `Content-Type:
application/json` but a malformed JSON body, such as `{"error": "boom"` (missing closing
brace). In the route handler at `packages/engine/src/routes/delivery.ts:82–99`,
`c.req.json()` throws, but the catch at line 87 converts this into `{}`, and line 88 calls
`FailDeliveryRequestSchema.safeParse(raw ?? {})`.
4. The schema `FailDeliveryRequestSchema` in `packages/types/src/delivery.ts:9–12` defines
both `error` and `retryable` as optional, so `safeParse({})` succeeds. The handler then
calls `deliveryEngine.failDelivery` with `opts = {}` at `delivery.ts:95–99`, and
`failDelivery` in `packages/engine/src/engine/delivery.ts:6–31` updates the delivery to
`status: 'failed'` with `error: null` and `retryable: null`, returning 200 OK instead of a
400 error. By contrast, the defer endpoint at `delivery.ts:125–138` treats JSON parse
failure as `null` and returns a 400, showing this swallow-and-default behavior on the fail
path is real and inconsistent with other validation.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/engine/src/routes/delivery.ts
**Line:** 87:88
**Comment:**
*Logic Error: Malformed JSON bodies are silently converted to `{}` in the fail endpoint, so invalid request payloads can still mutate delivery state to `failed` instead of returning `400`. This breaks API contract consistency with other validation failures and can cause accidental state transitions when clients send bad JSON. Do not coerce JSON parse errors into an empty object; treat parse failure as an invalid request.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.trajectories/index.json:
- Line 10: The index file contains an absolute filesystem path in the "path"
field (the trajectory entry value) which leaks local user/workspace info; update
the stored value to a repo-relative path (e.g.
".trajectories/completed/traj_1pjavtw1yit2.json") instead of "/Users/will/…".
Locate where the index is written/serialized (the "path" field for trajectory
entries) and change it to persist path.relative(repoRoot,
absoluteTrajectoryPath) (or otherwise strip the leading workspace/home prefix)
so consumers/CI can resolve it portably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a05a3d3-8849-418c-a627-9da1fc6a056a
📒 Files selected for processing (4)
.trajectories/completed/2026-06/traj_1pjavtw1yit2.json.trajectories/completed/2026-06/traj_1pjavtw1yit2.md.trajectories/completed/2026-06/traj_1pjavtw1yit2.trace.json.trajectories/index.json
✅ Files skipped from review due to trivial changes (3)
- .trajectories/completed/2026-06/traj_1pjavtw1yit2.md
- .trajectories/completed/2026-06/traj_1pjavtw1yit2.trace.json
- .trajectories/completed/2026-06/traj_1pjavtw1yit2.json
The fail body is optional, but `c.req.json().catch(() => ({}))` silently
coerced malformed JSON into {} and still marked the delivery failed (200),
inconsistent with the defer endpoint. Now read the raw body: an empty body
still defaults to {} (valid), but non-empty malformed JSON returns 400 without
mutating state. Adds conformance coverage for both cases.
The recorded trajectory index baked in an absolute local path (/Users/will/...), which isn't portable across machines/CI and leaks local workspace details. Store it repo-relative.
| const targetReason = opts.reason ?? existing.reason; | ||
| const reasonMatches = targetReason === null | ||
| ? isNull(deliveries.reason) | ||
| : eq(deliveries.reason, targetReason); |
There was a problem hiding this comment.
Suggestion: The defer transition reuses the existing delivery reason when the request omits reason, but existing rows are created with acceptance reasons like message/mention. This makes deferred records and delivery.deferred events carry a misleading reason even when no defer reason was provided. Use a defer-specific default (for example null, or only preserve a previous defer reason) instead of inheriting the original acceptance reason. [logic error]
Severity Level: Major ⚠️
- ⚠️ Deferred deliveries inherit acceptance reasons like 'message' or 'mention'.
- ⚠️ delivery.deferred events expose misleading reason metadata to consumers.
- ⚠️ Clients cannot distinguish unspecified defer reason from acceptance reason.
- ⚠️ Durable delivery inbox shows confusing reasons for deferred items.Steps of Reproduction ✅
1. Post a channel message to create delivery rows: in
`packages/engine/src/engine/message.ts:62-75` each recipient gets a `deliveries` row with
`status: 'accepted'` and `reason` set to `'message'` or `'mention'` (see `reason:
(mentionedHandles.has(m.agentName ?? '') ? 'mention' : 'message')`).
2. List the recipient's deliveries via the public API `GET /v1/deliveries` implemented in
`packages/engine/src/routes/delivery.ts:13-41`, which calls `listDeliveries()` in
`packages/engine/src/engine/delivery.ts:39-99`; the serialized delivery includes `reason`
from the DB (`serializeDelivery` at `delivery.ts:14-30`).
3. Defer one of these accepted deliveries without providing a reason by calling `POST
/v1/deliveries/:id/defer` with JSON body `{ "available_at": "<iso-timestamp>" }` (no
`reason` field). The route in `packages/engine/src/routes/delivery.ts:125-147` parses this
with `DeferDeliveryRequestSchema` (`packages/types/src/delivery.ts:66-69`), where `reason`
is optional, so `parsed.data.reason` is `undefined` and `deferDelivery()` is invoked with
`opts.reason` unset.
4. Inside `deferDelivery()` (`packages/engine/src/engine/delivery.ts:203-233`),
`existing.reason` is still the acceptance reason
(`'message'`/`'mention'`/`'dm'`/`'thread-reply'`), so `const targetReason = opts.reason ??
existing.reason;` at line 214 resolves to that acceptance reason when `opts.reason` is
`undefined`. The UPDATE at lines 227-233 persists `reason: targetReason`, and the route's
fanout payload at `routes/delivery.ts:155-162` and the updated delivery returned to the
client both report this inherited acceptance reason as the defer reason, even though the
defer request did not specify any reason. This matches the suggestion's claim that
omitting a defer reason incorrectly reuses the original acceptance reason instead of a
defer-specific default (e.g. `null` or preserving only prior defer reasons).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/engine/src/engine/delivery.ts
**Line:** 214:217
**Comment:**
*Logic Error: The defer transition reuses the existing delivery `reason` when the request omits `reason`, but existing rows are created with acceptance reasons like `message`/`mention`. This makes deferred records and `delivery.deferred` events carry a misleading reason even when no defer reason was provided. Use a defer-specific default (for example `null`, or only preserve a previous defer reason) instead of inheriting the original acceptance reason.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
CodeAnt AI finished reviewing your PR. |
deferDelivery defaulted the reason to the existing row's value, so deferring without a reason inherited the acceptance reason (message/mention/dm/...), making deferred records and delivery.deferred events carry a misleading reason. The defer reason is its own concept — default to null when none is provided. Adds a conformance test.
User description
Summary
Closes #154. Relaycast already maintains per-recipient
deliveriesrows internally, but the public engine/API/SDK surface for server-backed durable delivery handling was missing. This PR adds that contract end to end so Agent Relay (and other downstream SDKs) can report durable delivery support honestly instead of marking it unsupported.What's added
Engine routes (
packages/engine/src/routes/delivery.ts,engine/delivery.ts)GET /v1/deliveries— lists the calling agent's queued items, defaulting to the non-terminal queue (accepted+deferred), oldest-first, each carrying the message payload so an offline consumer can replay what it missed on reconnect. Supports?status=and?limit=.POST /v1/deliveries/:id/ack— idempotently transition a delivery todelivered.POST /v1/deliveries/:id/fail— idempotently recordfailedwitherrortext andretryable.POST /v1/deliveries/:id/defer— idempotently recorddeferredwithavailable_atand optionalreason.delivery.delivered/delivery.failed/delivery.deferredto the recipient agent. Deliveries are agent-scoped — only the recipient can list/ack/fail/defer its own; unknown/unowned ids return404.Types (
packages/types)Delivery,DeliveryItem,FailDeliveryRequest,DeferDeliveryRequest, andDeliveryStatusschemas.delivery.accepted,delivery.delivered,delivery.deferred, anddelivery.failedserver events wired intoServerEventSchema/WsClientEventSchema.SDK (
packages/sdk-typescript)agent.deliveries(),agent.ackDelivery(),agent.failDelivery(),agent.deferDelivery().events.deliveryAccepted/Delivered/Deferred/Failedhandlers.Docs:
README.mdandopenapi.yamldocument the new endpoints and schemas.Notes
deliveriestable (migration 0010) already hasstatus,retryable,available_at,error,deadline, etc.@relaycast/typesdependency from1.1.7→2.0.0(the current workspace version) so it links the local package instead of a stale published copy that shadowed the workspace and lacked the new types.Testing
delivery.test.ts(8 tests): list/ack-idempotency/fail/defer/validation/404/event-fanout.types.test.ts.npx turbo build test lintgreen across@relaycast/types,@relaycast/engine,@relaycast/sdk(plus full build).https://claude.ai/code/session_016a2r9QREZeksLczuGWes4Q
Generated by Claude Code
CodeAnt-AI Description
Include deferred deliveries in the replay queue and clear them when messages are read
What Changed
acceptedanddeferreditems, so deferred messages stay visible for later retry instead of disappearing from the pending listpending_deliveriesto match the delivery queue behaviorImpact
✅ Fewer missed retries after reconnect✅ No stale deferred items in pending delivery lists✅ Clearer agent delivery status💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.