Repository navigation
Send selected review comments to the authorized session - #161
Merged
Merged
Conversation
…tract `ReviewCapabilities` advertised only `rerunnable_checks`, which is why the batch send-to-agent and staging controls have been shipping as stated refusals rather than controls. bext-stack/automonique PR #221 lands the contract layer: `agent_deliverable_comments` plus three independently withheld pull-request slots. This commit is mechanical and grants nothing on its own. `ReviewCapabilities` grew two constructor arguments, and the response it travels in is now boxed upstream because the type is far larger than the refusal it competes with in the same enum; `PlatformReviewCapabilitiesLoad::Available` follows suit for the same reason, matching `PlatformReviewLoad::Available`. The lockfile change is limited to the two `automonique-*` source lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nf2PAqdKZxRuZSYGq3NE6o
`ReviewCapabilities` now advertises `agent_deliverable_comments`, so the control this issue asked for can finally be fenced. Fleet renders a send toggle only for the comments that list names, and dispatches `batch_send_comments_to_agent` with each `comment_id` and `expected_comment_revision` taken verbatim from the capability, at the capability's own `snapshot_revision`. The snapshot is consulted only to prove the two reads agree; it supplies no value that crosses the network. This is the unconfirmed lane, and that is deliberate rather than an omission. A rerun needs a digest because the client names the target and the effect fires in a system the daemon does not own. Delivery differs on every axis that motivated one: the session is registry-owned and never on the wire, the snapshot revision is fenced twice server-side, and the note set is fenced by the server's batch arm. Exactly-once falls out of the domain state machine. Minting a receipt correlation would have been worse than redundant, because the host's receipt lookup skips a retained action carrying one, so the client would have handed itself a token that makes its own receipt unfindable. What replaces the digest is the advertisement, and the rule that follows is the one this change exists to enforce: re-read it, never reuse it. Settling a delivery moves the comment out of `not_sent`/`refused` and bumps both the snapshot revision and the comment's own revision, so a re-read cannot still list it. `matches_capabilities` therefore revalidates a batch preview before it crosses the dispatch fence, and the persisted selection is retained against the advertisement rather than the snapshot, so a stale list empties itself instead of being posted. The custody fence is not weakened to fit. This lane needs no confirmation digest, but it is an exposed mutation and takes the same durable at-most-once record as every other one. That required extending the custody disk format, which knew only `AddComment`, `ApproveReview` and `RerunCheck` — so a batch delivery could not be recorded at all and would have refused at the fence. Two locale strings were corrected while passing: the staging refusal claimed Platform v2 publishes only the check rerun, which stopped being true with this contract, and the notes panel claimed batch delivery was unavailable. A note is a local draft with no server comment id, so its durable selection marks which drafts to prepare; delivery names server comments, and says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nf2PAqdKZxRuZSYGq3NE6o
SDUC-495 still said batch delivery was unexposed because `ReviewCapabilities` advertised only check reruns, and the staging entry repeated that claim as its reason. Both stopped being true when the contract grew `agent_deliverable_comments`, so the catalogue now describes what is actually exposed, why the lane carries no confirmation digest, and the one rule that follows from having no digest: re-read the advertisement, never reuse it. The staging reason is narrowed rather than dropped. Staging is still withheld, but for its own reasons — no staging digest, no receipt correlation, no representable hunk granularity — not because the rerun is the only capability that exists. SDTEST-1863..1866 are added at Green. SDTEST-1863 is the one worth reading: it demonstrates the stale-list case instead of asserting it, by replaying the server's own eligibility rule across a settlement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nf2PAqdKZxRuZSYGq3NE6o
11 tasks done
benfavre
added a commit
that referenced
this pull request
Aug 29, 2026
The combined staged/unstaged/untracked/conflicted view has shipped since PR #159. Only the mutations were missing, and for one structural reason: `ReviewCapabilities` carried no staging capability, so a stage could only have gone out unconfirmed and uncorrelated, and an ambiguous outcome would have been reconcilable by nothing but an uncorrelated key search. That is exactly what the confirmed lane exists to avoid. The capability landed, so this consumes it, following `delivery.rs` rather than inventing a second shape: the two are the same problem, one read-side projection reporting what the server proved plus which fence is missing when it proved nothing. Absence is the withholding. The three staging kinds share one capability type but not one grant: an operator installs `index_write`, `commit` and `conflict_resolution` independently. A deployment granting stage and unstage but not commit therefore advertises two entries, and the commit proposal keeps its observation badge and grows no button. A disabled button with a tooltip would claim the grant is present and merely unavailable, which is the opposite of what the list says. `conflict_resolutions` lists one entry per admissible side, because the side decides which blob lands and a digest cannot commit to a choice not yet made. A file with both stage 2 and stage 3 recorded renders two controls; a delete/modify pair renders one. Three things this had to touch beyond the obvious: `review_action`'s confirmation guard now asks `requires_confirmation()` instead of restating it as a local `matches!` on the rerun. The confirmed set grew from one member to eight with this protocol revision, and the local spelling had already gone stale for `merge_pull_request`, which was minting previews the transport could no longer encode. That constructor now takes its capability like the rerun does, so it refuses honestly instead of failing at the wire. `custody.rs` did not understand these four families. It would have returned `UnsupportedAction` at `prepare` and the dispatch would have died at the fence, which is precisely what PR #161 found for the batch delivery after PR #160 asserted the format already knew "these actions". It knew the two it named. The disk enum now spells each staging kind separately, because a recovered plan that could be re-read as another kind would let a narrower grant replay as a wider one, and a conflict resolution persists its side because the side is part of the effect's identity. Receipts are polled through `GetReviewReceipt` with the correlation digest, which `lookup_review_action` already routes for any confirmed preview: re-sending the action would be pointless anyway, since the write invalidated the confirmation it was minted with. A stale worktree withdraws the control locally rather than reaching the daemon. The server mints the confirmation over the commit HEAD resolved to, the whole index and every named path, so comparing the digest against a fresher capability document *is* the head-and-index comparison; the observation is exposed on the projection so a reader can see the fence rather than trust that one exists. Hunk granularity is not built and not faked. Proposals list file ids and `ReviewAnchor`'s hunk id serves comments only, so file-level staging is all the contract can express. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nf2PAqdKZxRuZSYGq3NE6o
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the batch-send half of #128.
The persistence half shipped in #160. Batch send was withheld for one reason:
ReviewCapabilitiesadvertised onlyrerunnable_checks, so no servercapability could fence it and the surface stated the gap instead of offering a
control.
bext-stack/automoniquePR #221 landsagent_deliverable_comments,which is exactly the missing piece.
What ships
Fleet renders a send toggle only for the comments that capability names,
and dispatches
batch_send_comments_to_agentwith eachcomment_idandexpected_comment_revisiontaken verbatim from the capability, at thecapability's own
snapshot_revision. The snapshot is consulted only to provethe two reads agree; it supplies no value that crosses the network.
The advertised list is already sorted by comment id server-side, and the
protocol requires a batch to be strictly ordered by that same id, so the
selection is filtered out of the advertisement rather than walked in its own
order — wire order falls out for free. Walking the selection would not have,
and
validate_client_shapewould have rejected it.An empty list means fail closed: no control, with its own stated reason,
distinct from an absent capability response and from a missing custody lane.
Why there is no confirmation digest, and why that is the design
A rerun needs one because the client names the target and the effect fires in
a system the daemon does not own. Delivery differs on every axis that
motivated it: the session is registry-owned and never on the wire, the
snapshot revision is fenced twice server-side, and the note set is fenced by
the server's batch arm. Exactly-once falls out of the domain state machine.
Minting a receipt correlation would have been worse than redundant: the
host's receipt lookup skips a retained action carrying one, so the client
would have handed itself a token that makes its own receipt unfindable.
The rule that replaces the digest
The advertisement is the fence, so it is re-read after a receipt settles
rather than reused. Two places enforce it:
matches_capabilitiesnow revalidates a batch preview before it crosses thedispatch fence — previously it returned
trueunconditionally for everyunconfirmed action.
snapshot, so a settled delivery empties it instead of leaving a stale list
selectable.
The custody fence is not weakened to fit the lane
Batch delivery needs no confirmation digest, but it is an exposed mutation and
takes the same durable at-most-once record as every other one. That required
extending the custody disk format, which knew only
AddComment,ApproveReviewandRerunCheck— so a batch delivery could not be recordedat all and would have refused at the fence.
The test worth reading
SDTEST-1863 demonstrates the stale-list property rather than asserting it.
It replays the server's own eligibility rule (fresh review, review authority,
not_sent/refused) as a test helper, so advertisements are computed, nothand-written. It then sends exactly what was advertised, settles one delivery
through the same transition the daemon performs, shows the re-read
advertisement drops only that comment, and shows the preview built from the
superseded list refused — including a fresh preview over the same batch and the
same advertised revisions under a new idempotency key, which is the exact
request a duplicate-delivery bug would have to make.
Both new fences were mutation-checked. Neutering the batch arm of
matches_capabilitiesfails SDTEST-1863; dropping the snapshot-revision checkin
advertised_agent_deliveriesfails SDTEST-1863 and SDTEST-1864. Thefirst pass of SDTEST-1864 survived that second mutation, because it only
exercised the projection and not
advertised_agent_deliveriesdirectly — thepath the action preview actually calls. That gap is now closed in both tests.
Also corrected while passing
Two locale strings had become false with this contract: the staging refusal
claimed Platform v2 publishes only the check rerun, and the notes panel claimed
batch delivery was unavailable. Staging is still withheld, but now for its own
reasons. A note is a local draft with no server comment id, so its durable
selection marks which drafts to prepare; delivery names server comments.
Scope
ReviewCapabilitiesalso grew three pull-request slots. They ship emptyupstream and are untouched here beyond the mechanical constructor change; this
PR adds no pull-request control.
Verification
cargo test --workspace --exclude shelldeck-ssh(768 passed, 0 failed),cargo clippy --workspace --no-deps -- -D warningsclean,cargo fmt --all -- --checkclean. The lockfile diff is limited to the twoautomonique-*sourcelines.
SDTEST-1863..1866 added; SDUC-495 amended with a
## Change logentry.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nf2PAqdKZxRuZSYGq3NE6o