feat(frontend): friendly tool approvals in agent chat mode - #5101
Conversation
Resolve a commit_revision delta ({set, remove}) against the current committed
parameters, mirroring the backend's semantics (dicts deep-merge; scalars and
lists replace; dotted-path removes), and classify the result into the same
ChangeSection list the commit modal renders. Partial deltas never misreport
untouched sections as removed; malformed or no-op deltas return null so
callers can fall back to raw-payload rendering.
…t modal Export it from @agenta/entity-ui/modals and add two presentation props: compact (no frame paddings, capped scroll height, no JSON toggle — for hosts without the modal's fixed h-full frame) and size="small" (tighter section-card density). Section and detail-view headers now stick to the top of the scroll area while their content scrolls — the CARD frame's overflow-hidden clipped sticky positioning, so cards are split into a sticky header block plus a framed body. Detail headers collapse back to the summary on click.
Chat (maximized) approvals lose the raw chrome: tool names are humanized into a single ask sentence (raw name stays under the tooltip and the Details payload expander), and the trace link becomes Build-only. Build keeps the exact-payload card unchanged. A per-tool renderer registry (mirroring the client-tools registry) lets approvals opt into fully custom bodies. First entry: commit_revision renders the commit modal's What's-changing sections — the delta is resolved against the revision's committed parameters (the same base the backend applies it to) and shown as a two-pane card with the agent's commit message, drill-in diffs, and an Approve & commit action. Unknown tools, missing entityId (the session-chat host), or unpreviewable deltas all fall back to the generic card.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
size:XL
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/packages/agenta-entity-ui/src/modals/commit/components/changes/AgentChangesSummary.tsx (2)
243-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim multi-line comments to comply with the one-line comment guideline.
Several of the new/changed comments span 3–4 lines (Lines 243-245, 302-304, 461-464), and 302-304 largely re-explains the same "CARD's overflow-hidden clips sticky positioning" rationale already documented on
DetailCard(Lines 243-245). Consider trimming to one line each, or factoring the shared rationale into a single comment referenced from both call sites.As per coding guidelines, "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception."
Also applies to: 302-304, 461-464
Source: Coding guidelines
260-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer Tailwind arbitrary-value utilities over inline
stylefor CSS vars.These new/changed lines use
style={{background: "var(--ag-colorBgContainer)"}}andstyle={{color: "var(--ag-colorWarning)"}}, while the same component already uses Tailwind arbitrary syntax for identical CSS vars elsewhere (e.g.border-[var(--ag-colorBorderSecondary)],bg-[var(--ag-colorFillTertiary)]). Neither case is a complex Ant Design override or JS-calculated dynamic style, so these could be Tailwind classes instead.As per coding guidelines, "Prefer Tailwind utility classes over CSS-in-JS, inline
style={{...}}, and separate CSS files; use CSS-in-JS only for complex Ant Design overrides, JS-calculated dynamic theme styles, or legacy components."♻️ Example fix
-<div - className="sticky top-0 z-[1] rounded-t-[10px]" - style={{background: "var(--ag-colorBgContainer)"}} -> +<div className="sticky top-0 z-[1] rounded-t-[10px] bg-[var(--ag-colorBgContainer)]">Also applies to: 307-310, 513-514
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40972c25-f4d4-48be-be48-3ea0d33338ef
📒 Files selected for processing (11)
web/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/components/ApprovalDock.tsxweb/oss/src/components/AgentChatSlice/components/approvals/CommitRevisionApproval.tsxweb/oss/src/components/AgentChatSlice/components/approvals/registry.tsxweb/packages/agenta-entities/src/workflow/commitDiff/index.tsweb/packages/agenta-entities/src/workflow/commitDiff/revisionDelta.tsweb/packages/agenta-entities/tests/unit/revision-delta.test.tsweb/packages/agenta-entity-ui/src/modals/commit/components/changes/AgentChangesSummary.tsxweb/packages/agenta-entity-ui/src/modals/commit/components/index.tsweb/packages/agenta-entity-ui/src/modals/commit/index.tsweb/packages/agenta-entity-ui/src/modals/index.ts
| const deepMerge = ( | ||
| base: Record<string, unknown>, | ||
| patch: Record<string, unknown>, | ||
| ): Record<string, unknown> => { | ||
| const merged = {...base} | ||
| for (const [key, value] of Object.entries(patch)) { | ||
| const current = merged[key] | ||
| merged[key] = isRecord(current) && isRecord(value) ? deepMerge(current, value) : value | ||
| } | ||
| return merged | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant slice with line numbers.
FILE="web/packages/agenta-entities/src/workflow/commitDiff/revisionDelta.ts"
wc -l "$FILE"
cat -n "$FILE" | sed -n '1,220p'Repository: Agenta-AI/agenta
Length of output: 4558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any existing guards against prototype-pollution-style keys in the repo,
# and identify call sites that pass untrusted payloads into the merge path.
rg -n '__proto__|constructor|prototype|deepMerge|classifyRevisionDeltaChanges|commit.delta|CommitRevisionApproval' web/packages/agenta-entities web/packages -g '!**/dist/**' -g '!**/build/**'Repository: Agenta-AI/agenta
Length of output: 17328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact implementation and any nearby helpers/types.
FILE="web/packages/agenta-entities/src/workflow/commitDiff/revisionDelta.ts"
sed -n '1,220p' "$FILE"Repository: Agenta-AI/agenta
Length of output: 3841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("web/packages/agenta-entities/src/workflow/commitDiff/revisionDelta.ts")
print(p.read_text())
PYRepository: Agenta-AI/agenta
Length of output: 3842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the merge function, its callers, and the source of `commit.delta`.
FILES=(
"web/packages/agenta-entities/src/workflow/commitDiff/revisionDelta.ts"
"web/packages/agenta-oss/src/components/workflow/CommitRevisionApproval.tsx"
)
for f in "${FILES[@]}"; do
echo "===== $f ====="
if [ -f "$f" ]; then
cat -n "$f" | sed -n '1,240p'
else
echo "MISSING"
fi
done
echo "===== search ====="
rg -n "classifyRevisionDeltaChanges|commit\.delta|deepMerge|delta" web/packages -g '!**/dist/**' -g '!**/build/**'Repository: Agenta-AI/agenta
Length of output: 17105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const patch = JSON.parse('{"__proto__":{"polluted":true},"ok":1}')
const base = {safe: 1}
const merged = {...base}
console.log("entries:", Object.entries(patch))
for (const [key, value] of Object.entries(patch)) {
merged[key] = value
}
console.log("merged.hasOwnProperty('__proto__'):", Object.prototype.hasOwnProperty.call(merged, "__proto__"))
console.log("merged.polluted:", merged.polluted)
console.log("Object.getPrototypeOf(merged).polluted:", Object.getPrototypeOf(merged).polluted)
console.log("({}).polluted:", ({}).polluted)
JS
rg -n 'UNSAFE_KEYS|__proto__|prototype|constructor' web/packages/agenta-entities/src/workflow/commitDiff web/packages/agenta-entities/tests/unit/revision-delta.test.tsRepository: Agenta-AI/agenta
Length of output: 350
Guard deepMerge against __proto__ keys.
Object.entries(patch) plus merged[key] = ... lets an untrusted delta mutate the merged object’s prototype when patch includes __proto__. Skip that key before assigning.
Review follow-ups on #5101: - deepMerge/removePath now read and write own keys only (defineProperty, hasOwnProperty), so special keys like __proto__ land as literal dict keys exactly as the backend resolver treats them, instead of hitting the prototype setter; regression test added. - The two new sticky-underlay backgrounds move from inline style to Tailwind arbitrary-value classes. - Trimmed the duplicated split-frame comments per the one-line rule.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Context
When the agent pauses on a tool approval, the chat shows a raw card: the wire tool name (
commit_revision,tools__composio__gmail__ADD_LABEL__b81), a JSON payload dump, and Approve/Deny. That is the right surface for Build mode, where you debug by exact names and payloads. In Chat mode (the maximized, non-technical view) it is unreadable. Thread: https://agentagroup.slack.com/archives/C0BBU3T45S5/p1783091983904759Changes
Chat-mode approvals get a friendly card. Build mode is unchanged.
Every tool (chat): the raw-name row becomes one sentence with a humanized name, reusing
parseGatewayToolNameplus anmcp__{server}__{tool}branch. The raw name stays one hover away (tooltip) and the exact payload one click away (expander, relabeled "Details" in chat). The "View full trace" link is now Build-only.Before (chat):
commit_revision+ payload JSON + ApproveAfter (chat, generic tool): "The agent wants to use Add label from Gmail before it can keep going." + Details expander
commit_revision(chat): a per-tool renderer registry (same dispatch pattern as the client-tools registry) routes it to a dedicated body. The payload's delta is resolved against the revision's committed parameters with the backend's exact semantics (dicts deep-merge; scalars and lists replace; dotted-path removes), then classified into the commit modal's "What's changing" sections. The card is two panes: the ask and the agent's commit message on the left, section accordions with drill-in diffs on the right. Approve reads "Approve & commit".Supporting changes:
@agenta/entities/workflow/commitDiff/revisionDelta.ts:classifyRevisionDeltaChanges(currentParams, delta). Returns null on malformed or no-op deltas, which is the fallback signal.AgentChangesSummaryis now exported from@agenta/entity-ui/modalswithcompact(no frame paddings, capped scroll, no JSON toggle) andsize="small"(tighter accordion density). Accordion and detail headers now stick to the top of the scroll area (the old CARD wrapper's overflow-hidden clipped sticky positioning); the pinned detail header collapses back to the summary on click. The commit modal picks up the sticky headers too.Tests
revision-delta.test.ts): list replacement, no phantom removals on partial deltas, dotted-path removes, base immutability, null fallbacks.turbo buildfor@agenta/entitiesand@agenta/entity-ui; OSStscclean on all touched files; pre-commit prettier, turbo lint, and gitleaks pass.commit_revisionflow, including a 210-line instructions diff (scroll cap, pinned header, clamped commit message). Light theme and the batched "1 of N / Approve all" case are not yet eyeballed; QA below covers them.What to QA
commit_revisionname, "Payload" expander, and "View full trace", exactly as before.