Skip to content

feat(ai-operator): task admission route, Delegate to Operator action, Playwright approve-after-browser-close (W08 of #5205) - #5272

Merged
ToddHebebrand merged 5 commits into
mainfrom
feature/5205-ai-operator/wave-5246
Sep 8, 2026
Merged

feat(ai-operator): task admission route, Delegate to Operator action, Playwright approve-after-browser-close (W08 of #5205)#5272
ToddHebebrand merged 5 commits into
mainfrom
feature/5205-ai-operator/wave-5246

Conversation

@ToddHebebrand

Copy link
Copy Markdown
Collaborator

Closes #5246

Wave W08 (P3-1f) of #5205: the write that starts an AI Operator task, the action that fires it, and the browser proof that a delegated task outlives the session that created it.

What this adds

1. POST /api/v1/ai/operator/tasks (spec §12) — the only route that creates a task.

  • Body is .strict(): a request carrying task, policySnapshot or approval is rejected outright. Requests cannot supply a principal, policy, or approval result.
  • RBAC ai_agents:write plus the same MFA step-up the agent-run trigger uses.
  • Server-side: resolves the device, checks org + site access (404 on a cross-org or site-restricted device — never a 403 that confirms existence), recomputes recipe availability from the two flags (422 with an actionable reason), enforces the pending-task cap (429), then calls W06's admitServiceRecoveryTask with the requester's authorized ceiling.
  • 202 { taskId } once the task and its outbox row commit — Redis being down does not fail admission.
  • Duplicate clientIdempotencyKey returns the existing task id. Implemented as ON CONFLICT DO NOTHING on the partial unique index plus a read-back, not a caught 23505: a unique violation aborts the surrounding transaction, so the read-back could not run inside it.

2. "Delegate to Operator" on the device page and on alert detail.

  • Confirm dialog: recipe fixed to service recovery, service name required (prefilled from the alert where extractable), org/device read-only.
  • Posts through runAction; on 202 navigates to /operator/tasks/<id>.
  • Renders null entirely unless the aiOperatorTasks runtime flag is on. The alert variant targets the alert's own org, never the globally selected one (spec §5.1).
  • The idempotency key is minted once per opened dialog and kept stable across retries of that dialog, so a double-click dedupes into one task server-side.
  • data-testid="delegate-to-operator" / delegate-to-operator-confirm.

3. Playwright: ai-operator-approve-after-browser-close.spec.ts (acceptance scenario 3, spec §7.1 "authority across time"). Page Objects in pages/OperatorTaskPage.ts, data-testid selectors only.

4. docker-compose.yml + .env.example: both flags mapped explicitly and documented, default off (decision D2 — internal/test orgs only). Compose interpolates only what the environment: block names, so without the mapping the feature could not be switched on in any deployment and GET /config would report it disabled with no error anywhere.

What the Playwright spec actually proves, and what it does not

Real, driven through the browser: the button, the dialog, the admission POST (202 + the server's task id), the navigation to the task detail page and its render, an idempotent replay of the identical request resolving to the same task with no second row, the destruction of the creating browser context, a second independent login, and a real WebAuthn approve ceremony clicked in the approvals inbox — followed by a DB assertion that the task-linked intent settled approved.

Seeded, with the reason stated in the fixture header: the pending intent/approval the task waits on. Producing it for real needs a live LLM run (the investigate run must propose the restart and call createActionIntent) plus a coordinator tick. The e2e stack has neither. That transition is proven against real Postgres by aiOperatorServiceRecoveryE2E.integration.test.ts (W06). The task itself is never seeded — it must come from the real button, or the spec fails.

Contract I could not satisfy: the brief asks the flow to reach completed + verified_resolved with exactly one operation row. It cannot, honestly. That tail needs a connected agent to execute the restart and an independent device read to verify it; the e2e stack runs no agent — tests/script-cancel.spec.ts records the same limit in its own words ("the command this creates is never delivered and its device_commands row stays pending forever"). Faking the device from outside the API process would have proved nothing about W08. The execute→verify→document tail is covered by the W06 integration suite; this spec covers the session boundary that suite structurally cannot.

Two stack prerequisites these WebAuthn specs have are now documented in e2e-tests/README.md: PUBLIC_APP_URL must match the browser origin (the RP ID check fails otherwise, and it fails only these specs), and the enrolment ceremony rotates the refresh token, burning the session it runs in — hence exportCredentials/importCredentials in e2e-tests/webauthn.ts to carry the enrolled key from one context to the next. Approver-device registration has been grant-gated since #2707, so the helper mints a registerGrantId first; without one the options route answers 403 register_step_up_required.

Known gap, not fixed here (recorded per the brief)

deadline_at is enforced only inside advanceTask, which a paused task never re-enters — so a paused task does not expire on its own. The pause/resume/stop routes and that decision belong to P3-5.

Verification

All run locally on this branch after merging origin/main (3e56e27).

apps/api  npx vitest run src/routes/aiOperatorTasks.test.ts src/routes/config.test.ts \
                         src/db/autoMigrate.test.ts src/db/migrationRlsScope.test.ts
          → 4 files, 158 tests passed

apps/web  npx vitest run src/components/aiOperator src/stores/featuresStore.test.ts \
                         src/lib/__tests__/no-silent-mutations.test.ts \
                         src/lib/routeScope.test.ts src/lib/i18n
          → 15 files, 514 tests passed

apps/api  npx vitest run --config vitest.integration.config.ts \
            src/__tests__/integration/aiOperatorAdmission.integration.test.ts \
            src/__tests__/integration/rls-coverage \
            src/__tests__/integration/tenantCascade \
            src/__tests__/integration/tenant-export-policy
          → 7 files, 56 tests passed   (live Postgres)

apps/api  tsc --noEmit   → clean
apps/web  tsc --noEmit   → clean

scripts/check-migration-naming.sh --against-ref origin/main → OK

e2e-tests playwright test tests/ai-operator-approve-after-browser-close.spec.ts
          → 1 passed (16.5s), against a live worktree stack. Run three times
            in a row, green each time (the fixture cleans up its own prior
            rows, so it is genuinely rerunnable rather than one-shot).

The integration suite covers cross-org and site-restricted device → 404, flags off → 422, cap → 429, duplicate key → same id, the task row and outbox row present after 202, and a forged task field rejected.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YGPxXGTWpJKUNbEdR3TZEL

Todd Hebebrand and others added 5 commits September 8, 2026 04:23
#5205)

The only door that creates an Operator task. `ai_agents:write` + the MFA
step-up (same pair `POST /ai/agents/:id/runs` carries), explicit org from the
body checked against the caller's access, device resolved and site-gated with
the same non-enumerating 404 the W07 read routes use, readiness recomputed on
launch (422 with an actionable code), a 100-per-org pending cap (429), and
202 with the task id.

Idempotency is a DATABASE constraint, not a route check. A duplicate POST can
dispatch a second service restart to a customer machine, and a route-level
read-then-insert loses the race between two concurrent clicks. So
`ai_operator_tasks` gains a nullable `client_idempotency_key` and a PARTIAL
unique index on `(org_id, client_idempotency_key)`, and admission inserts with
`ON CONFLICT ... WHERE ... DO NOTHING` — `DO NOTHING` rather than catching
23505, because a unique violation aborts the transaction the read-back would
have to run in. A conflict then reads the winner, scoped by org AND key, and
answers 202 with that id: a replay is a success, not a conflict.

Threading the key required an additive change to W06's `admitServiceRecoveryTask`
(an optional input field and a `replayed` flag on its result). That crosses the
wave boundary deliberately: every alternative that left the file untouched put
the reservation and the task insert in different transactions, and the
coordinator can pick a task up before the route binds anything. Design was put
to an advisor quorum (this session + codex `xhigh`, read-only); both reached
the same answer independently.

Also: `features.aiOperatorTasks` on `GET /config` (AND of both flags, both
default off per decision D2) plus a default-CLOSED `useAiOperatorTasksGate()`
web hook, so the UI action is absent — not merely disabled — when the feature
is off or `/config` is unreachable.

Contract registration: the new column is classified in `CORE_TENANT_EXPORT_POLICY`
(a new COLUMN on an org-cascade table breaks that contract, which is the point).
No RLS change — shape 1, policies are column-agnostic.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YMbWgjQdqXzi98eJ5P3Uqp
…(W08 of #5205)

`DelegateToOperatorButton` — hidden entirely (not disabled) unless
`features.aiOperatorTasks` is on, because the gate defaults CLOSED: an
unreachable or older `/config` must never surface an action that starts
autonomous remediation on a customer machine.

The client idempotency key is minted ONCE per opened dialog and held in a ref,
so a double-click or a retry after a network blip reuses it and the server
returns the same task. It is re-minted only when the dialog is re-opened,
which is the one case where the operator really is asking for a second task.

Alert detail passes the alert's OWN orgId (newly declared on the web type; the
API always returned it), never the globally selected org — spec §5.1,
"changing global organization context while drafting cannot retarget the
task" — and cites the alert as the source so the verification criterion has a
recurrence signal. Without one, W06's criterion can only reach
`investigation_complete`, never `verified_resolved`.

`extractServiceNameFromAlert` prefills the service field from the alert's
prose, since no structured service name exists on an alert row. It returns
null rather than guessing, and the field stays required and editable.

The device-page button has no `online` gate: a task is durable work with its
own deadline, not an immediate command.

Compose: both operator flags are mapped explicitly in `x-api-env`. Compose
interpolates only what that block names, so a value in `.env` alone never
reaches the container — without these the feature could not be switched on in
any deployment and `/config` would report it disabled with no error anywhere.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YMbWgjQdqXzi98eJ5P3Uqp
…#5205)

Acceptance scenario 3 / spec §7.1 "authority across time": a technician
delegates a service-recovery task from a device page, closes the browser, and
the approval the operator is waiting on is still decidable from a different
browser session later.

What the spec really drives: the Delegate to Operator button and dialog, the
POST /ai/operator/tasks admission (202 + the server's own task id), the
navigation to /operator/tasks/<id> and that page's render, an idempotent replay
of the same client key resolving to the same task with no second row, the
destruction of the creating browser context, a second independent login, and a
real WebAuthn approve ceremony clicked in the approvals inbox.

What is seeded, and why: the pending intent/approval the task waits on
(seed-operator-task-approval.sql). Reaching it for real needs a live LLM run
plus a coordinator tick; the e2e stack has neither. That transition is already
proven against real Postgres by aiOperatorServiceRecoveryE2E.integration.test.ts.
The task itself is NEVER seeded — it must come from the real button.

What the spec deliberately cannot reach: completed + verified_resolved. That
needs a connected agent to execute the restart and an independent device read
to verify it, and the e2e stack runs no agent (tests/script-cancel.spec.ts
records the same limit). Faking the device would prove nothing about W08.

Also:
- e2e-tests/webauthn.ts — virtual-authenticator + approver-device enrolment
  helpers, including credential export/import so a key enrolled in one browser
  context can sign in the next. Registration is grant-gated since #2707, so it
  mints a registerGrantId first; without one the options route 403s.
- README: two stack prerequisites these WebAuthn specs have (PUBLIC_APP_URL
  must match the browser origin or the RP ID check fails, and the enrolment
  ceremony rotates the refresh token and burns the session it runs in).
- .env.example: document the two AI Operator flags, both default off (D2).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YGPxXGTWpJKUNbEdR3TZEL
…alert call site, drop an unregistered eslint-disable (W08 of #5205)

Three review findings from the PR #5272 pass.

1. Silent-failure review (HIGH): the ON CONFLICT read-back miss in
   admitServiceRecoveryTask returned refusal 'invalid_input', which the route
   turns into a 422 reading to the technician as "your input was wrong" — with
   nothing logged anywhere. Nothing in a caller's request can cause it (index
   and read-back are both org+key scoped), so it is a broken invariant: a
   concurrent erasure racing admission, or a future change desynchronising the
   index from the lookup. Now throws, matching operationService.ts's
   dispatch-claim cardinality check, so it surfaces as a 500 the top-level
   handler logs instead of a consistency break with no breadcrumb. The
   caller-supplied key is not logged.

2. Test-coverage review: nothing proved the alert call site passes the ALERT's
   own orgId rather than the globally selected one — a swap would compile, pass
   every existing test, and aim a live remediation action at the wrong tenant.
   AlertDetailPage.delegateToOperator.test.tsx now asserts it with the org
   store deliberately seeded to a DIFFERENT id, plus the status gating
   (rendered for active/acknowledged, hidden for resolved/dismissed).
   Red-first verified: reverting the wiring to the store value fails the test
   with `expected 'org-globally-selected-9999' to be 'org-alert-owner-1111'`.

3. Lint: `// eslint-disable-next-line react-hooks/exhaustive-deps` was itself
   the lint error — that rule is not registered in this repo's config. Removed;
   the reason the dep is excluded stays as a plain comment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YGPxXGTWpJKUNbEdR3TZEL
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 8, 2026

Copy link
Copy Markdown

Deploying breeze with  Cloudflare Pages  Cloudflare Pages

Latest commit: 510bc88
Status: ✅  Deploy successful!
Preview URL: https://a16c8a25.breeze-9te.pages.dev
Branch Preview URL: https://feature-5205-ai-operator-wav-nzxz.breeze-9te.pages.dev

View logs

@ToddHebebrand

Copy link
Copy Markdown
Collaborator Author

PR review summary — /pr-review-toolkit:review-pr

Three specialist reviewers ran against gh pr diff 5272 (code-reviewer, silent-failure-hunter, pr-test-analyzer). Three findings, all fixed inline in 510bc88 — nothing deferred.

Fixed

1. HIGH (silent-failure-hunter) — a broken invariant was being reported as the caller's mistake, with no breadcrumb.
taskService.ts — the ON CONFLICT DO NOTHING read-back miss returned refusal: 'invalid_input', which the route turns into a 422 that reads to a technician as "your service name was wrong". Nothing in a caller's request can produce it (the index and the read-back are both org+key scoped), so it is a genuine consistency break — a concurrent erasure racing admission, or a future change desynchronising the index from the lookup — and it was landing in no log and no Sentry event. Now throws, matching operationService.ts's dispatch-claim cardinality check, so it surfaces as a 500 the top-level handler records. The caller-supplied key is deliberately not logged.

2. IMPORTANT (pr-test-analyzer) — the alert call site had no guard on which org it targets.
AlertDetailPage.tsx passes orgId={alert.orgId} precisely because spec §5.1 requires that changing the global org context while drafting cannot retarget the task. Nothing tested it: swapping it for useOrgStore's selected org would compile, pass every existing test, and aim a live remediation action at the wrong tenant. Added AlertDetailPage.delegateToOperator.test.tsx (5 tests) with the org store seeded to a deliberately different id, plus the status gating (rendered for active/acknowledged, hidden for resolved/dismissed).
Red-first verified, not assumed — reverting the wiring to the store value fails with:

AssertionError: expected 'org-globally-selected-9999' to be 'org-alert-owner-1111'

3. Lint (CI) — // eslint-disable-next-line react-hooks/exhaustive-deps was itself the lint error; that rule is not registered in this repo's config. Removed, reason kept as a plain comment.

Accepted as-is, with reasons

  • code-reviewer: no findings. It independently verified the RBAC→MFA ordering against the POST /ai/agents/:id/runs precedent, the cross-org 404-not-403 behaviour under real RLS, the partial-index conflict target repeating its WHERE predicate (the 42P10 trap), the NOT IN cap predicate expanding correctly in drizzle 0.45.2, the .strict() rejections, the runAction catch pattern, and the export-policy registration for the new column.
  • Soft cap race (code-reviewer, below its own confidence bar): the pending-cap count and the insert are separate round-trips, so a concurrent burst could land a few tasks past 100. Left alone deliberately — it is a user-recoverable 429 usage limit, not a tenancy or security invariant, and spec §7.2 asks for "a visible capacity result when full", not exact enforcement.
  • Agent-selection tie-break untested (pr-test-analyzer, rating 4): org-owned before partner-wide, then oldest-first. Real but low-value to pin now; it affects which of several enabled agents runs, not tenancy.
  • No standalone validator test for createOperatorTaskSchema (rating 3): its behaviour is covered through the route's HTTP-level it.each and again against live Postgres. Matches the file's existing convention.

Noted as genuinely strong

Both reviewers that looked for vacuous assertions found none in the places this repo has been burned before. The idempotent-replay tests assert row counts after the replay, not just replayed: true; the concurrent-POST test proves the database constraint serialises the race rather than app-level dedup; the cross-org 404 is asserted against a real device in a real second org under breeze_app; the migration's partial-index shape is asserted directly against pg_indexes; and the Playwright spec checks state, client_idempotency_key, requester_user_id and the approval→intent linkage in Postgres rather than trusting a success toast.

Verification after the fixes

aiOperatorTasks.test.ts 42 ✓ · AlertDetailPage* 14 ✓ · aiOperatorAdmission.integration.test.ts 17 ✓ (live PG) · tsc --noEmit clean in both apps · pnpm lint clean · migration-naming guard OK.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YGPxXGTWpJKUNbEdR3TZEL

@ToddHebebrand
ToddHebebrand added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 3837f4f Sep 8, 2026
72 checks passed
@ToddHebebrand
ToddHebebrand deleted the feature/5205-ai-operator/wave-5246 branch September 8, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

W08: P3-1f Delegate to Operator action on device and alert, task admission wiring, Playwright approve-after-browser-close flow (after W06)

1 participant