Repository navigation
fix(native-chat): accept sends through their own receipt - #26672
Conversation
Adds the agent_session_command_receipts table and an insert-if-absent receipt write that runs inside a command's own journal transaction. A receipt records the command's identity, canonical hash, status and a pointer to the row holding its result; it dies with its chat record. Nothing uses it yet: the existing operation ledger stays authoritative.
Rows are keyed by caller and operation id; a send looks across callers and other methods look up only their caller's row, matching the existing ledger. Bad rows and newer table shapes read as unreadable instead of throwing.
…tion id Global lookups now take rows in insertion order, so a read and an insert agree on which receipt answers an id regardless of the host clock. The primary key leads with the operation id, so its own index serves both lookups.
Send (direct, queued, steer) and /compact no longer commit a separate ledger reservation before the message is saved. Their command receipt is inserted-if-absent in the same transaction as the submission or queued draft; a repeated id replays from it, a changed request conflicts. Operation ids are checked for syntax only, so age, clock skew, pruning and the reservation's own storage can no longer refuse a send. The ledger success row they already co-committed stays as a temporary insert-if-absent write so families not yet switched keep seeing these ids; it is never read on this path.
…d space with Send, wake delivery once - After its commit, a send whose publication fails answers through the same receipt replay a retry takes, so one id gets one answer; the in-memory copy and journal reload are gone. - /compact ids are global like Send's: a retry under a new connection replays instead of needing a second acknowledgment write. - The receipt route calls its runner directly; the runner wakes delivery when a new message row commits. - The compatibility ledger row expires from host time; each temporary site is labelled. - Receipt read errors and post-commit failures are logged.
…d ledger Restart-continuation tests now model the case Send cannot vouch for (saved but not loaded into memory) and keep it unconfirmed; a later throw after the message is in the chat answers continued, as a resend would. Drop /compact's unreachable legacy replay branch and fix stale comments.
Review summaryThe problem. A native-chat Send could be refused ("too many requests", "this request has expired", a clock-skew refusal, or a failed reservation write) before the message was ever saved. The cause: a separate retry-reservation transaction, with its own age, clock, cleanup and storage rules, ran in front of every Send and What changes for the user. Send and How. The command's receipt (from #26653) is inserted-if-absent in the same database transaction as the message or queued draft. No separate admission, pruning or budget runs for these commands. Other command families still use the old ledger. Until they are switched, a TEMPORARY ledger row written in the same commit keeps cross-command id conflicts visible. Fixed during review (3 rounds plus a readiness pass):
Deferred (P3, tracked for later PRs in this stack):
Main sync (dca127e). #26653 merged, so this PR now targets main. What the merge changed:
Verified
Not verified
Seen in QA, not caused by this PR (same on the base build): typing a second message while the agent works steers the running turn instead of queueing it, and the first turn's reply never appears. |
Send and /compact now route on acceptsWithCommandReceipt: main made /clear and rewind settlesWithWrite plans with their own success receipts, and they stay on ledger settlement. /compact keeps #25704's queued answer under receipt replay.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughSend and Priority: ➖ Normal Estimated code review effort: Merge Risk: 🔵 Low · up to The change is mergeable with owner awareness of a possible duplicate compaction when an older /compact request is retried after upgrade. Pre-merge checks |
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
64ae6660-fd6d-42b4-af28-b4157021dc42
📒 Files selected for processing (41)
config/scripts/vitest-sqlite-runtime-files.mjssrc/main/native-chat/agent-session-journal/command-receipt-schema.tssrc/main/native-chat/agent-session-journal/command-receipt-table.test.tssrc/main/native-chat/agent-session-journal/command-receipt-transaction.test.tssrc/main/native-chat/agent-session-journal/command-receipt-transaction.tssrc/main/native-chat/agent-session-journal/journal-row-writer.tssrc/main/native-chat/agent-session-wire/structured-agent-session-attachment-admission.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-command-admission.tssrc/main/native-chat/agent-session-wire/structured-agent-session-command-receipt.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-admission.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-context.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-context.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-refusals.tssrc/main/native-chat/agent-session-wire/structured-agent-session-operation-settlement.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-operation-settlement.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-command.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-gate.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-pause-lift.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-repeat-press.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-send.tssrc/main/native-chat/agent-session-wire/structured-agent-session-refusal-retry.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-resend-answer.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-resend-ledger-refusal.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-failure-filing.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-interruption-test-harness.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-ownership.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-progress.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-send-preparation.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-send.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-turns.tssrc/main/native-chat/agent-session-wire/structured-conversation-compaction.test.tssrc/main/native-chat/agent-session-wire/structured-conversation-compaction.tssrc/main/runtime/agent-session-operation-admission.test.tssrc/main/runtime/agent-session-operation-admission.tssrc/main/runtime/agent-session-operation-receipt.tssrc/main/runtime/agent-session-record-rows.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-store-transactions.tstests/e2e/cross-version-wire/versioned-agent-session-wire.ts
💤 Files with no reviewable changes (1)
- src/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| if (outcome.status === 'succeeded' && outcome.conversationCommand) { | ||
| return { recorded: outcome.conversationCommand } | ||
| } | ||
| replay: (ctx) => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C30 "B15" src/main/native-chat/agent-session-wire/structured-conversation-compaction.test.ts
rg -n -C5 "priorRecord" src/main/native-chat/agent-session-wireRepository: stablyai/orca
Length of output: 14394
🏁 Script executed:
set -eu
printf '%s\n' '--- current compaction implementation ---'
nl -ba src/main/native-chat/agent-session-wire/structured-conversation-compaction.ts | sed -n '45,90p'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.ts | sed -n '180,235p'
printf '%s\n' '--- relevant tests around B15 and committed records ---'
rg -n -C12 --glob 'structured-conversation-compaction.test.ts' 'phase: .committed.|conversationCommand|B15|older build' src/main/native-chat/agent-session-wire
printf '%s\n' '--- base versus head for the relevant flow ---'
git diff 37833ce9a02ccd6754dc9b4eb3266d39ddf8a2fd dca127ecdf5b7904b47413978ad2ee56cfb7b6b0 -- src/main/native-chat/agent-session-wire/structured-conversation-compaction.ts src/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.tsRepository: stablyai/orca
Length of output: 42178
🏁 Script executed:
set -eu
printf '%s\n' '--- current compaction implementation ---'
nl -ba src/main/native-chat/agent-session-wire/structured-conversation-compaction.ts | sed -n '45,90p'
printf '%s\n' '--- current conversation command plan ---'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.ts | sed -n '180,235p'
printf '%s\n' '--- tests for prepared/committed conversation records ---'
rg -n -C12 --glob 'structured-conversation-compaction.test.ts' 'phase: .(prepared|committed).|conversationCommand|B15|older build' src/main/native-chat/agent-session-wire
printf '%s\n' '--- base versus head for the relevant files ---'
git diff 37833ce9a02ccd6754dc9b4eb3266d39ddf8a2fd dca127ecdf5b7904b47413978ad2ee56cfb7b6b0 -- src/main/native-chat/agent-session-wire/structured-conversation-compaction.ts src/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.tsRepository: stablyai/orca
Length of output: 41787
Guard only committed legacy compactions in run.
run calls performSend without checking the session record. A committed record from an older build can therefore rerun when its receipt or submission is gone.
The proposed priorRecord() guard must not be used directly. It converts matching prepared records to compactionUnconfirmed, so that guard would break B15, which intentionally reruns ledger-only prepared operations. Add a committed-only callback for run and keep priorRecord for replay.
Suggested fix
export function conversationCommandPlan(params: {
envelope: AgentSessionMutationEnvelope
delivery?: 'queue-if-active'
priorRecord: () => AgentSessionConversationCommandResult | null
+ committedPriorRecord: () => AgentSessionConversationCommandResult | null
}): MutationPlan<ConversationCommandAcceptance> {
@@
recoverUnknownFromDurableState: true,
run: async (ctx) => {
+ const prior = params.committedPriorRecord()
+ if (prior) {
+ return { ok: true, value: { recorded: prior } }
+ }
const sent = await performSend(ctx, { const priorRecord = (): AgentSessionConversationCommandResult | null => {
const prior = context.deps.store.getRecord(sessionId)?.conversationCommand
@@
}
}
+ const committedPriorRecord = (): AgentSessionConversationCommandResult | null => {
+ const prior = context.deps.store.getRecord(sessionId)?.conversationCommand
+ return prior?.operationId === clientOperationId &&
+ prior.callerKey === caller.callerKey &&
+ prior.phase === 'committed'
+ ? prior
+ : null
+ }
@@
params,
priorRecord,
+ committedPriorRecord,
arrival…eipts-send # Conflicts: # src/main/runtime/agent-session-record-store.ts
ELI5
You press Send in a native chat and Orca says "Orca has received too many requests in the last day. Your message was not sent." Nothing was wrong with the message. Before Orca saves a message, it first writes a separate "I've seen this request" reservation into a shared retry table. That reservation step has its own rules:
Any of them can refuse the send before the message is ever saved. An earlier fix (#26543) removed the count limit, but the step itself still runs before every send.
After this PR, Send and
/compactskip that step. The proof that a request was accepted (its "receipt", added in #26653) is saved in the same database transaction as the message. The reservation rules (age, clock skew, cleanup, the separate write) can no longer refuse a send.What Changed
User-facing:
Which commands:
agentSession.sendin every delivery mode (sent immediately, queued while the agent is busy, or added to the running turn) andagentSession.conversationCommand(/compact). These are the commands that accept by writing a message row.Mechanism:
admitAndRunAgentSessionMutation, the single route every mutating call takes, still recomputes and checks the request hash, then hands these commands toadmitCommandReceiptMutation(structured-agent-session-command-admission.ts). The id is checked for format only. Its embedded timestamp is not compared with the clock./compactshare one id space and match any caller.JournalOperationReceipt.writenow receives the row it commits with, so the receipt points at the sequence actually assigned. A small combinator lets one write carry several receipts. A queued send's receipt points at its draft.TEMPORARYcomment so the final PR can remove it in one pass.replayed: true. A message that was saved is never reported as "not sent". If the saved row could not be loaded into memory, Send answers accepted with a "may not have been sent" submission and/compactanswers "outcome unknown". The error is logged. On the base branch this case threw an internal error after the message was saved./compactare removed.A restart's automatic "continue" message goes through the same Send path. If its message is saved but can't be loaded into memory, it is still filed as unconfirmed with the chat warning. If a later step fails after the message is in the chat, it is reported as continued, because it was.
Why
The bug class is a bookkeeping transaction with its own refusal policy placed in front of the user's write. Removing one more rule (the age limit, say) would leave the class in place. The common pattern removes it: a command's receipt is inserted-if-absent inside the same transaction that commits the command's effect, and a duplicate reads the existing receipt. It has no separate admission and no count or age budget.
Alternatives considered:
/compactscoped per caller. Rejected: it forced a second "acknowledge another caller's message" path that threw internal errors on an already-accepted/compact./compactmessages are already keyed by the same operation id as Send, so one global scope is the simpler, correct model.Differences from the common pattern:
/compactuse the global scope.Old ledger-only rows (development data only): a send or
/compactrecorded only in the old ledger, with no receipt and no message in the current history, is not treated as replay proof. A retry of that id runs. Native chat has no real users yet, so there is no backfill. This would not be safe for existing users' data and is not claimed to be.Linked Issue
Builds on #26653 (receipt store, merged). Follow-up to #26543 (removed the count caps).
Visual Proof
Host-side change; checked in a hidden, isolated dev build of this branch (OMP agent against a local fake model), driven over the app's own RPC and composer. Screenshots from the PR build unless marked "before".
Testing
After the main sync (dca127e): node, cli and web typechecks pass, and 46 focused files / 585 tests pass, including
/clear, rewind, queued/compactand operation-settlement tests. Before it, focused suites at dfe3c3f: 32 files, 384 tests passed.orca-ci-checks(lint, code quality, localization, typecheck) passed 23/23. They cover:/compactreplay across callers gives one message and one provider command;Six existing tests in files this PR hadn't changed still expected the old ledger answers (CI caught them). They now expect the receipt answers. The restart tests were reworked as described above rather than flipped.
The QA screenshots above cover a normal send, queued delivery, a same-id retry while queued, a same-id/different-body conflict, and old/future ids before and after.
Not verified in the app:
/compact, because the OMP agent used for QA can't compact, so this PR's/compactpath wasn't reached there; it is covered by the automated tests. Also not reached in the app: a failure after commit.The change is host-side. SSH and WSL hosts run the same code on their own database. RPC parameters, result shapes and published stream bytes are unchanged. The only value change is
replayed: trueon a first reply after a post-commit failure (an internal error before), and no client reads that field.AI Disclosure
Review
Agent skill upstream boundary
Notes
Author: @BrennanKB5. Stack: #26653 (receipt store, merged) → this PR (Send,
/compact). Later PRs each switch one family: queued Send Now/Delete/Resume and prompt answers, Stop, settings and goals, create/rewind//clear. The final one removes the TEMPORARY ledger row and native chat's use of ledger admission, and decides the schema-version question.Synced with main after #26653 merged. Main had made
/clearand rewind commit their own success with their write (settlesWithWrite). So the receipt route now keys on its own plan flag (acceptsWithCommandReceipt, set only on Send and/compact), and/clearand rewind keep the ledger settlement they have on main. A/compactthat waits in line (#25704) is answered through receipt replay from its queued card, like a queued Send. Main's own delivery wakes in queued Send and/compactwere dropped in the merge, so the receipt runner stays their only owner.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)