feat(api): preserve send_at across a review hold - #833
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an interoperability bug between two beta outbound features (scheduled send via send_at and HITL review holds) by persisting the schedule on held drafts and re-arming the appropriate enqueue path on approval, while also surfacing scheduled_at consistently across API responses, SDKs, the review queue, and the web dashboard.
Changes:
- Persist
scheduled_atwhen an outbound message is held for review, and re-arm scheduled vs immediate sending when the hold is approved (including TTL auto-approve). - Surface
scheduled_atonpending_reviewresponses, approve responses, and review queue items; update OpenAPI + regenerated SDK models/docs accordingly. - Add/extend coverage across Go tests, a shared contract scenario, and web UI tests for scheduled chips.
Reviewed changes
Copilot reviewed 22 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/app/components/types.ts | Extends pending review summary type to include optional scheduled_at. |
| web/src/app/components/onboarding/api.ts | Threads scheduled_at through review wire types into PendingMessageSummary. |
| web/src/app/(app)/reviews/_components/PendingRow.tsx | Renders a scheduled-send chip for held outbound drafts that carry a schedule. |
| web/src/app/(app)/reviews/_components/PendingRow.test.tsx | Adds UI tests for scheduled chip rendering (future/lapsed/absent). |
| tests/contract/scenarios.yaml | Adds shared contract scenario ensuring schedules survive holds and are preserved through approval. |
| sdks/typescript/src/v1/generated/models/SendResultView.ts | Updates scheduledAt documentation to include held + approval behavior. |
| sdks/typescript/src/v1/generated/models/SendEmailRequest.ts | Updates sendAt documentation to reflect schedule preservation across review holds. |
| sdks/typescript/src/v1/generated/models/ReviewView.ts | Adds optional scheduledAt field to review queue model. |
| sdks/typescript/src/v1/generated/models/ReplyRequest.ts | Updates scheduled-send docs for reply to match new semantics. |
| sdks/typescript/src/v1/generated/models/ForwardRequest.ts | Updates scheduled-send docs for forward to match new semantics. |
| sdks/typescript/README.md | Updates SDK docs to reflect schedule preservation across review holds and self-send rejection. |
| sdks/python/src/e2a/v1/generated/models/send_result_view.py | Updates scheduled_at documentation to include held + approval behavior. |
| sdks/python/src/e2a/v1/generated/models/send_email_request.py | Updates send_at documentation to reflect schedule preservation across review holds. |
| sdks/python/src/e2a/v1/generated/models/review_view.py | Adds optional scheduled_at field to review queue model. |
| sdks/python/src/e2a/v1/generated/models/reply_request.py | Updates scheduled-send docs for reply to match new semantics. |
| sdks/python/src/e2a/v1/generated/models/forward_request.py | Updates scheduled-send docs for forward to match new semantics. |
| sdks/python/README.md | Updates SDK docs to reflect schedule preservation across review holds and self-send rejection. |
| mcp/src/tools/messages.ts | Updates MCP send_at field description to match new semantics. |
| internal/identity/trash_hold_guard_test.go | Updates test call site for the expanded ApproveAndAccept signature. |
| internal/identity/store.go | Extends ApproveAndAccept to read back scheduled_at and enqueue scheduled sends on approval when still future. |
| internal/identity/review.go | Includes scheduled_at in review list/detail queries for queue surfacing. |
| internal/identity/approve_accept_test.go | Updates test call sites for the expanded ApproveAndAccept signature. |
| internal/httpapi/reviews.go | Surfaces scheduled_at on review queue API responses. |
| internal/httpapi/outbound.go | Updates send/reply/forward request docs and includes scheduled_at on held send responses. |
| internal/httpapi/hitl.go | Returns status=scheduled + scheduled_at on approval when the preserved schedule is still future. |
| internal/hitlworker/worker.go | Extends outbound enqueuer interface and wires scheduled enqueue through TTL auto-approve. |
| internal/hitlworker/async_approve_test.go | Adds auto-approve test ensuring a future schedule uses the scheduled enqueue arm. |
| internal/agent/schedule_hold_test.go | Adds end-to-end-ish Go tests for hold persistence and approval re-arming semantics. |
| internal/agent/hitl_api.go | Wires scheduled enqueue into the API/dashboard approval path via ApproveAndAccept. |
| internal/agent/api.go | Persists schedule in hold transaction and rejects future scheduled self-sends before the hold branch. |
| docs/api.md | Updates public API docs to reflect schedule survival across holds. |
| api/openapi.yaml | Updates send_at/scheduled_at documentation and adds ReviewView.scheduled_at. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A scheduled send (send_at) caught by an HITL review hold no longer loses its schedule. The schedule is persisted on the held draft and re-armed on approval: submitted at send_at if still in the future, or immediately if it has already passed. Closes tokencanopy#815. - agent: stamp send_at on the held row (same tx as the hold); reject a future send_at for a self-send up front, since loopback can't schedule. - identity: ApproveAndAccept reads the draft's scheduled_at and enqueues on the scheduled River arm when still future, immediate otherwise. Wired through both the API approve path and the TTL auto-approve worker. - httpapi: surface scheduled_at on the pending_review send response and the approve response (status flips to scheduled on re-arm); add ReviewView.scheduled_at so the review queue shows the scheduled time. - web: render a "Sends ..." chip on held drafts in the review queue. - mcp/docs/SDK READMEs: rewrite the "does not survive a review hold" wording. - spec + TS/Python SDK bases regenerated; shared contract scenario scheduled_send_survives_review_hold added for the Go/TS/Python runners. - tests: agent + hitlworker re-arm coverage, identity signature updates, web PendingRow chip tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6b5c216 to
bc1c161
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/src/app/(app)/reviews/_components/PendingRow.tsx:300
formatScheduledFor(summary.scheduled_at)is called twice in the render condition and body. Because it usesDate.now()andtoLocaleString(), the two calls can produce inconsistent results (e.g., crossing the boundary from future→past between calls) and does redundant work. Compute the label once and render based on that single value.
{summary.scheduled_at && formatScheduledFor(summary.scheduled_at) && (
<Chip tone="info">{formatScheduledFor(summary.scheduled_at)}</Chip>
)}
|
Closing this PR: its head branch was deleted/recreated, which permanently desynced GitHub's PR head (pinned to a commit missing the #837 fix). The branch feat/schedule-survives-hold now holds main + this change + the #837 fix at a23a377. Replaced by a fresh PR from that branch (same content) to keep the merge deterministic. |
Summary
Closes #815.
A scheduled send (
send_at) that is caught by an HITL review hold no longerloses its schedule. Previously
send_atwas discarded at accept time, so anapproved message went out immediately on approval instead of at the
requested instant — the two beta features (scheduled send + review hold) did not
compose. Now the schedule is persisted on the held draft and re-armed on
approval:
send_atstill in the future at approval → return toscheduledand submit atsend_at.send_atalready passed at approval → submit immediately ("not before" is satisfied, and approval was the last blocker).send_atis also surfaced (asscheduled_at) on thepending_reviewsendresponse, on the approve response, and on the review queue, so a caller/reviewer
can see the message will send on a schedule rather than immediately.
How it's implemented
The fix has two halves — persist at hold, re-arm at approval — plus one
design observation that makes status/response parity fall out for free.
1. Persist the schedule on the held draft (
internal/agent/api.go).When outbound screening holds a message,
HoldForApprovalCoreThreadednow stampssend_atonto themessages.scheduled_atcolumn in the same transaction as thehold, using the existing
StampScheduledAtTx— the exact mechanism the directscheduled-accept path already uses (migration 084 added the column; no new
migration). The held
OutboundResultcarriesScheduledAtso thepending_reviewresponse and its idempotency-cached replay both surface it.
A future
send_atto the agent's own address is now rejected up front, beforethe hold branch — a self-send is delivered by an immediate in-process loopback
that has no scheduled arm, so it can never honor a schedule. This supersedes the
old "the hold takes precedence over the loopback check" behavior, under which a
held self-send silently dropped
send_at.2. Re-arm on approval (
internal/identity/store.go).ApproveAndAcceptreads the draft's ownscheduled_atback in the compare-and-setUPDATE … RETURNING(snapshot-safe: it's a plain UPDATE,scheduled_atisn'tmodified in that statement — no data-modifying-CTE re-select). If it is still in
the future, the send is enqueued via
EnqueueScheduledSendTx(at)(River first-run=
scheduled_at) instead of the immediateEnqueueSendTx; if it has alreadypassed, it takes the immediate arm. This is wired through both approval paths:
the account-scoped API/dashboard approve (
agent.approveOutboundAsyncComposed) andthe TTL auto-approve worker (
hitlworker), which now also carries the scheduledenqueuer on its
OutboundEnqueuerinterface.3. Why status/response parity is free.
Outbound uses a two-column model:
statusis the hold/lifecycle column,delivery_statusis the send progression. An approved message already lands atstatus=sent+delivery_status=accepted— identical to a directly-scheduledrow. So the only difference a scheduled approval needs is (a) which River arm
enqueues and (b) presenting
scheduled_at. No new status value, no message-shapedivergence: the approve response reports
status=scheduled(derived from a still-futurescheduled_at, mirroringSendResultViewon the direct path), and the persistedrow is indistinguishable from a normal scheduled send for the SendWorker, the
trash/restore path, and the fire-time monthly-cap re-check.
Client surface checklist
ApproveAndAccept(+ TTL worker); unit/integration coverage inagent,hitlworker,identity.messages.scheduled_atalready exists (084).make generate;send_atdocs rewritten on send/reply/forward,SendResultView.scheduled_atdoc, newReviewView.scheduled_at; bothgenerated/trees regenerated.--send-atalready flows through (verified live).send_atdescription rewritten (no longer says the schedule is dropped on hold).ReviewView.scheduled_at→PendingRow).Testing
scheduled_at;ApproveAndAcceptre-arms when future vs sends now when past; both the API approve and TTL auto-approve worker re-arm; held self-send + futuresend_at→ 400 up front.scheduled_send_survives_review_hold(schedule → hold → approve →status=scheduled,scheduled_atpreserved), proven through the Go, TypeScript, and Python live-server runners.PendingRowchip tests (future → "Sends …", lapsed → "Sends on approval", none → absent).status=scheduledand submitted at the target instant (confirmed via message lifecycle: queued at accept, submitted ≈scheduled_at), not at accept time.Compatibility
send_atand the review queue are both beta, so this sits inside the betasurface. The change is additive on the wire (new optional
scheduled_aton theheld/approve responses and
ReviewView); the only behavioral change —"approval sends immediately" → "approval honors the schedule" — is the bug being
fixed and is unlikely to be a deliberate dependency.