feat(REMOTE-2661): Add idempotency key to support server initiated bootstraps - #65
feat(REMOTE-2661): Add idempotency key to support server initiated bootstraps#65dmichelin wants to merge 7 commits into
Conversation
Adds an AgentPromptPurpose (SetupFailureDebug) and idempotency_key to AgentPromptRequest, so a token-less request can be explicitly authorized to bootstrap a conversation for a specific reason (e.g. debugging a retained environment-setup-failure session) instead of being treated as an ordinary new-conversation prompt. Adds AcknowledgeAgentPromptRequest (sharer -> server) so the sharer can report which conversation it created or reused for such a request, and threads idempotency_key through RejectAgentPromptRequest so a rejection can be recorded under the same key a retry will look up. Co-Authored-By: Warp Agent <agent@warp.dev>
…e debug bootstrap The sharer's AgentPromptRequested handling for a no-token, purpose-tagged bootstrap request (REMOTE-2661) authorized and started the conversation, but never told session-sharing-server which conversation resulted. The server's inject-message endpoint needs that conversation ID back to resolve the caller's synchronous wait, so every bootstrap injection was indistinguishable from a failed one and kept getting retried. - Network::send_agent_prompt_acknowledgement / set_pending_bootstrap_ack / take_pending_bootstrap_ack (sharer): new pending-ack correlation state and the upstream message that reports a bootstrap's resulting conversation token. - terminal_view_adaptor.rs: registers the pending ack right after accepting a purpose-tagged request (both the is_sharer fast path and the post-authorization path), and sends the acknowledgement once BlocklistAIHistoryEvent::ConversationServerTokenAssigned reports the new conversation's token. - idempotency_key threaded through every send_agent_prompt_rejection call site so a bootstrap rejection is recorded under the same key a retry looks up. - Viewer-side network.rs updated for the new required purpose/idempotency_key fields on AgentPromptRequest (always None for an ordinary live-viewer prompt) and the new AgentPromptFailureReason::NotEligibleForPurpose variant. Depends on warpdotdev/session-sharing-protocol#65 for the new AgentPromptPurpose/idempotency_key/AcknowledgeAgentPromptRequest wire types; pinned to that PR's branch commit until it merges. cargo check -p warp: clean. cargo check -p warp --tests: clean. cargo clippy -p warp --lib -- -D warnings: clean. cargo fmt --check -p warp: clean. Co-Authored-By: Warp Agent <agent@warp.dev>
|
Consumers: warpdotdev/session-sharing-server#483 (server-side inject endpoint + storage), warpdotdev/warp#14916 (client sharer-side ack). Part of the REMOTE-2661 debug-agent feature (see also warpdotdev/warp-server#14231). |
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Reviewed for size as part of a request to trim REMOTE-2661's ~5.4k added lines across four repositories. 65 lines is the smallest diff in the feature but it carries the largest structural cost, and roughly half of it serves a design the spec ultimately rejected.
Concerns
- The
purposehalf of this contract is unused (~30 lines here, suggestion).AgentPromptPurpose::SetupFailureDebug,AgentPromptRequest.purpose, andAgentPromptFailureReason::NotEligibleForPurpose(src/common/agent_prompt.rs:25-59) were introduced so a sharer could treatpurposeas proof of follow-up authorization.specs/REMOTE-2661/TECH.md:278-282records that design as considered and rejected in favor of asetupFailureDebugAuthorizationcallback, andTECH.md:13states the shared-session repos are unaffected. Tracing all four PRs confirms no producer: Warp's sharer authorizes through the callback regardless ofrequest.purposeand returnsInsufficientPermissionson denial, neverNotEligibleForPurpose. What warp-server actually consumes isidempotency_keyplusAcknowledgeAgentPromptRequest; those are the parts worth keeping, and "no conversation ID + idempotency key" is a sufficient bootstrap predicate on an authenticated injection. UpstreamMessagehas no unknown-variant fallback (important).src/sharer.rs:580-606addsAcknowledgeAgentPromptRequestto a closed serde enum. A new Warp sharer talking to an older session-sharing-server therefore has no compatible decode path for the acknowledgement, which is what forces the ordered four-component rollout (this crate published →session-sharing-server#484deployed →warp-server#14231→warp#14916). If that ordering cannot be guaranteed in every environment, an explicit compatibility arm is cheaper than the sequencing. I verified the missing arm by reading the enum; I did not exercise an old-server/new-client websocket, so the precise failure mode is inferred.
Removing the purpose surface takes ~115 lines out of this PR and session-sharing-server#484 together, plus another ~20-30 in the two consumer PRs, and it makes TECH.md's bootstrap section stop claiming that purpose authorizes anything — the spec is currently inconsistent with itself on that point. The acknowledgement and idempotency contract itself is load-bearing and should stay: PRODUCT.md §35 requires that a retry after a lost acknowledgement reuse the same conversation, and Warp's own task-ID map is an in-memory pre-token guard that cannot cover that case on its own.
Verdict
Checks: build not run, tests not run, CI not checked, visual proof n/a — read-only analysis.
Found: 0 critical, 1 important, 1 suggestion, 0 nits
Responding as wilson: Open session · View factory task
REMOTE-2661's spec (TECH.md:278-282) records purpose-as-authorization as considered and rejected in favor of the setupFailureDebugAuthorization callback, and nothing consumes it. The load-bearing half is bootstrap correlation: no server_conversation_token plus an idempotency_key, on an authenticated injection. Removes AgentPromptPurpose, AgentPromptRequest.purpose, and AgentPromptFailureReason::NotEligibleForPurpose. Keeps AgentPromptRequest.idempotency_key unchanged.
The upstream reject/acknowledge docs referred to a `purpose`-tagged request. The predicate is now an idempotency key with no server_conversation_token.
The enum is a closed serde enum, so a peer built against a newer protocol version cannot be decoded at all: one unknown variant fails the whole decode. An untagged catch-all is tried after every externally tagged variant, so an unrecognized message degrades to something the receiver can ignore.
warp-agent-staging
Bot
changed the title
|
Both findings from the size review are addressed. There were no inline threads to reply in, so this is the reply to that review.
No unknown-variant fallback (important) — added in Responding as wilson: Open session · View factory task |
The fallback's whole value is tolerating a variant that does not exist yet, so no current code path exercises it and no consumer fails if it silently regresses. Cover it directly: an unrecognized variant decodes to Unknown, a recognized variant still round trips externally tagged, and a variant carrying bytes still decodes, since the trailing untagged variant makes serde buffer every variant's content. Needs no dev-dependency or test harness the crate did not already have.
Measured in release on identical payloads, the untagged trailing variant forces serde's content-buffering path for the whole enum and roughly doubles the cost of decoding the highest-volume message: PtyBytesRead at 32 KiB went 349.5 us -> 694.9 us, and at 256 B went 4.113 us -> 6.486 us. It also bought nothing. The decode site already tolerates an undecodable message: session-sharing-server's ws_util.rs logs the error together with the raw text and continues the loop. The fallback replaced that with a successful decode into a variant carrying neither, so the observable behavior was unchanged and the diagnostics were worse. It also turned a malformed known variant into a silent skip, which for AcknowledgeAgentPromptRequest means a bootstrap failing as a timeout rather than a decode error. Reverts the variant and the tests that pinned it, restoring src/ to 329e1f3.
|
Correcting my earlier reply: the unknown-variant fallback has been reverted in Two reasons, both verified rather than reasoned about. It cost: measured in release on identical payloads, the untagged trailing variant forces serde's content-buffering path for the whole enum and roughly doubles the decode cost of the highest-volume message — The rollout ordering was never something the fallback could relax; an already-deployed peer carries the old closed enum either way. The PR description now documents the real old-server behavior instead. Responding as wilson: Open session · View factory task |
Bumps the protocol pin to warpdotdev/session-sharing-protocol#65 head (4888e758), where `AgentPromptRequest.purpose` and `AgentPromptFailureReason::NotEligibleForPurpose` no longer exist. Replaces the `purpose`-tag gate on registering a bootstrap acknowledgement with the predicate session-sharing-server itself applies: an idempotency key with no conversation to continue. The two must agree -- a bootstrap the server waits on but the sharer never acknowledges is treated as an undelivered injection and retried until the caller's deadline -- and both sides evaluate it against the parsed token, so an absent or unparseable conversation id counts as no conversation on either side.
The comment asserted that the sharer must remember a key's outcome for as long as the server may retry. No sharer implements that and none ever did; the client-side map that briefly existed was keyed by task id, not by the transmitted key. Deduplication is the session-sharing service's, which claims the key before injecting, so the sharer never sees a redelivery it has already answered.
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR extends the session-sharing wire protocol for bootstrap agent prompts by adding an optional idempotency key to AgentPromptRequest and RejectAgentPromptRequest, plus an upstream acknowledgement that reports the resolved conversation token.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
- No approved spec context was available beyond the PR description.
- Comment/test audit: the added public doc comments explain protocol semantics and match the surrounding wire-type style; no tests were added or changed.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
dmichelin
changed the title
| /// injecting, so a retry of a key it has already answered does not reach the sharer. The | ||
| /// sharer is not required to keep per-key state. | ||
| #[serde(default)] | ||
| pub idempotency_key: Option<String>, |
There was a problem hiding this comment.
Super-nit: Can we make it clear that this is specifically used for bootstrap in the name? The ai_task associated with this run itself will also have an idempotency key but that's not the one we are using here AFAICT.
3 participants
dmichelin note
This adds a new field to the AgentPromptRequest,
idempotency_keythat lets warp-server send a message via the session-sharing-server to the warp client, and receive a conversation ID. The intent is that this is essentially a callback: a user types/agent [prompt]into a remote warp client in a retained session with no conversation ID, warp-server receives that request then relays it to the session sharing server, the sandbox warp-client receives a signal to initiate a session which then calls back to the session sharing server, then the session sharing server relays back the conversation ID to warp-server. Other follow ups follow the old convention.Summary
Adds the wire types a server-side injection needs to bootstrap exactly one conversation in a shared session that has no conversation yet, and to learn which conversation it landed in (REMOTE-2661).
A bootstrap request is identified by its predicate, not by a tag:
server_conversation_tokenabsent andidempotency_keypresent, on an injection the session-sharing service has already authenticated as coming from warp-server. That server-to-service authentication is what distinguishes this path from a live viewer's ordinary new-conversation prompt; it is unchanged by this PR.Changes
AgentPromptRequest.idempotency_key: Option<String>— so a redelivered bootstrap (the first acknowledgement was lost) reuses the same conversation instead of starting a second one and abandoning the first turn.UpstreamMessage::AcknowledgeAgentPromptRequest— sharer → server, reports the conversation token the sharer created or reused, keyed by that idempotency key. This is the only way the server learns the conversation ID for a token-less prompt.idempotency_keythreaded throughUpstreamMessage::RejectAgentPromptRequest, so a rejection is recorded under the same key a retry looks up.Both new fields on existing types are
#[serde(default)], per the crate's backward-compatibility rule insrc/lib.rs. The new variant's own fields are not, and do not need to be: a sender old enough to omit them cannot produce the variant in the first place.Call flow
Consumers
warpdotdev/session-sharing-server#484(injection endpoint, durable acknowledgement storage) andwarpdotdev/warp#14916(sharer-side acknowledgement). Feature context:warpdotdev/warp-server#14231, specwarpdotdev/warp-server#14216.Both pin this crate by git
rev, and both are deliberately pinned at4888e75rather than this branch's head. Every commit after it is documentation only and cannot change either consumer's compiled artifact, so re-pinning would churn two lockfiles for no build difference. A single bump to the merged SHA after this lands picks it up.Validation
cargo build,cargo test,cargo fmt --check, andcargo clippy --all-targets --all-features -- -D warningsall pass. This is a types-only change; the crate has no test module, matching its state before this PR.Plans: