Skip to content

refactor(native-chat): move the phone's send-id retention rule into shared code - #24915

Closed
brennanb2025 wants to merge 2 commits into
mainfrom
brennanb2025/chat-send-retained-id-shared
Closed

brennanb2025 wants to merge 2 commits into
mainfrom
brennanb2025/chat-send-retained-id-shared

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​131 $\color{#cf222e}{\Huge{\mathbf{−}}}$​48 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​83
Prod 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​121 $\color{#cf222e}{\Huge{\mathbf{−}}}$​94 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​27

ELI5

The phone app already has a careful rule for not sending the same chat message twice. This PR moves that rule out of the phone's code into code the desktop app can also use. Nothing changes for anyone yet: the phone runs exactly the same rule, now from its new home.

What Changed

The problem this prepares for. In the desktop's native chat, a message that fails to send stays in the conversation with a small red line ("Message was not sent." / "Message delivery is unconfirmed.") and a Retry button. The planned fix (a follow-up to #24606) gives the failed message's text back to the composer instead. That is only safe if sending the same text again can't deliver it twice when the first attempt may in fact have reached the agent.

The phone already solves this. Every send carries an operation id, and the host delivers a given id at most once. The phone keeps ("retains") the id for a message whose fate is unclear, keyed by what the message says. Re-sending the same text therefore reuses the same id, and the host answers it from its record instead of delivering again. Two pieces decide this:

  • The "spent" rule: given the host's answer to a send, whether the id is used up (a later identical message is a new message and gets a new id) or must be kept (the message may already be with the agent).
  • The intent key: what a retained id is saved under. It is the session, the text with trailing whitespace trimmed, each attachment's fingerprint, and whether the send asked to be queued.

What moved. Before, both lived in the phone app (mobile/src/session/mobile-structured-send-delivery.ts, and inline in mobile-structured-agent-session-send.ts plus mobileStructuredSendOperationKey in the phone's operation journal). After, they live in src/shared/structured-agent-session-send-retention.ts:

  • structuredAgentSessionSendDelivery (the spent rule, unchanged line for line);
  • structuredAgentSessionSendIntentKey (the intent key);
  • structuredAgentSessionSendOperationKey (the last hashing step of that key).

The phone imports them from there. The call-result type the spent rule reads (StructuredAgentSessionMutationCallResult) moved from the phone's RPC module into src/shared/structured-agent-session-mutation.ts, next to the fingerprint functions, because shared code can't import from the phone. The phone's send-outcome type (MobileNativeChatSendOutcome) is now an alias of the shared one, so the four outcomes are listed in one place. The test fixture for a host's send answer moved to src/shared too, so the shared and phone tests use one copy.

The reliability gate for "one send id reaches the agent at most once" (config/reliability-gates.jsonc) listed the phone's old delivery test, which this move deletes. It now runs the shared test instead, with fresh evidence runs.

What users see. No change on the phone or the desktop. The desktop does not use the moved code yet; the follow-up does.

Saved keys are unchanged. The phone stores retained ids on the device under the intent key. The hashing domains keep their mobile.* names, so a phone upgraded across this change still finds the ids it saved. A new test rebuilds the key the way the phone code did before this PR and checks they match.

Why

The follow-up plans to reuse this rule on the desktop rather than write a second copy. Two versions of "when is this id used up" would drift, and that rule is what keeps one message from reaching the agent twice. Moving it first, as a change with no behaviour difference, keeps the follow-up's review about behaviour only.

This is not yet the desktop's rule, and the follow-up will have to change it. The desktop's current outbox decides the same question differently in at least two cases:

  • A send that throws a plain failure: the desktop keeps the id and retries under it (disposeStructuredAgentSessionSendFailure), while this rule spends it on a first attempt.
  • An id the host refuses as expired: the desktop's Retry gives the message a new id (structured-agent-session-outbox-retry.ts), while this rule keeps it on a retained retry.

The follow-up must settle each of these on purpose. Its design is still open, and the PR should wait for it (see Review).

The rule also relies on what a failed call result means: that the request provably did not run. Today only the phone's RPC wrapper produces that result and guarantees it. Any desktop caller has to give the same guarantee.

Alternatives considered:

  • Copy the rule into desktop code. Rejected: two rules to keep in step on a duplicate-delivery path.
  • Move the whole retained-id store too. Out of scope: the phone's store is built on the phone's storage API. The desktop will keep its retained ids in its own storage in the follow-up.

Known issue (not fixed here)

The phone's retained ids never expire (mobile/src/session/mobile-structured-send-operation-journal.ts, "Ambiguity has no TTL"). The moved rule holds the same latch: when the host refuses a retried id because it has expired, the rule keeps the id instead of spending it (the refused branch of structuredAgentSessionSendDelivery). Sending the same text again is then refused for good. The host stops accepting an id after its replay window, so an expired retained id is refused for good and is never released. Once 4,096 build up (AGENT_SESSION_DURABLE_OPERATION_GLOBAL_LIMIT), the phone refuses every new send ("Message not sent"). This PR is a pure move and leaves that alone. Follow-up: an expired refusal spends the id, retained ids expire with the host's replay window, and a full store evicts expired ids first instead of blocking the send.

Linked Issue

N/A (maintainer). This is the shared-rule step of the native-chat failed-send follow-up to #24606.

Visual Proof

N/A: a code move with no behaviour change. No screen draws differently.

Testing

  • The spent rule's tests moved with it to src/shared/structured-agent-session-send-retention.test.ts, unchanged apart from names. So did the operation-key hashing test from the journal tests.

  • New intent-key tests: the key equals one rebuilt from the hashing domain names alone, the way the old phone code built it (with and without queueing); and different sessions, texts, attachments and queueing each give a different key.

  • Ablation: renaming the operation domain, the intent domain, or the image-preview domain each fails the saved-key test. Reverted.

  • Both commands of the at-most-once reliability gate pass: 13 root files, 196 tests; 8 phone files, 112 tests. The wider phone send, cancel, stop, journal and native-chat suites pass too (15 files, 224 tests).

  • Reliability gate manifest check passes.

  • Typecheck: node project clean; web project clean (run before the second commit, which touches no web code); the phone project and its test-file ratchet have no errors outside the existing generated-module ones.

  • Lint: oxlint clean, formatting clean, and the changed-code quality gate has 0 new findings.

  • Platforms: pure functions with no platform, SSH or folder-workspace dependence. Not run in the app.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Reviewed for move fidelity, regressions and design. No runtime defect was found. The saved phone keys come out byte-identical to the old code's over edge-case inputs, and the phone bundle pulls in the same modules as before. Hold this as a draft until the follow-up's design is decided: if that design keeps unclear sends in the outbox instead of keying a kept id by the message text, the desktop never needs this rule and this move isn't needed.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

  • Mobile: same code paths, same saved keys.
  • Backwards compatibility: no wire or stored-format change.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Author: @BrennanKB5

…hared code

The phone decides when a retained send operation id is spent, and keys retained
ids by session + trimmed text + attachment fingerprints. Both now live in
src/shared/structured-agent-session-send-retention.ts so the desktop can reuse
them. The phone imports them from there; no behaviour change, and the saved
key domains are unchanged so retained ids on an upgraded phone still match.
…'s tests

The reliability gate still named the phone's old delivery test, which the move deleted, so the
manifest check failed. It now runs the shared test, with fresh evidence runs. The send-answer
fixture moves to src/shared so the shared and phone tests use one copy, and the phone's send
outcome type becomes an alias of the shared one. The saved-key pin now rebuilds the operation
domain literally, so renaming it fails the test.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review summary

The problem. In the desktop's native chat, a message that fails to send stays in the conversation with a red line and a Retry button. A planned follow-up gives the text back to the composer instead. That is only safe if sending the same text again can't deliver it twice. The phone already avoids double delivery: it keeps the operation id of any send whose fate is unclear, and reuses it when the same text is sent again. This PR moves that phone rule into shared code so the follow-up could use it on the desktop.

What changes for users. Nothing, on the phone or the desktop. The phone runs the same rule from its new location, and no desktop code uses it yet.

What the review checked.

  • The move changes nothing. Apart from renames and one comment word, the rule is the old phone function line for line. The keys the phone saves on the device come out byte-identical to the old code's: base and head were compared side by side over edge cases (trailing whitespace, emoji, empty values, attachments with and without fingerprints), for both normal and queued sends. A phone upgraded or downgraded across this change still finds the ids it saved.
  • The phone bundle is unchanged. Bundling four phone entry points before and after pulls in the same files, except the moved one.
  • Nothing else broke. Every test the old phone files had is still there. All 205 phone session test files pass (1,969 tests), and so do the shared tests. The reliability-gate manifest check, oxlint and the changed-code quality gate pass. Nothing on current main uses the old names, and the branch merges cleanly. CI is green.
  • Readiness checklist: no P0, P1 or P2 issues.

Fixed in this round (PR description only, no code change).

  • The description said the desktop needs "this exact rule". It doesn't. The desktop's current outbox answers "is this id used up?" differently in at least two cases:

    • A plain thrown failure: the desktop keeps the id, while this rule spends it on a first attempt.
    • An id the host refuses as expired: the desktop's Retry gives it a new id, while this rule keeps it.

    The description now says the follow-up must settle these differences on purpose.

  • The description now states the guarantee the rule relies on: a failed result means the request provably did not run. Today only the phone's RPC layer produces that result and guarantees it.

  • The known-issue section now points at the rule itself. When the host refuses a retried id because it has expired, the rule keeps the id instead of spending it, so the same text is refused for good. The phone's missing expiry on kept ids is the other half of the same bug. This bug already existed before this PR, which doesn't fix it.

Deferred.

  • (P2) The expired-id latch above. Its fix belongs in the follow-up that changes behaviour, not in a pure move.
  • (P2) A code comment on the failed guarantee. It is needed once a second caller produces that result; today there is none.
  • (P3) The saved-key test rebuilds its expected value with the same hashing function. A literal expected hash would also lock the hashing itself.
  • (P3) The phone send module restates an attachment type that the shared module now exports.
  • (P3) The rule still returns the phone's outcome and English error text, and its header comment describes a client without an outbox. Both need to change if the desktop adopts it.

Why this stays a draft. Nothing in the code is wrong. Whether this PR should land depends on the follow-up's design, which is still open. If the follow-up keeps unclear sends in the outbox under their original id, rather than keying a kept id by the message text, the desktop never needs this rule. The common pattern works that way. In that case this move adds churn without a user.

Verified / not verified.

  • Verified: tests, saved-key identity, bundle contents, gates and lint (above).
  • Not verified:
    • Not run in the app. There is no screen change to capture, and the phone bundle contents are unchanged.
    • No typecheck on the merged-with-main tree. The node, web and phone typechecks passed on the PR head before review, and CI's typecheck is green.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing this. The send design settled on a different shape: a send's id belongs to its saved outbox entry and is resent under that same id until the host answers, as the common pattern's outbox client does. Ids are not remembered by message text. The phone's text-keyed id store will be replaced when the phone gets the same saved outbox (a later PR), so moving that rule into shared code would spread the shape we're removing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant