fix(chat): offer only resolvable decisions on collapsed tool rows (#5434)

[CrysisDeu]

Summary

CollapsibleToolGroup offered a Trust decision on its inline approval row (and could label a decision "Trusted"), while the resolve path behind both ChatPage mounts — toApiDecision into the one-shot api.resolveApproval — has no trust verb: a trust click was silently downgraded to a one-shot approve, claiming a standing grant the backend never records. Same class as #5400 / PR #5433 (spawn-approval card), third surface after #5202/#5203. Latent today (hasPermission={false} on both mounts), fixed while still unreachable.

Why not a blanket removal (divergence from #5433's shape, same principle): this component has one mount whose resolve path DOES honor trust — app-sdk ChatEmbed routes decisions verbatim through POST /api/chat/slots/{slot}/approve, a deliberate, test-pinned behavior ("sends Trust as trust, not as a plain approve"). Removing Trust outright would regress that live surface, and its ChatMessageList mock would keep vitest green while the real button vanished. So #5433's principle — offer only verbs the resolve path honors — is applied per mount:

• CollapsibleToolGroup gains a fail-closed canTrust prop; the Trust button renders only when the mount declares it.
• ChatMessageList threads canTrust (default withheld; added to the renderItem memo deps); ChatEmbed declares it.
• ChatPage's mounts stay one-shot: toApiDecision no longer maps a stray 'trust' to 'approve' (fails safe to reject) and carries a comment naming the constraint, as CollapsibleToolGroup wires the #5400 trust/one-shot mismatch, latent behind hasPermission #5434 suggests.

Testing

• CollapsibleToolGroupCov80.test.tsx: default mount offers exactly Approve/Reject (mutation-verified: re-adding an ungated Trust button fails it); canTrust mount offers Approve/Trust/Reject and reports trust verbatim.
• ChatMessageList.test.tsx: canTrust threads through and is withheld by default (mutation-verified: dropping the thread fails it).
• ChatEmbed.test.tsx: the embed declares canTrust (mutation-verified: dropping the declaration fails it) — pins the anti-regression for the one legitimate Trust surface.
• ChatPage.collapsedGroupTrust.test.tsx (new): source contract that neither ChatPage mount declares canTrust (mutation-verified). A render test is impossible by construction: groupDisplayItems skips permission rows entirely (the pinned ApprovalBar owns them), so no transcript can mount these groups — the same latency that made CollapsibleToolGroup wires the #5400 trust/one-shot mismatch, latent behind hasPermission #5434 an inspection finding.
• Local gates: npx tsc -b clean; full npx vitest run 23284 passed / 0 failed.
• Pre-push review lanes: GPT (gpt-5.6-sol) PASS, no findings. Opus (claude-opus-5) PASS, 0 blocking; adopted A1 (ChatPage mount pin) and A3 (catalog-key assertion), declined A2 (fail-closed default on the no-approval_id edge branch is deliberate), A4/A5 informational — filed as follow-up issues.

Screenshots

Why no screenshot: no reachable rendered state changes. Both ChatPage mounts pass hasPermission={false} (the approval row never renders there — verified unreachable by construction, see the new contract test), and ChatEmbed — the only production surface that renders the row — keeps its exact Approve/Trust/Reject set via canTrust.

Closes #5434

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 51e6937121d44050b97a50756d9aed34d4361bb5 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 51e6937

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 51e6937121d44050b97a50756d9aed34d4361bb5: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 51e6937121d44050b97a50756d9aed34d4361bb5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The description's claims check out: ChatEmbed routes decisions verbatim to the slot approve endpoint (which honors trust), both ChatPage mounts resolve through the one-shot resolveApproval (which doesn't), and toApiDecision has no other callers, so narrowing it is safe. The per-mount canTrust split is a justified divergence from the sibling fix's blanket removal. My review follows.

Design-Verdict: PASS

Fail-closed per-mount capability declaration correctly preserves the one surface whose resolve path honors trust; claims verified against the code.

Suggestions

• The source-scraping contract test exists only because ChatPage's mounts carry approval plumbing (permissionMeta/onApprove wiring) that can never render; deleting that unreachable wiring in a follow-up would remove the hazard class itself and retire the brittle source-text pin.

[DESIGN-REVIEWED] 51e6937

[github-actions]

UX Review (Fable 5) — ✅ PASS

UX-level review of 51e6937121d44050b97a50756d9aed34d4361bb5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

The Trust button now appears only where trusting actually persists — labels finally match the grant, and no reachable surface changes.

The diff removes a latent lie: CollapsibleToolGroup's "Trust"/"Trusted" affordance is now gated by fail-closed canTrust, so the one mount whose endpoint records standing trust (ChatEmbed) keeps the full Approve/Trust/Reject row, while ChatPage's one-shot mounts — unreachable today (hasPermission={false}, permission-only groups skipped) — can no longer promise a standing grant the backend never records. Tests pin both sides, including that a trust click on a canTrust mount reports verbatim and earns its "Trusted" label. No rendered UI changes for existing users; the guarded failure mode (Trust resolving as silent denial via the narrowed toApiDecision) is fenced off by the source-contract test.

[UX-REVIEWED] 51e6937

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 51e6937121d44050b97a50756d9aed34d4361bb5 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 51e6937

Verdict parsed from the review's SHA-scoped output markers for commit 51e6937121d44050b97a50756d9aed34d4361bb5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 51e6937121d44050b97a50756d9aed34d4361bb5: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 51e6937121d44050b97a50756d9aed34d4361bb5 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Composing the review now.

First-Principles-Verdict: CONCERNS

The per-mount canTrust gate earns its place, but the root cause — a trust verb silently downgraded on the one-shot path — has one counted unfixed sibling in ChatInput.

What this change ships

Intent: stop the collapsed tool row from offering a Trust that the main chat's resolve path silently records as a one-shot approve — a FIX.

1. Collapsed-row Trust button hidden everywhere except embedded app chat — justified
2. Embedded app chat keeps its Trust button via explicit canTrust — justified anti-regression
3. New fail-closed canTrust prop on group + message list — one setter (ChatEmbed), justified: unconditional removal regresses the one path that honors trust
4. Stray 'trust' on the main chat path now rejects instead of approving — declared, fail-safe
5. "Trusted" label unreachable on mounts that cannot grant trust — the fix's other half
6. Contract test also pins hasPermission={false} on ChatPage mounts — rides along, undeclared

Watch

• Point patch with 1 unfixed sibling of the same cause: ChatInput.tsx:163-164's own toApiDecision still maps 'trust'/'trust_reads' → 'approve' — reachable when a trust click lands with no activeSlot (the else at ChatInput.tsx:813), and ApprovalCard.tsx:49 then shows "Trusted — auto-approving future calls" for a grant the backend never recorded. Grepped toApiDecision: 2 definitions, this PR fixes only ChatPage's. The unattended downgrade at line 805 is documented; this fallback is not.

Subtractions

• Shrink ChatPage.collapsedGroupTrust.test.tsx's second assertion: drop expect(block).toContain('hasPermission={false}') — it freezes an attribute outside the trust contract, so legitimately arming the row later must unpin a test named for a different invariant. The canTrust assertion alone carries the CollapsibleToolGroup wires the #5400 trust/one-shot mismatch, latent behind hasPermission #5434 pin.

[FIRST-PRINCIPLES-REVIEWED] 51e6937

[CrysisDeu]

Disposition — First Principles (CONCERNS)

Watch: ChatInput's own toApiDecision still maps trust→approve — ACKNOWLEDGED, tracked.
Correct and confirmed independently by the pre-push Opus lane. Filed before this lane ran as #5486, including the reachability question (activeSlot null fallback at ChatInput.tsx:813) and the two candidate resolutions. Out of scope here by the same rule that scoped #5434 out of #5433: one surface per PR, each with its own reviewable blast radius. The PR body's Testing section declares the follow-up.

Subtraction: drop the hasPermission={false} assertion from the contract test — DECLINED, with the rationale on the record.
The assertion is not an unrelated attribute freeze: per #5434's own issue text, "the defect arms itself the moment either ChatPage site flips hasPermission truthy" — arming the row is the precise event that turns this latent defect class live, so it belongs to the same invariant the test pins. The failure an author hits when legitimately arming the row is the designed tripwire: the assertion's adjacent comment tells them exactly what to re-verify (the toApiDecision constraint) before deleting the line. Cost of keeping it: one deliberate test edit at arming time. Cost of dropping it: the arming event — the single moment #5434 warns about — passes with no forced checkpoint.

Item 6 "rides along, undeclared": fair on the PR body; the hasPermission pin is part of the contract test's declared purpose per the rationale above. Not editing the body post-green (a body edit re-triggers a full check round for prose only); this comment is the declaration.