fix(approvals): drop stuck pending permission_request messages (#723)

* fix(web): exclude permission_request from turn-complete safety net

The session.turn.completed handler iterated over messages and forced
metadata.status="complete" on anything carrying tool_call_id. Permission
prompts also carry tool_call_id (for FE pairing with their tool_call),
so this loop overwrote the user's "approved" decision on every prompt
in the turn — and the FE component re-rendered the Approve/Deny buttons.

* fix(backend): protect permission_request rows from tool_call sweeps

Three permission_request invariants the prior code violated, mirroring
the FE bug fix:

- CompletePendingToolCallsForTurn: SQL UPDATE filtered by tool_call_id
  presence, also catching permission_request rows. Add type filter so
  the safety net only touches actual tool messages.
- GetMessageByToolCallID: returned the oldest row carrying that
  tool_call_id, which races to the permission_request when timing
  shifts. Exclude permission_request type so a tool_update can never
  land on the user's approve/reject row.
- applyToolCallMessageUpdate: defensive guard that refuses to overwrite
  a permission_request, making the invariant explicit at the layer that
  does the writing.

Adds a regression test for CompletePendingToolCallsForTurn covering
approved/rejected/pending permission rows.

* test(e2e): cover approval-reappear regression with mock-agent scenario

Adds a multi-permission mock-agent scenario (three sequential bash
tools, each requiring approval) and an E2E test that approves all
three and asserts no permission action row remains visible after
the agent's turn ends, and that the resolved state survives a reload.

Supporting test infrastructure:
- data-testid hooks on PermissionActionRow.
- permissionActionRows() / permissionApproveButtons() on SessionPage.
- backend fixture force-disables AGENTCTL_AUTO_APPROVE_PERMISSIONS=false
  so a developer's local AUTO_APPROVE=true env doesn't silently bypass
  the permission UI in tests that exercise the approve/reject flow.

* test(backend): cover GetMessageByToolCallID permission_request exclusion

Addresses claude-review feedback on the approvals fix:
- Adds a regression test that seeds both a permission_request and a
  tool_call with the same tool_call_id (permission_request created
  first, so an ORDER BY created_at ASC sweep without the type filter
  would race to it) and asserts the lookup returns the tool_call row.
- Bumps the applyToolCallMessageUpdate invariant guard from Warn to
  Error: reaching that branch means a caller bug, not a transient
  condition, and Warn would silently swallow the violation.

* fix(e2e): scope auto-approve override to permission-approval test only

The previous fixture force-disabled AGENTCTL_AUTO_APPROVE_PERMISSIONS
globally, which broke session-isolation.spec.ts and other tests that
use scenarios with permission-required tools (e.g. /e2e:read-and-edit)
and rely on auto-approve to let the agent finish without UI clicks.

Default the env to "true" in the fixture; let permission-approval.spec
flip it to "false" via a backend.restart() override in beforeAll, then
restore in afterAll. Adds an envOverrides parameter to backend.restart.
This commit is contained in:
José Almeida
2026-05-01 00:25:51 +01:00
committed by GitHub
parent 45a4f0b016
commit fcc7ceb8b6
9 changed files with 274 additions and 7 deletions
@@ -17,7 +17,10 @@ export const PermissionActionRow = memo(function PermissionActionRow({
isResponding = false,
}: PermissionActionRowProps) {
return (
<div className="flex items-center gap-2 px-3 py-2 rounded-sm bg-amber-500/10">
<div
className="flex items-center gap-2 px-3 py-2 rounded-sm bg-amber-500/10"
data-testid="permission-action-row"
>
<span className="text-xs text-amber-600 dark:text-amber-400 flex-1">
Approve this action?
</span>
@@ -26,6 +29,7 @@ export const PermissionActionRow = memo(function PermissionActionRow({
variant="outline"
onClick={onReject}
disabled={isResponding}
data-testid="permission-reject"
className="h-6 px-3 text-foreground border-border bg-background hover:bg-muted hover:border-foreground/40 transition-colors cursor-pointer"
>
<IconX className="h-4 w-4 mr-1 text-red-500" />
@@ -36,6 +40,7 @@ export const PermissionActionRow = memo(function PermissionActionRow({
variant="outline"
onClick={onApprove}
disabled={isResponding}
data-testid="permission-approve"
className="h-6 px-3 text-foreground border-border bg-background hover:bg-muted hover:border-foreground/40 transition-colors cursor-pointer"
>
{isResponding ? (