feat(frontend): show the frozen content and diff on commit approvals - #5762
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // The manifest is a SIBLING of the payload, never inside it, so the generic card has to render | ||
| // it itself: the frozen content is what the approval binds, in every mode. Skipped when a | ||
| // specialized body is active, because that body renders the manifest already. | ||
| const fallbackManifest = useMemo( |
There was a problem hiding this comment.
Why this memo has a gate
Two components can render the same manifest. The specialized commit body renders it in Chat mode. This generic card renders it in every other mode. Both read the same current.manifest value.
The renderer ? null gate keeps only one of them active. When a specialized body is present, this memo returns null. The specialized body then owns the render.
If you remove the gate, Chat mode shows the diff, the file list, and the digest two times on one card. A reader can understand that as two separate changes to approve.
If you move the render out of the else branch, you get the same double render. The specialized body still draws its own copy.
The current?.manifest dependency matters as well. A turn can hold several gates. The dock steps through them one at a time. The memo must recompute when the shown gate changes, or the card keeps the content of the previous gate.
| } | ||
|
|
||
| /** Manifests keyed by toolCallId, from the egress's `data-approval-manifest` sibling parts. */ | ||
| const manifestsByToolCallId = (parts: UIMessage["parts"] = []): Map<string, unknown> => { |
There was a problem hiding this comment.
Why the manifest arrives beside the tool call and not inside it
This function reads the data-approval-manifest parts of the turn. It keys each manifest by its tool call id. The dock then joins each pending gate to its own manifest.
The manifest cannot travel inside the tool input. There are two reasons.
The first reason is ownership. The tool input holds the arguments that the model wrote. The manifest holds bytes that the runner read from the sandbox. If we merge them, the card shows the user a payload that the model never sent. The user must be able to trust that the payload is the model's own text.
The second reason is the frame format. The tool-approval-request frame is a strict object with a fixed key set. It cannot carry an extra field.
The join is therefore load bearing. If the tool call id does not match, the card falls back to the raw arguments. Those arguments hold a file marker such as {"@ag.file": "instructions.md"}. That is a path and nothing more. The user then approves bytes that the user never saw.
| } | ||
| // Mirrors the live egress's sibling data part, so a reloaded transcript renders the | ||
| // same card. `tool-approval-request` is a strict object and cannot carry this. | ||
| if (reqPayload.manifest !== undefined) { |
There was a problem hiding this comment.
Why replay rebuilds the part instead of reading the manifest directly
The live stream sends the manifest as a separate data part. A page reload does not replay the live stream. A reload reads the stored session records. This block turns the stored gate record back into the same part shape that the live stream produces.
The card therefore reads one shape only. It does not test whether the run is live or reloaded. That keeps the live path and the replay path from drifting apart over time.
If you delete this block, a reload degrades the card. The gate then shows the raw arguments, and those arguments hold a file path only. Approve stays active in that state. A user can approve content that the card never displayed. This is the exact state the card must never reach.
One related fact for a cold reader. The runner attaches the manifest to the emitted event, but it does not write the resolved content into the durable interaction row. The transcript record is the only replay source for this data.
| * The agent's own note about this call (R12), labelled so it reads as a claim and not as a fact | ||
| * about what the commit does. The real diff sits beside it (read-config.md section 12.2, rule 5). | ||
| */ | ||
| const StatedIntent = ({intent}: {intent: CallDescription}) => ( |
There was a problem hiding this comment.
Why the card labels two texts that look similar
This block shows the per-call description. The commit message gets a separate label below it.
The two texts are different things.
The description sits at the top level of the tool input, beside workflow_revision. It explains this call to the person who is watching. The runner deletes it before it builds the HTTP request, so the platform never stores it. The card keeps it because the runner strips it at dispatch time, after the gate.
The message sits inside workflow_revision. It is the commit message. The platform saves it on the new revision and shows it in the history.
The card previously showed the message alone, and it showed it with no label. A reader takes an unlabelled quote in that position as the agent explaining the call. That reading is wrong, because the message describes the change in the history.
Both texts come from the model. Neither text is a fact about what the call does. The labels separate them, and the real diff stays beside them. If you remove the labels, the card again presents stored history as live intent.
The contract for this rule is docs/design/agent-config-editing/contracts/read-config.md, section 12.2.
8751d4e to
ab22e0f
Compare
dd50fb6 to
a2883a1
Compare
ab22e0f to
fda241d
Compare
a2883a1 to
4cc3fb3
Compare
fda241d to
2e6b425
Compare
|
@coderabbitai review |
|
4cc3fb3 to
7f77f92
Compare
2e6b425 to
8a32996
Compare
7f77f92 to
e80845e
Compare
8a32996 to
d75f18b
Compare
e80845e to
8d275fd
Compare
d75f18b to
00e5676
Compare
|
@coderabbitai review |
|
00e5676 to
9ef9f89
Compare
0837141 to
67d75b2
Compare
0b3b5ad to
92e7619
Compare
67d75b2 to
10e7d71
Compare
92e7619 to
803ae6d
Compare
10e7d71 to
28316b4
Compare
803ae6d to
e44019a
Compare
28316b4 to
395f4ea
Compare
e44019a to
c19ceaf
Compare
395f4ea to
2be2849
Compare
c19ceaf to
d7dac02
Compare
2be2849 to
6a9d9d7
Compare
d7dac02 to
1d88425
Compare
6a9d9d7 to
aa02582
Compare
1d88425 to
ae7c8c3
Compare
The approval dock renders the frozen manifest and the base-bound diff through the existing commit_revision registry body: file list with sizes, digests, and executable flags, single-file mode as a unified diff. One consumer serves the live data part and the replayed transcript; the manifest resolves by toolCallId on both paths.
…ecked) renderToStaticMarkup over a new test-library dependency: the tree executes, props flow, every branch runs; interaction stays untested and says so. Brief corrections recorded: the 50-file cap is unreachable (8-marker commit cap) and deliberately untested; the 400-line cap is runner-side and the card asserts its diffTruncated sentence.
…abels agent intent (final review F4, F12)
…iff sections open by default (E2E U3-U4)
…copes labelled (UI re-run residual)
Railway Preview Environment
|
…-removal refactor (keep the Badge idiom from release, keep the defaultOpen/sectionOpenState feature from the lane, drop the stale antd imports)
1 participant
Context
When an agent decides to save a new version of itself, the run pauses and the approval dock asks you to approve the commit. Until now that card could not show you what you were approving.
The reason is the shape of the tool call. When the agent commits content from a file in its workspace, its arguments carry a marker, not the bytes:
{"operation": "set", "target": ["instructions"], "value": {"@ag.file": "instructions.md"}}That is a path and nothing else. The bytes live in the sandbox. The runner resolves them at the gate, freezes them, and builds an approval manifest with the file sizes, digests, executable bits, and a unified diff against the text being replaced. The manifest cannot ride inside the tool input, because the input is the model's own arguments and must stay that way, and it cannot ride on the approval request either, because that object has a strict key set. So it arrives as a sibling
data-approval-manifestpart, and the frontend had no code reading it. You saw a path, and Approve was one click away.This lane wires that manifest into the card. It targets its stack parent
agent-config-editing-s3b-wire-pyand is part of the agent-config-editing stack.Changes
ApprovedContentManifestis the new component that renders what a commit imports: a unified diff per replaced field with added and removed line counts, then each imported file with its path, size, executable bit, and digest, then a sentence stating that the digest covers the full content and not just the part shown. It renders the runner's diff text as-is.DiffViewfrom@agenta/uiis deliberately not used, because it normalizes content as JSON or YAML and would mangle a Markdown instructions document.ApprovalDocknow collects manifests from the turn's sibling parts, keys them by tool call id, and hands the right one to the card.transcriptToMessagesemits the same data part when a stored transcript replays, so a page reload renders the same card instead of a poorer one.The card shows this in both UI modes. Chat mode gets it inside the two-pane commit body, beside the "What's changing" summary. Build mode and any host without a selected revision get it under the raw payload block. This matters because Build mode is the default: the config panel is open unless you maximize the chat, so it is the card most approvals actually get.
Before, in Build mode, the whole card body was the payload block:
After, the same gate adds the substance under it:
The commit card also stops presenting the persisted commit message as the agent's intent. Those are two different things. The per-call
descriptiondescribes the call to the human watching and is never saved. Themessageinsideworkflow_revisionis the commit message and is saved on the revision. The card used to render only the message, unlabelled, in the position where a reader takes it as the agent explaining itself. It now readsdescriptionand shows it under "What the agent says it is doing", with the message below it under "Commit message". A description longer than the catalog's 500-character cap is cut and marked as shortened.Tests / notes
The AgentChatSlice suite runs 95 tests across 15 files, all passing, plus a clean
tsc --noEmitoverweb/oss. Most of that is new:ApprovedContentManifest.test.tsxcovers the diff, file, and digest rendering including truncation;ApprovalDock.test.tsxcovers which branch renders the manifest;CommitRevisionApproval.test.tsxcovers both card shapes with and without imported content and with a distinct description and message;transcriptToMessages.test.tscovers the replay mirror.Two notes for a reviewer. The dock tests use the real registry, card, and manifest components and mock only the presentational shells, so the "renders exactly once in Chat mode" assertion is real and not an artifact of a stub. And the manifest tests were checked for vacuity: with the manifest render disabled in place, exactly the three Build-mode assertions failed and the rest passed.
One deliberate limit. Build mode still shows the raw payload plus the manifest rather than the full two-pane commit body. Enabling the specialized body in Build mode is a separate UX decision, tracked in
docs/design/agent-config-editing/open-issues.md. This lane makes the content visible everywhere first, because that is the safety property.What to QA
Added by the E2E campaign and review fix rounds (5 Aug, evening)