From 82af256d0cde28730bcb17e1eba48712d70d8276 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 16:28:57 +0000 Subject: [PATCH 1/5] docs: add competitive landscape analysis, starting with Littlebird MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Littlebird (littlebird.ai) reads as a head-on competitor and isn't one. It is an ambient context layer — always-on macOS observation of structured window text, meeting transcription, local work memory, and scheduled recurring prompts — positioned as giving better answers about your work rather than doing the work. It has no approval gate, no per-step verification, no audit chain, and no multi-tenant model. Records the three real threats, which are not customer overlap: narrative capture of the "AI that knows your work" story, ownership of the layer above Ghost's Phase 2 capture problem, and asymmetric SOC 2 urgency (a badge for a $17/mo app, a procurement blocker for Ghost). Lists the tripwires that would make it a direct competitor. Adopt/refuse framed as in cloud/docs/PRIOR_ART.md. The load-bearing finding: a notification surface currently blocks three separately deferred items (multiple approvers, Slack/email approval, scheduled runs), and scheduled recurring execution is now a competitive gap rather than backlog. Refuses ambient capture outright — it violates the "no monitoring the customer hasn't asked for" trust boundary. Also notes that the legacy desktop tree already built and deprioritized this wedge (core/atlas.rs, observer mode, core/ocr.rs). Recorded as evidence the wedge is fundable, explicitly not as a reason to return to it. Sourcing is secondary throughout: littlebird.ai and several review sites are blocked by the network egress proxy, so the doc carries a caveat to re-verify before any of it reaches a pitch or roadmap. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1 --- docs/README.md | 1 + docs/competitive-landscape.md | 233 ++++++++++++++++++++++++++++++++++ 2 files changed, 234 insertions(+) create mode 100644 docs/competitive-landscape.md diff --git a/docs/README.md b/docs/README.md index 36988ea..0717290 100644 --- a/docs/README.md +++ b/docs/README.md @@ -21,6 +21,7 @@ If a doc contradicts this index or `cloud/README.md`, treat the cloud docs as tr | [`product-direction.md`](product-direction.md) | Category, ICP, non-goals | | [`business-model.md`](business-model.md) | How we make money | | [`audiences.md`](audiences.md) | Who we sell to first | +| [`competitive-landscape.md`](competitive-landscape.md) | Commercial competitors — who's adjacent, what to adopt and refuse | | [`trust-pipeline.md`](trust-pipeline.md) | Approval / verify / audit principles | | [`why-deterministic-gates.md`](why-deterministic-gates.md) | Why the gate is a pure function, not a model call — and what the audit chain does not prove | | [`../CONTRIBUTING.md`](../CONTRIBUTING.md) | Setup, the two env traps, trust invariants | diff --git a/docs/competitive-landscape.md b/docs/competitive-landscape.md new file mode 100644 index 0000000..8f5c859 --- /dev/null +++ b/docs/competitive-landscape.md @@ -0,0 +1,233 @@ +# Competitive landscape + +Ghost sells **governed execution**: a human-approved, verified, audited pipeline over +software that was never designed to be automated. That is a narrow category, and the +products it gets compared to mostly sit in *adjacent* categories — context layers, +assistants, RPA, and workflow orchestrators. + +`cloud/docs/PRIOR_ART.md` covers the open-source orchestrators (Temporal, Camunda, +Windmill, n8n, …) as engineering prior art. This document covers **commercial +competitors** — who a buyer might put next to Ghost on a shortlist, and what to take +or refuse from each. + +The map: + +| Category | Example | Relationship to Ghost | +|---|---|---| +| **Ambient context layer** | [Littlebird](#littlebird) | Adjacent above — analyzed below | +| Governed execution / trust runtime | *(no named direct competitor yet)* | Ghost's category | +| Agent frameworks | Claude, Cursor, Codex | **Clients**, not competitors — they propose, Ghost gates | +| No-code automation | Zapier, Make, n8n Cloud | Connectors without a trust pipeline — explicit non-goal | +| Enterprise RPA | UiPath, Automation Anywhere | Same problem, coordinate-first, no approval-gate story | +| Screen-recall / memory | Rewind, Windows Recall | Same layer as Littlebird, weaker on privacy stance | + +Only Littlebird is analyzed in depth so far. The rest are named to keep the map honest, +not because a survey of each has been done. + +--- + +## Littlebird + +> **Sourcing caveat.** `littlebird.ai` is blocked by this environment's network egress +> proxy, so nothing here is quoted from the vendor's own site. Everything below comes +> from press coverage, review sites, and search summaries (see [Sources](#sources)). +> Treat feature specifics as **secondary-source**, and re-verify against the product +> before any of this reaches a pitch, a comparison page, or a roadmap commitment. + +### What it actually is + +A macOS-native, always-on assistant that builds a **private memory of your work**. It +reads *structured text* from the active window across every app you have open — not +screenshots — transcribes your meetings, and stores that context as local text on the +Mac. You then ask it questions, and it answers with the context already in hand. +"Routines" run a saved prompt on a recurring schedule (daily briefing, weekly activity +summary). Connectors to email and calendar pull in further context and, per one review, +can "take actions on your behalf." + +The company: founded 2024 by Alap Shah, Naman Shah, and Alexander Green — the Shah +brothers previously founded Sentieo (sold to AlphaSense). **$11M seed**, announced March +2026. Product Hunt #1 product of the day. **SOC 2 certified**, GDPR/CCPA compliant, +AES-256 at rest, TLS 1.3 in transit, no training on user data. + +Pricing: **free** Basic tier; Plus at **$17/mo** annual or $20 monthly; higher tiers +reported up to ~$100/mo; 14-day trial on paid plans. + +### The finding: this is not a head-on competitor + +Littlebird is a **context layer**, not an execution platform. The clearest statement of +this comes from a competitor's own comparison page, which is where positioning is +usually least flattering and most accurate: Littlebird "is a smart observer on your +machine; unlike agents … that do the work itself, Littlebird gives you better answers +about your work." Another review puts it as "the context layer that feeds into your +other tools." + +Set against Ghost's pipeline, the overlap is thin and the gap is structural: + +| Capability | Littlebird | Ghost Cloud | +|---|---|---| +| Ambient cross-app context capture | **Yes** — all macOS apps, always on | No — and refused, see below | +| Meeting transcription | **Yes** | No | +| Persistent work memory + Q&A | **Yes**, local | No | +| Scheduled recurring prompts | **Yes** ("Routines") | **No trigger surface at all** | +| Records a demonstrated workflow | Observes, doesn't compile to steps | Yes — Phase 2 Chrome extension → typed steps | +| Executes multi-step work across apps | Limited, via email/calendar connectors | **Yes** — browser + API, typed steps | +| Deny-by-default gate on send/pay/delete | Not evidenced | **Yes** — deterministic `classifyStep` | +| Human approval before sensitive actions | Not evidenced | **Yes**, with expiry, single-use | +| Per-step outcome verification | No | **Yes** | +| Hash-chained audit log | No | **Yes**, two-level | +| Incidents / undo / durable resume | No | **Yes** (undo designed, not built) | +| Multi-tenant org isolation, RBAC | Single-user Mac app | **Yes** | +| SOC 2 | **Yes** | **No** | +| Self-serve free tier | **Yes** | **No** — implementation-led | +| Platform | macOS only | Cloud (browser + API) | + +Ghost wins every row that describes *doing the work under controls*. Littlebird wins +every row that describes *knowing about the work* — plus, notably, the two rows that are +about being a real company you can buy from today: SOC 2 and a free tier. + +They are also not competing for the same dollar. $17/mo personal productivity versus +$500–2,000/mo team plus $2.5k–15k implementation are different budgets, different +buyers, and different sales motions. + +### So what is the actual threat? + +Three of them, and none is "Littlebird takes Ghost's customers." + +**1. Narrative capture.** Littlebird has $11M, a shipping consumer product, a #1 Product +Hunt launch, and press on the story "AI that already knows what you're working on." +Ghost has a stronger engine and no self-serve on-ramp. The risk is that Littlebird +defines the category in the buyer's head first, and Ghost gets evaluated as *"Littlebird +but harder to set up"* — judged on a rubric written by a product that doesn't do +governed execution and doesn't need to. + +**2. They own the layer above Ghost's hardest unsolved problem.** Ghost's Phase 2 answer +to "which workflow should we automate?" is: record one in Chrome, compile it to typed +steps. Littlebird already watches *every app on the machine, continuously*. It is much +better positioned to know which work is repetitive — because it sees all of it, and +Ghost sees only what someone chose to record in a browser tab. If Littlebird adds +execution, it descends into Ghost's layer carrying context Ghost cannot get. + +**3. Asymmetric compliance urgency.** Littlebird has SOC 2 as a $17/mo consumer app, +where it's a nice-to-have. Ghost sells approval-gated audit trails to regulated ops +teams — bookkeeping, healthcare admin, financial ops — where SOC 2 is a **procurement +blocker**, and Ghost doesn't have it. A product whose entire pitch is auditability being +outrun on third-party attestation by a personal-productivity app is a positioning +problem, not just a checklist gap. + +### The tripwires + +Littlebird becomes a direct competitor the moment it does any of these. Each is a step +down into Ghost's layer: + +1. **Write-execution beyond email/calendar** — especially browser automation. +2. **A team/org tier** with shared memory and admin controls. +3. **An approval concept** — any "confirm before it sends" gate. +4. **Beyond macOS**, i.e. server-side or cross-platform execution. +5. **Compiling observed activity into a reusable, editable routine** rather than a + recurring prompt. This is the one to watch hardest — it is Ghost's Phase 2, reached + from ambient capture instead of from an extension. + +If none of these ship, Littlebird stays a complement — plausibly even a context source +Ghost consumes. + +### An uncomfortable note for the record + +Ghost's legacy desktop app **already built Littlebird's wedge and deprioritized it**. +`src-tauri/src/core/atlas.rs` + `storage/atlas.rs` are a local, offline semantic-memory +graph over the user's work. `commands/experimental.rs` includes observer mode. +`core/ocr.rs` does on-device OCR with no network. Local-first, no-cloud, never-delete. + +Littlebird raised $11M taking approximately that wedge to market as a $17/mo macOS app +with meeting notes and a good privacy story. + +The correct reading is **not** "go back to the desktop app." The wedge was abandoned for +sound reasons recorded in `CLAUDE.md`: the local-first delivery model doesn't carry +forward, and ambient observation is against Ghost's own trust boundary. The correct +reading is narrower and more useful: **the wedge is fundable, and the market rewards +products that visibly know the user's work.** Ghost's version of that has to be built +out of run history, not surveillance. + +--- + +## What Ghost should adopt + +Framed the way `PRIOR_ART.md` frames its steals — the refusals matter as much. + +**Adopt: a trigger surface, and treat it as competitive, not backlog.** +Littlebird ships scheduled recurring execution at $17/mo. `PRIOR_ART.md` lists "Triggers +and schedules" as deferred with the note *"no trigger surface exists at all."* That +reads differently now. It also has a genuine product blocker recorded alongside it — +scheduling *unattended* runs of an approval-gated workflow needs an answer for who gets +notified — which leads directly to the next item. + +**Adopt: a notification surface. This is the highest-leverage unblock in the repo.** +It currently blocks three separately-deferred things: multiple approvers, Slack/email +approval routing (both from Windmill), and scheduled runs. One piece of infrastructure, +three features, and one of them is now a competitive gap. Build this first. + +**Adopt: workflow intelligence from run history, not from observation.** +Littlebird's real insight is that a product feels indispensable when it already knows +your work. Ghost can have a scoped version of this without watching anything: it already +stores every run, every step, every approval, every verification. It should be able to +tell a customer *"this workflow ran 43 times last month, 91% of its steps never needed +approval, and step 7 has failed 4 times"* — which is a better automation-ROI argument +than ambient capture can make, because it's measured on work Ghost actually executed. +No new capture surface, no new trust boundary, uses data that exists. + +**Adopt: state the screenshot stance as loudly as they state theirs.** +Littlebird made "we don't take screenshots" a headline privacy claim. Ghost **does** +capture per-step screenshots — and that's defensible: they are evidence for an auditor, +scoped to a run a human triggered, on a workflow a human approved, not ambient recording +of a person's day. That distinction is real and currently under-stated. Rule 10 forbids +overclaiming; it does not forbid claiming what is true. Write it down in +`trust-pipeline.md` with the same clarity Littlebird writes theirs. + +**Adopt: SOC 2, with urgency.** See the asymmetry above. For Ghost's buyer this is a +blocker, not a badge. + +**Adopt: one self-serve path that completes.** Not a free tier, and not a reversal of +the implementation-led model in `business-model.md` — that ordering is deliberate. But +"$2,500 minimum before you see it work" loses every evaluation where the buyer wants to +try before they talk. One templated workflow a prospect can run end-to-end — gate, +approve, verify, audit log, all of it — is the demo the engine has earned and doesn't +currently have a front door for. + +## What Ghost should refuse + +**Always-on ambient capture across all applications.** This violates a stated trust +boundary in `CLAUDE.md` — *"No monitoring the customer hasn't asked for"* — and it is a +different product with a different buyer. Ghost's capture is scoped, per-workflow, and +initiated by a human who is demonstrating a task on purpose. Do not blur that to match a +competitor's feature list. + +**A local-first / on-device trust story.** Already decided and documented. Ghost's trust +story is least privilege + approval + verification + tamper-evident audit. Littlebird's +strong local-privacy story is an argument for stating Ghost's story better, not for +changing it. + +**Meeting transcription, personal memory, and "ask me about your day."** Adjacent +product, adjacent buyer, and `product-direction.md` already lists "not a chatbot or 'AI +coworker'" as a non-goal. + +**macOS-native as the primary surface.** Ghost executes against browsers and APIs on +servers. Desktop automation stays where the strategy puts it: after browser, before +vision. + +--- + +## Sources + +- [Littlebird raises $11 Million — PR Newswire](https://www.prnewswire.com/news-releases/littlebird-raises-11-million-to-launch-the-only-ai-that-already-knows-what-youre-working-on-302721664.html) +- [Littlebird raises $11M for its AI-assisted 'recall' tool — TechCrunch](https://techcrunch.com/2026/03/23/littlebird-raises-11m-to-capture-context-from-your-computer-so-you-can-query-your-data/) *(egress-blocked; cited from search summary)* +- [Littlebird — Product Hunt](https://www.producthunt.com/products/littlebird) +- [Littlebird pricing](https://littlebird.ai/pricing) *(egress-blocked)* +- [Littlebird Review 2026 — Efficient App](https://efficient.app/apps/littlebird) *(egress-blocked; cited from search summary)* +- [Littlebird Review — Agent Finder](https://agent-finder.co/reviews/littlebird) +- [6 Best Littlebird Alternatives — Carly](https://www.usecarly.com/blog/littlebird-alternatives/) — source of the "smart observer … gives you better answers about your work" characterization +- [Littlebird AI Review — Toolworthy](https://www.toolworthy.ai/tool/littlebird-ai) +- [Littlebird AI — App Store](https://apps.apple.com/us/app/littlebird-ai/id6737920045) + +Internal: [`product-direction.md`](product-direction.md) · +[`business-model.md`](business-model.md) · [`trust-pipeline.md`](trust-pipeline.md) · +[`../cloud/docs/PRIOR_ART.md`](../cloud/docs/PRIOR_ART.md) · +[`../cloud/docs/CURSOR_HANDOFF.md`](../cloud/docs/CURSOR_HANDOFF.md) From b3d0dff0c073baf78f0a3eb3f872296c03fa6365 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 17:22:44 +0000 Subject: [PATCH 2/5] feat(cloud): route exceptions to a desk instead of parking runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An incident was a state, not a product. A run that stopped was reachable only by knowing its id and opening it — fine when you triggered it thirty seconds ago, useless as the way an ops team finds the twelve things that broke overnight. This makes the 15% that needs a human findable, owned, and classified. Adds a third deterministic classifier alongside sensitive.ts and replay.ts. Where classifyStep asks "must a human approve this?" and replaySafety asks "is it safe to re-apply silently?", classifyException asks "this stopped — whose desk, and is retrying safe?" Nine kinds (TRANSIENT, TARGET_MISSING, AUTH, VERIFICATION, OUTCOME_UNKNOWN, APPROVAL_EXPIRED, RESTORE_UNSAFE, DATA, UNKNOWN) each map to an owner: operator, author, or administrator. That mapping is the routing decision — a changed selector belongs to whoever maintains the workflow, an expired credential to an administrator. Two signal tiers, ordered. Ghost's own structured prefixes and the recorded step outcome are authoritative; raw Playwright text is best-effort and only ever refines an otherwise-UNKNOWN verdict. An unrecognized reason stays UNKNOWN rather than being forced into the nearest bucket, because a wrong-but-confident label defeats the purpose. Rule ordering is load-bearing and pinned by a test: a locator timeout contains "Timeout", so TARGET_MISSING must be tested before the generic timeout rule or every changed selector is misfiled as transient and retried forever. The one part with teeth: retryMayDuplicate. OUTCOME_UNKNOWN means the step may already have taken effect, and the engine deliberately lets a human retry it anyway (journal.ts, on clearing inFlight) — only a person can check the target system. But that was a one-click button sitting next to a paragraph of prose, indistinguishable from retrying a network blip, and nothing recorded that the warning was seen. The route now refuses an unacknowledged risky retry with a 409 and requires acknowledgeDuplicateRisk, recording it in the run journal and the audit log. No new prohibition — the same retry still happens for any caller that says it understands. It just cannot happen by accident, and the decision is attributable. Duplicate risk is a conjunction, not a synonym for the kind: it composes replaySafety, so an indeterminate `verify` (a read, free to repeat) carries no warning while an indeterminate `click` does. A warning that fires on reads gets clicked through when it matters. Also adds the queue itself: GET /api/exceptions and an /exceptions page grouped by owner and sorted oldest-first (a parked run goes stale, it does not improve with age), plus assignment. Assignment is open to any member — deciding who looks at a problem is not authorizing the action that caused it — and is tenant-isolated: an assignee must be a member of the org that owns the run, or any user id would be accepted and leak account existence across tenants. Routing fields clear when the run leaves INCIDENT so a resolved exception cannot linger in the queue. Classification happens when the incident is raised, not on read: the queue then filters in SQL, and Run.error can be overwritten by a later failure, so a disposition computed on read might not be the one the run actually stopped on. Validation: pnpm typecheck, lint, build all clean; 515 tests pass across 58 files against real Postgres + Redis, so the ~90 DB-gated tests actually ran rather than skipping. Migration applies from scratch on a fresh database with `prisma migrate diff` reporting no drift. The repo's own middleware test caught a real bug mid-build — /exceptions was added as a page without an auth matcher entry, so it would have rendered unauthenticated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1 --- .../web/src/app/(app)/exceptions/page.tsx | 261 ++++++++++++++ .../apps/web/src/app/api/exceptions/route.ts | 203 +++++++++++ .../[id]/incident/incident-routing.test.ts | 326 ++++++++++++++++++ .../src/app/api/runs/[id]/incident/route.ts | 122 ++++++- cloud/apps/web/src/components/app-sidebar.tsx | 3 +- .../web/src/components/exception-assignee.tsx | 84 +++++ .../apps/web/src/components/run-timeline.tsx | 115 ++++-- cloud/apps/web/src/lib/run-view.ts | 47 +++ cloud/apps/web/src/middleware.ts | 1 + cloud/apps/worker/src/jobs/runWorkflow.ts | 66 +++- cloud/packages/core/package.json | 1 + .../migration.sql | 22 ++ cloud/packages/core/prisma/schema.prisma | 24 ++ .../core/src/classifier/exception.test.ts | 224 ++++++++++++ .../packages/core/src/classifier/exception.ts | 305 ++++++++++++++++ 15 files changed, 1767 insertions(+), 37 deletions(-) create mode 100644 cloud/apps/web/src/app/(app)/exceptions/page.tsx create mode 100644 cloud/apps/web/src/app/api/exceptions/route.ts create mode 100644 cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts create mode 100644 cloud/apps/web/src/components/exception-assignee.tsx create mode 100644 cloud/packages/core/prisma/migrations/20260817170000_run_exception_routing/migration.sql create mode 100644 cloud/packages/core/src/classifier/exception.test.ts create mode 100644 cloud/packages/core/src/classifier/exception.ts diff --git a/cloud/apps/web/src/app/(app)/exceptions/page.tsx b/cloud/apps/web/src/app/(app)/exceptions/page.tsx new file mode 100644 index 0000000..76d8889 --- /dev/null +++ b/cloud/apps/web/src/app/(app)/exceptions/page.tsx @@ -0,0 +1,261 @@ +import Link from "next/link"; +import { auth } from "@/auth"; +import { prisma } from "@/lib/db"; +import { Card, CardBody } from "@/components/ui/card"; +import { parseWorkflowSteps } from "@ghost/core/schema/step"; +import { + classifyException, + type ExceptionKind, + type ExceptionOwner, +} from "@ghost/core/classifier/exception"; +import { ExceptionAssignee } from "@/components/exception-assignee"; + +export const dynamic = "force-dynamic"; + +/** + * The exception queue — the work an ops team actually does. + * + * Ghost's promise is that it involves a human only when necessary. That promise + * is only kept if the necessary involvement is *findable*: before this page an + * incident could only be reached by knowing a run's id, which meant the failure + * mode of a 40-invoice batch was twelve parked runs nobody knew about. + * + * Grouped by owner rather than by workflow or time, because the owner is the + * routing decision: a changed selector is the workflow author's problem, an + * expired credential is an administrator's, and a rejected value is the + * operator's. Sorting by anything else makes every reader re-triage the list. + */ + +const OWNER_COPY: Record = { + operator: { + title: "For an operator", + blurb: + "Needs a judgement call about the work itself, or a look at the target system.", + }, + author: { + title: "For the workflow author", + blurb: + "The workflow no longer matches the software it drives. Retrying will fail the same way.", + }, + administrator: { + title: "For an administrator", + blurb: + "Credentials, permissions, or approvals need attention before these can proceed.", + }, +}; + +const OWNER_ORDER: ExceptionOwner[] = ["operator", "author", "administrator"]; + +function kindTone(kind: ExceptionKind): string { + // OUTCOME_UNKNOWN is the only kind that carries a risk of repeating an effect, + // so it is the only one styled as a danger rather than a warning. If + // everything is red, the one that matters stops standing out. + if (kind === "OUTCOME_UNKNOWN") return "text-[var(--color-danger)]"; + if (kind === "TARGET_MISSING" || kind === "AUTH") + return "text-[var(--color-warning)]"; + return "text-[var(--color-muted)]"; +} + +function age(from: Date): string { + const mins = Math.floor((Date.now() - from.getTime()) / 60_000); + if (mins < 1) return "just now"; + if (mins < 60) return `${mins}m`; + const hours = Math.floor(mins / 60); + if (hours < 24) return `${hours}h`; + return `${Math.floor(hours / 24)}d`; +} + +export default async function ExceptionsPage() { + const session = await auth(); + const orgId = session?.user.orgId; + + const runs = orgId + ? await prisma.run.findMany({ + where: { orgId, status: "INCIDENT" }, + // Oldest first: a parked run goes stale, it does not improve with age. + orderBy: { createdAt: "asc" }, + take: 200, + include: { + workflowVersion: { + include: { workflow: { select: { name: true } } }, + }, + incidentAssignee: { select: { id: true, name: true, email: true } }, + }, + }) + : []; + + // One batched lookup for the stopped-on step of every parked run, rather than + // a query per row. + const stepRows = + runs.length > 0 + ? await prisma.runStep.findMany({ + where: { + runId: { in: runs.map((r) => r.id) }, + index: { in: [...new Set(runs.map((r) => r.cursor))] }, + }, + select: { + runId: true, + index: true, + status: true, + label: true, + endedAt: true, + }, + }) + : []; + const stopped = new Map(stepRows.map((s) => [`${s.runId}:${s.index}`, s])); + + const members = orgId + ? await prisma.membership.findMany({ + where: { orgId }, + select: { user: { select: { id: true, name: true, email: true } } }, + }) + : []; + const assignable = members.map((m) => m.user); + + const rows = runs.map((run) => { + let step; + try { + step = parseWorkflowSteps(run.workflowVersion.steps)[run.cursor]; + } catch { + step = undefined; + } + const recorded = stopped.get(`${run.id}:${run.cursor}`) ?? null; + const disposition = classifyException({ + reason: run.error ?? "", + step, + recordedOutcome: + recorded?.status === "UNKNOWN" + ? "UNKNOWN" + : recorded?.status === "FAILED" + ? "FAILED" + : null, + }); + // Stored kind is what the engine decided at the time and is what an auditor + // should see; fall back to the live computation only for runs raised before + // the column existed. Duplicate risk always takes the cautious answer. + const kind = (run.incidentKind as ExceptionKind | null) ?? disposition.kind; + return { + run, + kind, + owner: disposition.owner, + headline: disposition.headline, + guidance: disposition.guidance, + retryMayDuplicate: + disposition.retryMayDuplicate || + kind === "OUTCOME_UNKNOWN" || + recorded?.status === "UNKNOWN", + stepLabel: recorded?.label ?? step?.label ?? step?.type ?? null, + // When it stopped, from the step — `Run.endedAt` stays null for an + // INCIDENT because the run is not terminal. + stoppedAt: recorded?.endedAt ?? run.createdAt, + }; + }); + + return ( +
+
+

Exceptions

+

+ Runs that stopped and need a person. Oldest first, grouped by who can + resolve them. +

+
+ + {rows.length === 0 ? ( + + +

Nothing waiting

+

+ No run is parked. Exceptions appear here the moment one stops and + cannot safely continue on its own. +

+
+
+ ) : ( + OWNER_ORDER.map((owner) => { + const group = rows.filter((r) => r.owner === owner); + if (group.length === 0) return null; + const copy = OWNER_COPY[owner]; + return ( +
+
+

+ {copy.title} + + {group.length} + +

+

+ {copy.blurb} +

+
+ + {group.map( + ({ + run, + kind, + headline, + guidance, + retryMayDuplicate, + stepLabel, + stoppedAt, + }) => ( + + +
+ + {run.workflowVersion.workflow.name} + + v{run.workflowVersion.version} + + + + stopped {age(stoppedAt)} ago + +
+ +
+ {headline} + + {" — step "} + {run.cursor + 1} + {stepLabel ? ` · ${stepLabel}` : ""} + +
+ +

+ {guidance} +

+ + {retryMayDuplicate && ( +

+ This step's effect may already have happened. + Check the target system before retrying. +

+ )} + + {run.error && ( +
+                          {run.error}
+                        
+ )} + + +
+
+ ), + )} +
+ ); + }) + )} +
+ ); +} diff --git a/cloud/apps/web/src/app/api/exceptions/route.ts b/cloud/apps/web/src/app/api/exceptions/route.ts new file mode 100644 index 0000000..529803b --- /dev/null +++ b/cloud/apps/web/src/app/api/exceptions/route.ts @@ -0,0 +1,203 @@ +import { NextResponse } from "next/server"; +import { auth } from "@/auth"; +import { prisma } from "@/lib/db"; +import { parseWorkflowSteps } from "@ghost/core/schema/step"; +import { + classifyException, + type ExceptionKind, + type ExceptionOwner, +} from "@ghost/core/classifier/exception"; + +/** + * The exception queue: every run in this org waiting on a human, oldest first. + * + * This is the surface that makes incidents a *product* rather than a state. + * Before it, an incident was reachable only by knowing a run's id and opening + * that run — fine when you triggered the run yourself thirty seconds ago, + * useless as the way an ops team finds the twelve things that broke overnight. + * + * Oldest-first is deliberate and not a UI preference: an exception is work + * someone is waiting on, and the run that has been parked longest is the one + * most likely to have gone stale — a portal session that will need + * re-authenticating, a batch whose downstream deadline is closest. Newest-first + * would bury exactly the rows that need attention. + * + * ## Reads only + * + * Nothing here mutates or resumes anything. Resolution stays in + * `POST /api/runs/[id]/incident`, which re-checks the run's state under its own + * authorization. A queue that could also act would need every one of those + * checks duplicated here. + */ + +/** Cap on rows returned, so one org's bad night cannot fetch unboundedly. */ +const MAX_ROWS = 200; + +export interface ExceptionRow { + runId: string; + workflowName: string; + workflowVersion: number; + stepIndex: number; + stepLabel: string | null; + kind: ExceptionKind; + owner: ExceptionOwner; + headline: string; + guidance: string; + retryUseful: boolean; + retryMayDuplicate: boolean; + error: string | null; + /** + * When the run stopped, taken from the stopped-on step. + * + * NOT `Run.endedAt`: an INCIDENT is not terminal, so that column is still null + * — reading it here produced a field that was always null. Falls back to the + * run's creation time when the step row carries no end (a run that halted + * before the step was ever written). + */ + stoppedAt: string | null; + assignee: { id: string; name: string | null; email: string | null } | null; + triggeredBy: { name: string | null; email: string | null } | null; +} + +export async function GET(req: Request) { + const session = await auth(); + if (!session?.user?.orgId) { + return NextResponse.json({ error: "unauthorized" }, { status: 401 }); + } + const orgId = session.user.orgId; + + const url = new URL(req.url); + // `mine=1` narrows to the caller's own assignments — the "my work" view. + const mine = url.searchParams.get("mine") === "1"; + const kindFilter = url.searchParams.get("kind"); + const ownerFilter = url.searchParams.get("owner"); + + const runs = await prisma.run.findMany({ + where: { + orgId, + status: "INCIDENT", + ...(mine && session.user.id + ? { incidentAssigneeId: session.user.id } + : {}), + ...(kindFilter ? { incidentKind: kindFilter } : {}), + }, + // Oldest first — see the module comment. + orderBy: { createdAt: "asc" }, + take: MAX_ROWS, + include: { + workflowVersion: { include: { workflow: { select: { name: true } } } }, + incidentAssignee: { select: { id: true, name: true, email: true } }, + triggeredBy: { select: { name: true, email: true } }, + }, + }); + + // The stopped-on step for every run in the page, in one query rather than one + // per row. Prisma cannot filter a relation by a column of the parent row + // (`index = run.cursor`), so the pairing is done here instead of in SQL — but + // it is still a single round trip, which is the part that matters. + const stepRows = await prisma.runStep.findMany({ + where: { + runId: { in: runs.map((r) => r.id) }, + index: { in: [...new Set(runs.map((r) => r.cursor))] }, + }, + select: { + runId: true, + index: true, + status: true, + label: true, + endedAt: true, + }, + }); + const stoppedStep = new Map( + stepRows.map((s) => [`${s.runId}:${s.index}`, s]), + ); + + const rows: ExceptionRow[] = []; + + for (const run of runs) { + // The stored `incidentKind` is the classification made when the run + // stopped, and it is what an auditor should see. Re-classify only when it is + // absent — a run that predates this column, or one whose incident was + // raised by an older worker. Never overwrite the stored value on read. + let kind = run.incidentKind as ExceptionKind | null; + + // The step is needed for the disposition text either way, and for duplicate + // risk when re-classifying. A malformed stored version must not take the + // whole queue down with it, so parse defensively. + let step; + try { + step = parseWorkflowSteps(run.workflowVersion.steps)[run.cursor]; + } catch { + step = undefined; + } + + const recorded = stoppedStep.get(`${run.id}:${run.cursor}`) ?? null; + + const disposition = classifyException({ + reason: run.error ?? "", + step, + recordedOutcome: + recorded?.status === "UNKNOWN" + ? "UNKNOWN" + : recorded?.status === "FAILED" + ? "FAILED" + : null, + }); + + // Trust the stored kind for the label, but take the *safety* flag from the + // live computation. If the two ever disagree about duplicate risk, the + // cautious answer wins: a stale stored kind must not be able to downgrade a + // warning that the current classifier would raise. + kind = kind ?? disposition.kind; + const retryMayDuplicate = + disposition.retryMayDuplicate || + kind === "OUTCOME_UNKNOWN" || + recorded?.status === "UNKNOWN"; + + const shaped: ExceptionRow = { + runId: run.id, + workflowName: run.workflowVersion.workflow.name, + workflowVersion: run.workflowVersion.version, + stepIndex: run.cursor, + stepLabel: recorded?.label ?? step?.label ?? step?.type ?? null, + kind, + owner: disposition.owner, + headline: disposition.headline, + guidance: disposition.guidance, + retryUseful: disposition.retryUseful, + retryMayDuplicate, + error: run.error, + stoppedAt: (recorded?.endedAt ?? run.createdAt).toISOString(), + assignee: run.incidentAssignee + ? { + id: run.incidentAssignee.id, + name: run.incidentAssignee.name, + email: run.incidentAssignee.email, + } + : null, + triggeredBy: run.triggeredBy + ? { name: run.triggeredBy.name, email: run.triggeredBy.email } + : null, + }; + + if (ownerFilter && shaped.owner !== ownerFilter) continue; + rows.push(shaped); + } + + // Counts are computed over the returned rows, so they describe exactly what + // the caller is looking at rather than a different, unfiltered population. + const byOwner: Record = {}; + const byKind: Record = {}; + for (const r of rows) { + byOwner[r.owner] = (byOwner[r.owner] ?? 0) + 1; + byKind[r.kind] = (byKind[r.kind] ?? 0) + 1; + } + + return NextResponse.json({ + total: rows.length, + truncated: runs.length === MAX_ROWS, + byOwner, + byKind, + exceptions: rows, + }); +} diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts new file mode 100644 index 0000000..625c21f --- /dev/null +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts @@ -0,0 +1,326 @@ +import { + afterAll, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; +import { prisma } from "@ghost/core/db"; +import type { WorkflowSteps } from "@ghost/core/schema/step"; + +/** + * Exception routing on the incident route: assignment, and the acknowledgement + * a risky retry now requires. + * + * The behaviour under test is narrow but load-bearing. `OUTCOME_UNKNOWN` means + * "this step may already have taken effect" — the engine deliberately lets a + * human retry it anyway (see journal.ts on clearing `inFlight`), because only a + * person can check the target system. What it must not do is let that happen as + * a one-click accident indistinguishable from retrying a network blip, or leave + * no record that the warning was ever shown. + * + * Requires DATABASE_URL; skips cleanly without one. + */ + +const hasDb = Boolean(process.env.DATABASE_URL); + +const enqueueRunWorkflow = vi.hoisted(() => + vi.fn(async (_data: unknown) => undefined), +); +vi.mock("@/lib/queue", () => ({ + enqueueRunWorkflow, + enqueueCompensateRun: async () => undefined, +})); + +const session = vi.hoisted(() => ({ + current: null as null | { user: { id: string; orgId: string } }, +})); +vi.mock("@/auth", () => ({ auth: async () => session.current })); + +const steps: WorkflowSteps = [ + { id: "nav", type: "navigate", url: "https://example.com" }, + { + id: "pay", + type: "click", + selector: { role: "button", name: "Pay invoice" }, + }, +]; + +describe.skipIf(!hasDb)("incident routing (Postgres)", () => { + let orgId: string; + let userId: string; + let otherOrgId: string; + let outsiderId: string; + const slug = `exc-${Date.now()}`; + + beforeAll(async () => { + const org = await prisma.organization.create({ + data: { name: "Exc", slug }, + }); + orgId = org.id; + const user = await prisma.user.create({ + data: { + email: `${slug}@example.com`, + memberships: { create: { orgId, role: "OWNER" } }, + }, + }); + userId = user.id; + + // A second tenant, to prove assignment cannot cross an org boundary. + const other = await prisma.organization.create({ + data: { name: "Other", slug: `${slug}-other` }, + }); + otherOrgId = other.id; + const outsider = await prisma.user.create({ + data: { + email: `${slug}-outsider@example.com`, + memberships: { create: { orgId: otherOrgId, role: "OWNER" } }, + }, + }); + outsiderId = outsider.id; + }); + + afterAll(async () => { + for (const id of [orgId, otherOrgId]) { + await prisma.organization + .delete({ where: { id } }) + .catch(() => undefined); + } + for (const id of [userId, outsiderId]) { + await prisma.user.delete({ where: { id } }).catch(() => undefined); + } + }); + + beforeEach(() => { + session.current = { user: { id: userId, orgId } }; + enqueueRunWorkflow.mockClear(); + }); + + /** + * An INCIDENT run parked on step 1 (the click), with the recorded step status + * controlling whether its effect is known. + */ + async function incidentRun(opts: { + stepStatus: "FAILED" | "UNKNOWN"; + error: string; + incidentKind?: string; + }) { + const wf = await prisma.workflow.create({ + data: { + orgId, + name: `wf-${Math.random().toString(36).slice(2, 8)}`, + versions: { create: { version: 1, steps: steps as never } }, + }, + include: { versions: true }, + }); + const run = await prisma.run.create({ + data: { + orgId, + workflowVersionId: wf.versions[0]!.id, + status: "INCIDENT", + cursor: 1, + error: opts.error, + incidentKind: opts.incidentKind, + triggeredById: userId, + steps: { + create: { + index: 1, + type: "click", + status: opts.stepStatus, + label: "Pay invoice", + error: opts.error, + }, + }, + }, + }); + return run.id; + } + + async function post(runId: string, body: unknown) { + const { POST } = await import("./route"); + return POST( + new Request(`http://localhost/api/runs/${runId}/incident`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }), + { params: Promise.resolve({ id: runId }) }, + ); + } + + // ---- The acknowledgement gate ---------------------------------------- + + it("refuses an unacknowledged retry when the step's effect may already have happened", async () => { + const runId = await incidentRun({ + stepStatus: "UNKNOWN", + error: + "OUTCOME_UNKNOWN: step 1 may or may not have taken effect — socket hang up", + }); + + const res = await post(runId, { action: "retry" }); + expect(res.status).toBe(409); + const body = (await res.json()) as { + requiresAcknowledgement?: boolean; + kind?: string; + }; + expect(body.requiresAcknowledgement).toBe(true); + expect(body.kind).toBe("OUTCOME_UNKNOWN"); + + // The run must not have moved, and nothing may have been enqueued. + const run = await prisma.run.findUnique({ where: { id: runId } }); + expect(run?.status).toBe("INCIDENT"); + expect(enqueueRunWorkflow).not.toHaveBeenCalled(); + }); + + it("allows the same retry once the risk is acknowledged, and records that it was", async () => { + const runId = await incidentRun({ + stepStatus: "UNKNOWN", + error: + "OUTCOME_UNKNOWN: step 1 may or may not have taken effect — socket hang up", + }); + + const res = await post(runId, { + action: "retry", + acknowledgeDuplicateRisk: true, + }); + expect(res.status).toBe(200); + + const run = await prisma.run.findUnique({ where: { id: runId } }); + expect(run?.status).toBe("QUEUED"); + expect(enqueueRunWorkflow).toHaveBeenCalledOnce(); + + // The acknowledgement is in the run journal, so the decision is attributable + // to a person and sealed into the hash chain like any other event. + const event = await prisma.runEvent.findFirst({ + where: { runId, type: "step.retry_requested" }, + orderBy: { seq: "desc" }, + }); + const payload = event?.payload as { + acknowledgedDuplicateRisk?: boolean; + kind?: string; + } | null; + expect(payload?.acknowledgedDuplicateRisk).toBe(true); + expect(payload?.kind).toBe("OUTCOME_UNKNOWN"); + + // And in the org-wide audit log. + const audit = await prisma.auditEvent.findFirst({ + where: { orgId, entityId: runId, action: "run.incident_retried" }, + orderBy: { createdAt: "desc" }, + }); + const meta = audit?.metadata as { + acknowledgedDuplicateRisk?: boolean; + } | null; + expect(meta?.acknowledgedDuplicateRisk).toBe(true); + }); + + it("does not demand acknowledgement for an ordinary failure", async () => { + // A plain recorded FAILED on a transient network error: retry is the normal, + // safe, one-click path and must stay that way. If this test ever needs an + // acknowledgement, the gate has become noise and will be clicked through. + const runId = await incidentRun({ + stepStatus: "FAILED", + error: "net::ERR_CONNECTION_RESET at https://example.com", + incidentKind: "TRANSIENT", + }); + + const res = await post(runId, { action: "retry" }); + expect(res.status).toBe(200); + expect(enqueueRunWorkflow).toHaveBeenCalledOnce(); + + const event = await prisma.runEvent.findFirst({ + where: { runId, type: "step.retry_requested" }, + orderBy: { seq: "desc" }, + }); + const payload = event?.payload as { + acknowledgedDuplicateRisk?: boolean; + } | null; + // Absent, not false: the flag's presence in the chain is the evidence. + expect(payload?.acknowledgedDuplicateRisk).toBeUndefined(); + }); + + it("ignores a stale stored kind that would understate the risk", async () => { + // The stored label says TRANSIENT, but the recorded step outcome says the + // effect is unknown. The authoritative signal must win, or writing a wrong + // `incidentKind` would be a way to bypass the gate. + const runId = await incidentRun({ + stepStatus: "UNKNOWN", + error: "Timeout 30000ms exceeded", + incidentKind: "TRANSIENT", + }); + + const res = await post(runId, { action: "retry" }); + expect(res.status).toBe(409); + }); + + // ---- Resolution clears the queue ------------------------------------- + + it("clears routing fields when the incident resolves", async () => { + const runId = await incidentRun({ + stepStatus: "FAILED", + error: "net::ERR_CONNECTION_RESET", + incidentKind: "TRANSIENT", + }); + await prisma.run.update({ + where: { id: runId }, + data: { incidentAssigneeId: userId }, + }); + + await post(runId, { action: "retry" }); + + const run = await prisma.run.findUnique({ where: { id: runId } }); + // A resolved exception must leave the queue, and must not stay "assigned" to + // someone who has finished with it. + expect(run?.incidentKind).toBeNull(); + expect(run?.incidentAssigneeId).toBeNull(); + }); + + // ---- Assignment ------------------------------------------------------- + + it("assigns an exception to a member and records it", async () => { + const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); + + const res = await post(runId, { action: "assign", assigneeId: userId }); + expect(res.status).toBe(200); + + const run = await prisma.run.findUnique({ where: { id: runId } }); + expect(run?.incidentAssigneeId).toBe(userId); + // Assignment resumes nothing. + expect(run?.status).toBe("INCIDENT"); + expect(enqueueRunWorkflow).not.toHaveBeenCalled(); + + const audit = await prisma.auditEvent.findFirst({ + where: { orgId, entityId: runId, action: "run.incident_assigned" }, + }); + expect(audit).not.toBeNull(); + }); + + it("unassigns on a null assignee", async () => { + const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); + await post(runId, { action: "assign", assigneeId: userId }); + + const res = await post(runId, { action: "assign", assigneeId: null }); + expect(res.status).toBe(200); + const run = await prisma.run.findUnique({ where: { id: runId } }); + expect(run?.incidentAssigneeId).toBeNull(); + }); + + it("refuses to assign an exception to a user outside the org", async () => { + // Tenant isolation. Without the membership check this would accept any user + // id, putting another org's user on this org's queue and confirming that + // account exists. + const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); + + const res = await post(runId, { action: "assign", assigneeId: outsiderId }); + expect(res.status).toBe(404); + const run = await prisma.run.findUnique({ where: { id: runId } }); + expect(run?.incidentAssigneeId).toBeNull(); + }); + + it("rejects an unknown action", async () => { + const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); + const res = await post(runId, { action: "resolve" }); + expect(res.status).toBe(400); + }); +}); diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts index 68878ba..365666e 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts @@ -6,6 +6,7 @@ import { appendAuditEvent, appendRunEvent } from "@ghost/core/audit-log"; import { RUN_EVENT_TYPES } from "@ghost/core/run-events"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyStep } from "@ghost/core/classifier"; +import { classifyException } from "@ghost/core/classifier/exception"; /** * Resolve an incident: retry the failed step, or skip it. @@ -46,9 +47,13 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str const userId = session.user.id ?? null; const { id } = await params; - const body = (await req.json().catch(() => ({}))) as { action?: string }; - if (body.action !== "retry" && body.action !== "skip") { - return NextResponse.json({ error: "action must be retry|skip" }, { status: 400 }); + const body = (await req.json().catch(() => ({}))) as { + action?: string; + assigneeId?: string | null; + acknowledgeDuplicateRisk?: boolean; + }; + if (body.action !== "retry" && body.action !== "skip" && body.action !== "assign") { + return NextResponse.json({ error: "action must be retry|skip|assign" }, { status: 400 }); } const run = await prisma.run.findFirst({ @@ -57,6 +62,45 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str }); if (!run) return NextResponse.json({ error: "no incident on this run" }, { status: 404 }); + // ---- Assign ----------------------------------------------------------- + // Handled before the compensation guard below, because assignment is the one + // control that is always appropriate: a failed *reversal* is exactly the kind + // of incident that needs a named owner, even though retry and skip are refused + // on it. + // + // Assignment changes no run state and resumes nothing, so it is open to any + // member. Deciding who looks at a problem is not authorizing the action that + // caused it. + if (body.action === "assign") { + let assigneeId: string | null = null; + if (body.assigneeId != null) { + if (typeof body.assigneeId !== "string") { + return NextResponse.json({ error: "assigneeId must be a string or null" }, { status: 400 }); + } + // Tenant isolation: an exception may only be assigned to a member of the + // org that owns the run. Without this check any user id would be accepted, + // leaking the existence of accounts across tenants and putting another + // org's user on this org's queue. + const member = await prisma.membership.findFirst({ + where: { orgId, userId: body.assigneeId }, + select: { userId: true }, + }); + if (!member) { + return NextResponse.json({ error: "assignee is not a member of this org" }, { status: 404 }); + } + assigneeId = member.userId; + } + + await prisma.run.update({ where: { id }, data: { incidentAssigneeId: assigneeId } }); + await appendAuditEvent(orgId, userId, { + action: assigneeId ? "run.incident_assigned" : "run.incident_unassigned", + entityType: "Run", + entityId: id, + metadata: { stepIndex: run.cursor, assigneeId }, + }); + return NextResponse.json({ ok: true, assigneeId }); + } + // A compensation that failed also stops as INCIDENT, and these controls are // forward-recovery controls. Letting them run on a reversal would append // forward-step recovery events to a run whose steps are all already complete: @@ -88,6 +132,9 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // up. The journal sequence is already monotonic per run, so it distinguishes // them without inventing a second counter. let resumeSeq = 0; + // Whether this resolution retried a step whose effect may already have + // happened. Hoisted so the audit event below can record the decision. + let acknowledgedDuplicateRisk = false; if (body.action === "skip") { if (classifyStep(step).sensitive) { @@ -116,7 +163,17 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str }); await tx.run.update({ where: { id }, - data: { status: "QUEUED", error: null, cursor: index + 1 }, + // Clear the routing fields: this run is no longer an open exception, so + // it must leave the queue and must not stay assigned to someone who has + // finished with it. A later failure raises a fresh, re-classified + // incident. + data: { + status: "QUEUED", + error: null, + cursor: index + 1, + incidentKind: null, + incidentAssigneeId: null, + }, }); const { seq } = await appendRunEvent( id, @@ -130,6 +187,41 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str resumeSeq = seq; }); } else { + // Retrying a step whose effect may already have happened is a decision the + // engine deliberately leaves to a human (see journal.ts on clearing + // `inFlight`) — but it must be a decision, not a mis-click. The disposition + // is recomputed here rather than read from `Run.incidentKind`, so a stale + // stored label cannot wave a risky retry through. + // + // This adds no prohibition: the retry still happens, on the same terms as + // before, for any caller that says it understands the risk. What changes is + // that it cannot happen *accidentally*, and the record shows the warning was + // shown. + const recorded = await prisma.runStep + .findUnique({ where: { runId_index: { runId: id, index } }, select: { status: true } }) + .catch(() => null); + const disposition = classifyException({ + reason: run.error ?? "", + step, + recordedOutcome: + recorded?.status === "UNKNOWN" ? "UNKNOWN" : recorded?.status === "FAILED" ? "FAILED" : null, + }); + + if (disposition.retryMayDuplicate && body.acknowledgeDuplicateRisk !== true) { + return NextResponse.json( + { + error: + "this step may already have taken effect, so retrying it could repeat that effect. " + + "Confirm in the target system first, then retry with acknowledgeDuplicateRisk: true.", + kind: disposition.kind, + guidance: disposition.guidance, + requiresAcknowledgement: true, + }, + { status: 409 }, + ); + } + acknowledgedDuplicateRisk = disposition.retryMayDuplicate; + // Retry: clear the recorded failure so the journal fold stops reporting it. // The step's own `step.started`/`step.failed` history stays in the chain — // this appends, it never rewrites. @@ -138,13 +230,25 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str where: { runId: id, index }, data: { status: "PENDING", error: null }, }); - await tx.run.update({ where: { id }, data: { status: "QUEUED", error: null } }); + await tx.run.update({ + where: { id }, + // See the skip branch: leaving INCIDENT clears the routing fields. + data: { status: "QUEUED", error: null, incidentKind: null, incidentAssigneeId: null }, + }); const { seq } = await appendRunEvent( id, { type: RUN_EVENT_TYPES.stepRetryRequested, stepIndex: index, - payload: { phase: "incident", retriedById: userId }, + payload: { + phase: "incident", + retriedById: userId, + kind: disposition.kind, + // Present only when the retry carried duplicate risk, so its + // presence in the chain is itself the evidence of an informed + // decision rather than a flag that is always there. + ...(disposition.retryMayDuplicate ? { acknowledgedDuplicateRisk: true } : {}), + }, }, tx, ); @@ -156,7 +260,11 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str action: body.action === "skip" ? "run.incident_skipped" : "run.incident_retried", entityType: "Run", entityId: id, - metadata: { stepIndex: index }, + metadata: { + stepIndex: index, + kind: run.incidentKind, + ...(acknowledgedDuplicateRisk ? { acknowledgedDuplicateRisk: true } : {}), + }, }); await enqueueRunWorkflow({ diff --git a/cloud/apps/web/src/components/app-sidebar.tsx b/cloud/apps/web/src/components/app-sidebar.tsx index bcf245f..cb393ce 100644 --- a/cloud/apps/web/src/components/app-sidebar.tsx +++ b/cloud/apps/web/src/components/app-sidebar.tsx @@ -2,7 +2,7 @@ import Link from "next/link"; import { usePathname } from "next/navigation"; -import { LayoutDashboard, Workflow, Film, PlayCircle, ShieldCheck, Settings, Code2 } from "lucide-react"; +import { LayoutDashboard, Workflow, Film, PlayCircle, TriangleAlert, ShieldCheck, Settings, Code2 } from "lucide-react"; import { cn } from "@/lib/cn"; import { SOURCE_URL } from "@/lib/source-url"; @@ -11,6 +11,7 @@ const NAV = [ { href: "/workflows", label: "Workflows", icon: Workflow }, { href: "/recordings", label: "Recordings", icon: Film }, { href: "/runs", label: "Runs", icon: PlayCircle }, + { href: "/exceptions", label: "Exceptions", icon: TriangleAlert }, { href: "/audit", label: "Audit", icon: ShieldCheck }, { href: "/settings", label: "Settings", icon: Settings }, ]; diff --git a/cloud/apps/web/src/components/exception-assignee.tsx b/cloud/apps/web/src/components/exception-assignee.tsx new file mode 100644 index 0000000..5bfb124 --- /dev/null +++ b/cloud/apps/web/src/components/exception-assignee.tsx @@ -0,0 +1,84 @@ +"use client"; + +import { useState, useTransition } from "react"; +import { useRouter } from "next/navigation"; + +interface Member { + id: string; + name: string | null; + email: string | null; +} + +/** + * Assign an open exception to a person. + * + * Deliberately a plain select rather than a modal or a drag target: this is the + * control an ops lead uses a dozen times in a triage pass, and every extra click + * is one they pay repeatedly. Assignment changes no run state and resumes + * nothing, so it needs no confirmation step. + * + * The route is the authority on who may be assigned — it re-checks org + * membership — so the list here is a convenience, not the security boundary. + */ +export function ExceptionAssignee({ + runId, + assignee, + members, +}: { + runId: string; + assignee: Member | null; + members: Member[]; +}) { + const router = useRouter(); + const [pending, startTransition] = useTransition(); + const [error, setError] = useState(null); + const [value, setValue] = useState(assignee?.id ?? ""); + + async function assign(next: string) { + setError(null); + setValue(next); + const res = await fetch(`/api/runs/${runId}/incident`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + action: "assign", + assigneeId: next === "" ? null : next, + }), + }); + if (!res.ok) { + const body = (await res.json().catch(() => ({}))) as { error?: string }; + setError(body.error ?? "could not assign"); + // Put the control back to the truth, so it never shows an assignment the + // server rejected. + setValue(assignee?.id ?? ""); + return; + } + startTransition(() => router.refresh()); + } + + return ( +
+ + + {error && {error}} +
+ ); +} diff --git a/cloud/apps/web/src/components/run-timeline.tsx b/cloud/apps/web/src/components/run-timeline.tsx index cbe0796..f61dc20 100644 --- a/cloud/apps/web/src/components/run-timeline.tsx +++ b/cloud/apps/web/src/components/run-timeline.tsx @@ -44,6 +44,15 @@ interface RunView { * this exists so nobody discovers the rule by having a click rejected. */ canApprove: boolean; + /** Deterministic disposition of the stop. Non-null only for INCIDENT. */ + exception: { + kind: string; + owner: string; + headline: string; + guidance: string; + retryUseful: boolean; + retryMayDuplicate: boolean; + } | null; } interface ChainView { org: { intact: boolean; count: number }; @@ -105,6 +114,10 @@ export function RunTimeline({ runId }: { runId: string }) { const [undo, setUndo] = useState(null); const [busy, setBusy] = useState(false); const [notice, setNotice] = useState(null); + // Two-step confirmation for a retry whose effect may already have happened. + // Local to the panel; the route is the authority and refuses an + // unacknowledged risky retry regardless. + const [confirmRetry, setConfirmRetry] = useState(false); const load = useCallback(async () => { const res = await fetch(`/api/runs/${runId}`, { cache: "no-store" }); @@ -306,12 +319,19 @@ export function RunTimeline({ runId }: { runId: string }) { {incidentStep?.label ? ` — ${incidentStep.label}` : ""} - {incidentStep?.status === "UNKNOWN" && ( -

- This step started but never reported an outcome, and its effect cannot safely be - repeated. Check the target system before retrying — Ghost will not guess whether it - took effect. -

+ {run.exception && ( +
+

+ {run.exception.headline} +

+

{run.exception.guidance}

+
)} {run.restoreScreenshotUrl && (
@@ -328,23 +348,72 @@ export function RunTimeline({ runId }: { runId: string }) { />
)} -
- - -
+ {/* + Retry is de-emphasised when the classifier says it cannot help (a + changed selector fails identically) and becomes a two-step + confirmation when it could repeat an effect that already happened. + The route enforces the same rule — it refuses an unacknowledged + risky retry with a 409 — so this is the honest affordance for a + decision the server will demand anyway, not a client-side guard + standing in for one. + */} + {confirmRetry ? ( +
+

+ Retry anyway? This step may already have taken effect, and retrying could repeat + it. Confirm in the target system first. +

+
+ + +
+
+ ) : ( +
+ + +
+ )} )} diff --git a/cloud/apps/web/src/lib/run-view.ts b/cloud/apps/web/src/lib/run-view.ts index 590e0d6..f534798 100644 --- a/cloud/apps/web/src/lib/run-view.ts +++ b/cloud/apps/web/src/lib/run-view.ts @@ -2,6 +2,8 @@ import { prisma } from "@/lib/db"; import { canApproveRun } from "@ghost/core/roles"; import { throttleReason } from "@ghost/core/concurrency"; import { loadActor } from "@/lib/members"; +import { parseWorkflowSteps } from "@ghost/core/schema/step"; +import { classifyException, type ExceptionKind } from "@ghost/core/classifier/exception"; /** * Build the run detail view: status, ordered steps, pending approvals, and @@ -107,8 +109,53 @@ export async function buildRunView(orgId: string, viewerId: string, runId: strin } } + // Exception disposition, for an INCIDENT run only. Computed here so the + // timeline can lead with what kind of problem this is and whether retrying + // risks repeating an effect, instead of a raw driver error and two + // equal-weight buttons. + // + // The stored `incidentKind` is what the engine decided when it stopped and is + // what an auditor should see; the live computation supplies the guidance text + // and — always — the cautious answer on duplicate risk, so a stale stored + // label can never downgrade a warning. + let exception: { + kind: ExceptionKind; + owner: string; + headline: string; + guidance: string; + retryUseful: boolean; + retryMayDuplicate: boolean; + } | null = null; + + if (run.status === "INCIDENT") { + let stoppedStep; + try { + stoppedStep = parseWorkflowSteps(run.workflowVersion.steps)[run.cursor]; + } catch { + stoppedStep = undefined; + } + const recorded = run.steps.find((s) => s.index === run.cursor); + const d = classifyException({ + reason: run.error ?? "", + step: stoppedStep, + recordedOutcome: + recorded?.status === "UNKNOWN" ? "UNKNOWN" : recorded?.status === "FAILED" ? "FAILED" : null, + }); + const kind = (run.incidentKind as ExceptionKind | null) ?? d.kind; + exception = { + kind, + owner: d.owner, + headline: d.headline, + guidance: d.guidance, + retryUseful: d.retryUseful, + retryMayDuplicate: + d.retryMayDuplicate || kind === "OUTCOME_UNKNOWN" || recorded?.status === "UNKNOWN", + }; + } + return { throttle, + exception, id: run.id, status: run.status, error: run.error, diff --git a/cloud/apps/web/src/middleware.ts b/cloud/apps/web/src/middleware.ts index 291edb2..62b9faa 100644 --- a/cloud/apps/web/src/middleware.ts +++ b/cloud/apps/web/src/middleware.ts @@ -71,6 +71,7 @@ export const config = { matcher: [ "/audit/:path*", "/dashboard/:path*", + "/exceptions/:path*", "/recordings/:path*", "/runs/:path*", "/settings/:path*", diff --git a/cloud/apps/worker/src/jobs/runWorkflow.ts b/cloud/apps/worker/src/jobs/runWorkflow.ts index 293bff1..88a0cec 100644 --- a/cloud/apps/worker/src/jobs/runWorkflow.ts +++ b/cloud/apps/worker/src/jobs/runWorkflow.ts @@ -24,6 +24,7 @@ import { import { planRestore, type ExecutedRecord } from "../runtime/restore.js"; import { claimSlot, recordThrottle, releaseSlot } from "../runtime/slots.js"; import { classifyError } from "../runtime/errors.js"; +import { classifyException } from "@ghost/core/classifier/exception"; import { effectiveRetry, effectiveTimeout, backoffFor } from "../runtime/policy.js"; import { BrowserSession, @@ -346,7 +347,14 @@ export async function runWorkflowJob(job: Job): Promise { // approval or a broken expression is not recoverable — retrying cannot // change either — so those stay terminal. if (action.recoverable) { - await raiseIncident(runId, run.orgId, run.triggeredById, action.index, action.reason); + await raiseIncident( + runId, + run.orgId, + run.triggeredById, + action.index, + action.reason, + steps[action.index], + ); } else { await failRun(runId, run.orgId, run.triggeredById, action.index, action.reason); } @@ -381,6 +389,7 @@ export async function runWorkflowJob(job: Job): Promise { run.triggeredById, action.index, `OUTCOME_UNKNOWN: ${action.reason}`, + action.step, ); return; } @@ -572,6 +581,7 @@ async function executeStep(args: ExecuteArgs): Promise { actorId, index, `RESTORE_UNSAFE: ${restored.reason}`, + step, ); return { kind: "halt" }; } @@ -663,7 +673,14 @@ async function executeStep(args: ExecuteArgs): Promise { }); if (!passed) { - await raiseIncident(runId, orgId, actorId, index, `verification failed at step ${index}`); + await raiseIncident( + runId, + orgId, + actorId, + index, + `verification failed at step ${index}`, + step, + ); return { kind: "halt" }; } @@ -691,6 +708,7 @@ async function executeStep(args: ExecuteArgs): Promise { actorId, index, `OUTCOME_UNKNOWN: step ${index} may or may not have taken effect — ${message}`, + step, ); return { kind: "halt" }; } @@ -720,12 +738,19 @@ async function executeStep(args: ExecuteArgs): Promise { entityId: `${runId}:${index}`, metadata: { error: message }, }); - await raiseIncident(runId, orgId, actorId, index, message); + await raiseIncident(runId, orgId, actorId, index, message, step); return { kind: "halt" }; } } - await raiseIncident(runId, orgId, actorId, index, lastError || "step exhausted its retries"); + await raiseIncident( + runId, + orgId, + actorId, + index, + lastError || "step exhausted its retries", + step, + ); return { kind: "halt" }; } @@ -916,6 +941,17 @@ async function failRun( * A failed or unresumable step stops the run and waits for a human, rather than * killing it. Borrowed from Camunda incidents: an ops person watching a * 40-invoice batch wants to fix the one broken step, not restart the batch. + * + * The incident is *classified* here, when it is raised, rather than on read. Two + * reasons: the queue can then filter and sort in SQL instead of re-classifying + * every open incident on every page load, and `Run.error` can be overwritten by + * a later failure on the same run — so a disposition computed on read might not + * be the one the engine actually stopped on. The audit event records the + * classification next to the reason, which is what lets someone later ask "what + * did Ghost think this was at the time?" and get an answer that cannot drift. + * + * Classification never changes control flow. It decides what a human is *shown* + * and whose desk the work lands on; the state transition is identical either way. */ async function raiseIncident( runId: string, @@ -923,16 +959,34 @@ async function raiseIncident( actorId: string | null, index: number, reason: string, + step?: WorkflowStep, ): Promise { + // The recorded step outcome outranks the reason text: `UNKNOWN` is written + // only after the engine has decided an effect is genuinely indeterminate, and + // that judgement must survive a reason string that reads like a plain timeout. + const recorded = await prisma.runStep + .findUnique({ where: { runId_index: { runId, index } }, select: { status: true } }) + .catch(() => null); + const recordedOutcome = + recorded?.status === "UNKNOWN" ? "UNKNOWN" : recorded?.status === "FAILED" ? "FAILED" : null; + const disposition = classifyException({ reason, step, recordedOutcome }); + await prisma.run.update({ where: { id: runId }, - data: { status: "INCIDENT", error: reason, cursor: index }, + data: { status: "INCIDENT", error: reason, cursor: index, incidentKind: disposition.kind }, }); await appendAuditEvent(orgId, actorId, { action: "run.incident", entityType: "Run", entityId: runId, - metadata: { stepIndex: index, reason, runChainHead: await runChainHead(runId) }, + metadata: { + stepIndex: index, + reason, + kind: disposition.kind, + owner: disposition.owner, + retryMayDuplicate: disposition.retryMayDuplicate, + runChainHead: await runChainHead(runId), + }, }); } diff --git a/cloud/packages/core/package.json b/cloud/packages/core/package.json index dd2f7be..852238f 100644 --- a/cloud/packages/core/package.json +++ b/cloud/packages/core/package.json @@ -10,6 +10,7 @@ "./schema/step": "./src/schema/step.ts", "./classifier": "./src/classifier/sensitive.ts", "./classifier/replay": "./src/classifier/replay.ts", + "./classifier/exception": "./src/classifier/exception.ts", "./queue": "./src/queue.ts", "./audit": "./src/audit.ts", "./audit-log": "./src/auditLog.ts", diff --git a/cloud/packages/core/prisma/migrations/20260817170000_run_exception_routing/migration.sql b/cloud/packages/core/prisma/migrations/20260817170000_run_exception_routing/migration.sql new file mode 100644 index 0000000..a7fa818 --- /dev/null +++ b/cloud/packages/core/prisma/migrations/20260817170000_run_exception_routing/migration.sql @@ -0,0 +1,22 @@ +-- Exception routing: classify an incident when it is raised, and let a human own it. +-- +-- Both columns are nullable with no default: every existing row is either not an +-- incident (so they are correctly null) or is a pre-existing incident that was +-- never classified. The queue renders an unclassified incident as UNKNOWN rather +-- than backfilling a guess, because the reason string it would have to guess from +-- may since have been overwritten by a later failure on the same run. + +-- AlterTable +ALTER TABLE "Run" ADD COLUMN "incidentKind" TEXT, +ADD COLUMN "incidentAssigneeId" TEXT; + +-- CreateIndex +CREATE INDEX "Run_orgId_status_createdAt_idx" ON "Run"("orgId", "status", "createdAt"); + +-- CreateIndex +CREATE INDEX "Run_incidentAssigneeId_idx" ON "Run"("incidentAssigneeId"); + +-- AddForeignKey +-- ON DELETE SET NULL: removing a person from the org returns their open +-- exceptions to the unassigned queue. It must never cascade a run away. +ALTER TABLE "Run" ADD CONSTRAINT "Run_incidentAssigneeId_fkey" FOREIGN KEY ("incidentAssigneeId") REFERENCES "User"("id") ON DELETE SET NULL ON UPDATE CASCADE; diff --git a/cloud/packages/core/prisma/schema.prisma b/cloud/packages/core/prisma/schema.prisma index ad0e72a..863e86c 100644 --- a/cloud/packages/core/prisma/schema.prisma +++ b/cloud/packages/core/prisma/schema.prisma @@ -39,6 +39,7 @@ model User { memberships Membership[] createdFlows Workflow[] @relation("WorkflowCreatedBy") triggeredRuns Run[] @relation("RunTriggeredBy") + assignedIncidents Run[] @relation("RunIncidentAssignee") resolvedApprovals Approval[] @relation("ApprovalResolvedBy") agentCredentials AgentCredential[] invitationsSent Invitation[] @relation("InvitationCreatedBy") @@ -422,9 +423,29 @@ model Run { // retention window. artifactsPurgedAt DateTime? + // Exception routing. Both are denormalized caches for the queue view, in the + // same sense as `cursor`: written in the same transaction that raises the + // incident, never the source of truth for a control-flow decision. + // + // `incidentKind` is the deterministic classifier's verdict + // (packages/core/src/classifier/exception.ts) recorded at the moment the run + // stopped, so the queue filters and sorts in SQL instead of re-classifying + // every row on read. Stored rather than derived because the reason string it + // was computed from can be superseded by a later failure on the same run, and + // "what did Ghost think this was when it stopped?" deserves the answer from + // that moment. + // + // Both are cleared when the run leaves INCIDENT — a resolved exception must + // not linger in the queue, nor stay assigned to whoever finished with it. + incidentKind String? + incidentAssigneeId String? + org Organization @relation(fields: [orgId], references: [id], onDelete: Cascade) workflowVersion WorkflowVersion @relation(fields: [workflowVersionId], references: [id], onDelete: Cascade) triggeredBy User? @relation("RunTriggeredBy", fields: [triggeredById], references: [id], onDelete: SetNull) + // SetNull, not Cascade: removing a person from the org must never delete the + // run they were looking at. The exception returns to unassigned. + incidentAssignee User? @relation("RunIncidentAssignee", fields: [incidentAssigneeId], references: [id], onDelete: SetNull) steps RunStep[] approvals Approval[] events RunEvent[] @@ -432,6 +453,9 @@ model Run { @@index([orgId]) @@index([workflowVersionId]) @@index([status, leaseExpiresAt]) + // The exception queue's own query: this org's open incidents, oldest first. + @@index([orgId, status, createdAt]) + @@index([incidentAssigneeId]) // Admission counts a workflow's in-flight runs on every start and resume. @@index([workflowVersionId, status]) @@index([workflowVersionId, slotHeldAt]) diff --git a/cloud/packages/core/src/classifier/exception.test.ts b/cloud/packages/core/src/classifier/exception.test.ts new file mode 100644 index 0000000..fbf6ccc --- /dev/null +++ b/cloud/packages/core/src/classifier/exception.test.ts @@ -0,0 +1,224 @@ +import { describe, expect, it } from "vitest"; +import { + EXCEPTION_KINDS, + classifyException, + needsAuthoring, + type ExceptionKind, +} from "./exception.js"; +import type { WorkflowStep } from "../schema/step.js"; + +const click: WorkflowStep = { + id: "s1", + type: "click", + selector: { css: "#pay" }, +}; + +const verify: WorkflowStep = { + id: "s2", + type: "verify", + assertion: { kind: "textPresent", expected: "Paid" }, +}; + +function kindOf(reason: string, step?: WorkflowStep): ExceptionKind { + return classifyException({ reason, step }).kind; +} + +describe("classifyException — authoritative signals win", () => { + it("treats a recorded UNKNOWN outcome as OUTCOME_UNKNOWN regardless of the reason text", () => { + // The reason here looks like a plain transient timeout. The recorded outcome + // is the engine's own considered judgement and must override it, or a + // possibly-completed payment would be offered a one-click retry. + const d = classifyException({ + reason: "Timeout 30000ms exceeded", + step: click, + recordedOutcome: "UNKNOWN", + }); + expect(d.kind).toBe("OUTCOME_UNKNOWN"); + expect(d.retryMayDuplicate).toBe(true); + }); + + it("reads Ghost's own structured prefixes", () => { + expect( + kindOf("OUTCOME_UNKNOWN: step 4 may or may not have taken effect"), + ).toBe("OUTCOME_UNKNOWN"); + expect( + kindOf("RESTORE_UNSAFE: captured session digest did not match"), + ).toBe("RESTORE_UNSAFE"); + expect(kindOf("RUN_TIMEOUT: exceeded wall-clock budget of 600000ms")).toBe( + "TRANSIENT", + ); + }); + + it("reads the unprefixed phrases the engine writes", () => { + expect(kindOf("approval for step 3 expired before it was used")).toBe( + "APPROVAL_EXPIRED", + ); + expect(kindOf("verification failed at step 7")).toBe("VERIFICATION"); + }); + + it("does not sweep up unrelated errors that merely mention approval", () => { + // Whole-phrase anchoring, not keyword matching: this is a missing button on + // an approvals page, which is an authoring problem, not an expired gate. + expect(kindOf('waiting for locator("#approval-banner")')).toBe( + "TARGET_MISSING", + ); + }); +}); + +describe("classifyException — best-effort text rules", () => { + it("files a Playwright locator timeout as a target change, not a blip", () => { + // Regression guard for rule ordering: this string contains "Timeout", so a + // generic timeout rule placed first would misfile every changed selector as + // transient and retry it forever. + const reason = + 'Timeout 30000ms exceeded.\nCall log:\n - waiting for locator("#submit-order")'; + expect(kindOf(reason)).toBe("TARGET_MISSING"); + }); + + it("files strict-mode violations with the author too", () => { + expect( + kindOf("strict mode violation: locator resolved to 3 elements"), + ).toBe("TARGET_MISSING"); + }); + + it("recognizes auth refusals", () => { + expect(kindOf("Request failed with status 403")).toBe("AUTH"); + expect(kindOf("Your session has expired, please sign in again")).toBe( + "AUTH", + ); + }); + + it("recognizes rejected values", () => { + expect(kindOf("Invalid value for field 'amount'")).toBe("DATA"); + expect(kindOf("Purchase order number is required")).toBe("DATA"); + }); + + it("recognizes network and lifecycle noise as transient", () => { + expect( + kindOf("net::ERR_CONNECTION_RESET at https://portal.example.com"), + ).toBe("TRANSIENT"); + expect(kindOf("Target page, context or browser has been closed")).toBe( + "TRANSIENT", + ); + expect(kindOf("Request failed with status 503")).toBe("TRANSIENT"); + }); + + it("falls back to a bare generic timeout as transient", () => { + expect(kindOf("Timeout 5000ms exceeded")).toBe("TRANSIENT"); + }); +}); + +describe("classifyException — fails closed", () => { + it("leaves an unrecognized reason UNKNOWN rather than guessing", () => { + const d = classifyException({ + reason: "the frobnicator disagreed", + step: click, + }); + expect(d.kind).toBe("UNKNOWN"); + expect(d.retryUseful).toBe(false); + }); + + it("does not let the bare retry-exhausted wrapper imply a category", () => { + // This string carries no information about cause. Classifying it as + // transient would offer a retry on a step that has already exhausted its + // retries for an unknown reason. + expect(kindOf("step exhausted its retries")).toBe("UNKNOWN"); + }); + + it("treats an empty reason as UNKNOWN", () => { + expect(kindOf("")).toBe("UNKNOWN"); + }); + + it("assumes duplicate risk when the step is not known", () => { + // No step means the classifier cannot prove the effect was confined to the + // browser, so it must assume the dangerous case. + const d = classifyException({ reason: "OUTCOME_UNKNOWN: gone dark" }); + expect(d.retryMayDuplicate).toBe(true); + }); +}); + +describe("retryMayDuplicate is the conjunction, not a synonym for the kind", () => { + it("warns on an indeterminate mutating step", () => { + const d = classifyException({ reason: "OUTCOME_UNKNOWN: ?", step: click }); + expect(d.retryMayDuplicate).toBe(true); + }); + + it("does not warn on an indeterminate read", () => { + // Repeating a `verify` costs nothing. If this warned, the warning would + // appear often enough to be ignored when it matters. + const d = classifyException({ reason: "OUTCOME_UNKNOWN: ?", step: verify }); + expect(d.retryMayDuplicate).toBe(false); + }); + + it("never warns for kinds other than OUTCOME_UNKNOWN", () => { + for (const reason of [ + "net::ERR_CONNECTION_RESET", + 'waiting for locator("#x")', + "verification failed at step 2", + "Request failed with status 403", + ]) { + expect(classifyException({ reason, step: click }).retryMayDuplicate).toBe( + false, + ); + } + }); +}); + +describe("dispositions are complete and coherent", () => { + it("gives every kind an owner, headline, and guidance", () => { + for (const kind of EXCEPTION_KINDS) { + // Reach each kind through the classifier where a reason exists for it, + // and assert the table itself is filled in for all of them. + const d = classifyException({ reason: `${kind}:` }); + expect(d.headline.length).toBeGreaterThan(0); + expect(d.guidance.length).toBeGreaterThan(0); + expect(["operator", "author", "administrator"]).toContain(d.owner); + } + }); + + it("routes target changes to the author and auth to the administrator", () => { + expect(needsAuthoring("TARGET_MISSING")).toBe(true); + expect(needsAuthoring("TRANSIENT")).toBe(false); + expect(classifyException({ reason: "403 Forbidden" }).owner).toBe( + "administrator", + ); + expect( + classifyException({ reason: "approval for step 1 expired" }).owner, + ).toBe("administrator"); + }); + + it("only claims retry is useful for transient failures", () => { + // Every other kind recurs identically on retry. If this list grows, the + // guidance text for the new kind needs to justify it. + const useful = EXCEPTION_KINDS.filter( + (k) => classifyException({ reason: syntheticReasonFor(k) }).kind === k, + ).filter( + (k) => classifyException({ reason: syntheticReasonFor(k) }).retryUseful, + ); + expect(useful).toEqual(["TRANSIENT"]); + }); +}); + +/** A reason string that classifies to the given kind, for table-driven tests. */ +function syntheticReasonFor(kind: ExceptionKind): string { + switch (kind) { + case "TRANSIENT": + return "RUN_TIMEOUT: exceeded budget"; + case "TARGET_MISSING": + return 'waiting for locator("#x")'; + case "AUTH": + return "403 Forbidden"; + case "VERIFICATION": + return "verification failed at step 1"; + case "OUTCOME_UNKNOWN": + return "OUTCOME_UNKNOWN: dark"; + case "APPROVAL_EXPIRED": + return "approval for step 1 expired before it was used"; + case "RESTORE_UNSAFE": + return "RESTORE_UNSAFE: digest mismatch"; + case "DATA": + return "Invalid value for field 'x'"; + case "UNKNOWN": + return "no idea"; + } +} diff --git a/cloud/packages/core/src/classifier/exception.ts b/cloud/packages/core/src/classifier/exception.ts new file mode 100644 index 0000000..62918e3 --- /dev/null +++ b/cloud/packages/core/src/classifier/exception.ts @@ -0,0 +1,305 @@ +import type { WorkflowStep } from "../schema/step.js"; +import { replaySafety } from "./replay.js"; + +/** + * Deterministic exception classifier — the routing half of Camunda incidents. + * + * Third sibling of `classifyStep` (sensitive.ts) and `replaySafety` (replay.ts), + * held to the same discipline: pure, rule-based, exhaustive, fail-closed. The + * three answer different questions about the same step: + * + * | Classifier | Question | + * |---|---| + * | `classifyStep` | Must a human approve this before it runs? | + * | `replaySafety` | Is it safe to re-apply silently while restoring state? | + * | `classifyException` | This stopped. Who fixes it, and is retrying safe? | + * + * ## Why this is not a model call + * + * Engineering rule 1 — AI may propose, deterministic code executes. An incident + * disposition decides whether a human is *offered* a one-click retry on a step + * that may already have charged a customer. A model that classifies that wrong + * once has caused a double payment, so the mapping is a rule table that can be + * read, tested, and argued with. + * + * ## Two tiers of signal, and why the order matters + * + * 1. **Authoritative.** The recorded step outcome, and the structured prefixes + * Ghost itself emits (`OUTCOME_UNKNOWN:`, `RESTORE_UNSAFE:`, `RUN_TIMEOUT:`). + * These mean exactly what they say. + * 2. **Best-effort.** Raw driver text from Playwright or a target system. Useful + * — "waiting for locator" really does mean the page changed — but it is a + * third party's prose and may be reworded by an upgrade at any time. + * + * Tier 1 is checked first and wins outright. Tier 2 only ever refines an + * otherwise-`UNKNOWN` result, and an unrecognized reason stays `UNKNOWN` rather + * than being forced into the nearest-looking bucket. A wrong-but-confident label + * on an incident is worse than no label, because the whole point is telling a + * human where to look. + */ + +export type ExceptionKind = + /** Infrastructure or timing. Nothing is wrong with the workflow. */ + | "TRANSIENT" + /** The element is gone. The target UI changed under a recorded workflow. */ + | "TARGET_MISSING" + /** Credentials, session, or permission at the target system. */ + | "AUTH" + /** The step ran and the outcome was not what the workflow asserted. */ + | "VERIFICATION" + /** The step may or may not have taken effect. Retrying may repeat it. */ + | "OUTCOME_UNKNOWN" + /** An approval was granted but expired before the run could consume it. */ + | "APPROVAL_EXPIRED" + /** Page state could not be provably rebuilt, so the run refused to guess. */ + | "RESTORE_UNSAFE" + /** A value was rejected by the target system. */ + | "DATA" + /** Unrecognized. Needs human judgement — never assumed benign. */ + | "UNKNOWN"; + +/** + * Which desk an exception belongs on. + * + * This is the actual routing decision, and it is why this file exists rather + * than a label on a status badge. "A run stopped" is not actionable; "the portal + * changed and whoever maintains this workflow needs to re-record step 4" is. + */ +export type ExceptionOwner = + /** The person running the work. Can decide, retry, or escalate. */ + | "operator" + /** Whoever maintains this workflow. The steps themselves need changing. */ + | "author" + /** Whoever holds credentials and grants approvals for this org. */ + | "administrator"; + +export interface ExceptionDisposition { + kind: ExceptionKind; + /** Which desk this belongs on. */ + owner: ExceptionOwner; + /** One line naming the class of problem, safe to show in a queue row. */ + headline: string; + /** What the owner should actually do. */ + guidance: string; + /** + * Would retrying the same step plausibly change the outcome? + * + * False does not mean "forbidden" — a human may always retry, and the engine + * deliberately lets them (see journal.ts on clearing `inFlight`). It means the + * UI should not lead with retry, because the same failure will recur. + */ + retryUseful: boolean; + /** + * Could retrying repeat an effect that has already happened? + * + * The one field with teeth. When true, a retry needs explicit human + * acknowledgement rather than a one-click button, and that acknowledgement is + * recorded — the difference between an informed decision and an accident. + */ + retryMayDuplicate: boolean; +} + +/** Authoritative prefixes emitted by Ghost's own engine. */ +const PREFIX_KINDS: ReadonlyArray = [ + ["OUTCOME_UNKNOWN:", "OUTCOME_UNKNOWN"], + ["RESTORE_UNSAFE:", "RESTORE_UNSAFE"], + ["RUN_TIMEOUT:", "TRANSIENT"], +]; + +/** + * Best-effort patterns over third-party error text, most specific first. + * + * Ordering is load-bearing: a Playwright locator timeout contains the word + * "Timeout", so `TARGET_MISSING` must be tested before the generic timeout rule + * or every changed selector would be misfiled as a transient blip and retried + * forever. + */ +const TEXT_RULES: ReadonlyArray = [ + // Playwright's locator failures. "strict mode violation" is the opposite + // problem — too many matches — but lands on the same desk: the selector no + // longer identifies one thing, so the workflow needs re-authoring. + [ + /waiting for (?:locator|selector)|strict mode violation|no element matches/i, + "TARGET_MISSING", + ], + [ + /element is not (?:visible|attached|enabled)|not attached to the DOM/i, + "TARGET_MISSING", + ], + + // Auth before generic HTTP, since a 403 body often also mentions the URL. + [/\b(?:401|403)\b|unauthorized|forbidden|access denied/i, "AUTH"], + [ + /session (?:has )?expired|please (?:sign|log) ?in|login required|invalid credentials/i, + "AUTH", + ], + + // Value rejected by the target, as opposed to the field not being found. + [/\b(?:invalid|malformed)\b.*\b(?:value|format|input|field)\b/i, "DATA"], + [ + /is required\b|must be (?:a|an|at least|no more)|does not match the expected/i, + "DATA", + ], + + // Network and browser-lifecycle noise. Genuinely transient. + [ + /net::ERR_|ECONNREFUSED|ECONNRESET|ETIMEDOUT|EAI_AGAIN|socket hang up/i, + "TRANSIENT", + ], + [ + /target (?:page|closed|crashed)|browser has been closed|page crashed/i, + "TRANSIENT", + ], + [ + /\b(?:429|502|503|504)\b|too many requests|service unavailable|bad gateway/i, + "TRANSIENT", + ], + + // Generic timeout last, after every specific timeout shape above. + [/timeout .* exceeded|timed out/i, "TRANSIENT"], +]; + +const DISPOSITIONS: Record< + ExceptionKind, + Omit +> = { + TRANSIENT: { + owner: "operator", + headline: "Temporary failure", + guidance: + "Infrastructure or timing, not the workflow. Retry the step; if it fails the same way twice, treat it as a target change instead.", + retryUseful: true, + }, + TARGET_MISSING: { + owner: "author", + headline: "Target element not found", + guidance: + "The page no longer matches what was recorded. Retrying will fail identically — re-record or edit the step's selector, then start a new run.", + retryUseful: false, + }, + AUTH: { + owner: "administrator", + headline: "Authentication or permission refused", + guidance: + "The target system rejected Ghost's session or permissions. Refresh the stored credential or widen its scope, then retry.", + retryUseful: false, + }, + VERIFICATION: { + owner: "operator", + headline: "Outcome did not match expectation", + guidance: + "The action ran but the result was not what the workflow asserted. Check the target system: if the outcome is actually acceptable, skip the check; if not, the run did the wrong thing and should be reversed.", + retryUseful: false, + }, + OUTCOME_UNKNOWN: { + owner: "operator", + headline: "Effect unknown — may already have happened", + guidance: + "The step started and never reported back. Ghost will not guess. Confirm in the target system whether it took effect before deciding: retrying could repeat it.", + retryUseful: false, + }, + APPROVAL_EXPIRED: { + owner: "administrator", + headline: "Approval expired before use", + guidance: + "Someone approved this step but the run could not consume it in time. A fresh approval is required — the expired one cannot be reused.", + retryUseful: false, + }, + RESTORE_UNSAFE: { + owner: "operator", + headline: "Could not rebuild page state", + guidance: + "Resuming would have meant guessing at browser state, so the run stopped instead. Start a fresh run rather than retrying from here.", + retryUseful: false, + }, + DATA: { + owner: "operator", + headline: "Value rejected by the target", + guidance: + "The target system refused a value this run supplied. Retrying sends the same value — correct the input or the extracting step first.", + retryUseful: false, + }, + UNKNOWN: { + owner: "operator", + headline: "Unclassified failure", + guidance: + "Ghost could not categorize this failure. Read the error and the screenshot before acting; treat the step's effect as uncertain.", + retryUseful: false, + }, +}; + +export interface ClassifyExceptionInput { + /** The incident reason recorded on the run. */ + reason: string; + /** The step the run stopped on, when it is known. */ + step?: WorkflowStep; + /** + * The recorded outcome of that step, when known. + * + * `"UNKNOWN"` is authoritative and overrides the reason text: the engine only + * writes it after deciding an action's effect is genuinely indeterminate. + */ + recordedOutcome?: "UNKNOWN" | "FAILED" | null; +} + +export function classifyException( + input: ClassifyExceptionInput, +): ExceptionDisposition { + const kind = classifyKind(input); + const base = DISPOSITIONS[kind]; + + // Duplicate risk is the conjunction of two independent facts: the engine does + // not know whether the effect happened, AND the step is one whose effect + // reaches outside the browser. An indeterminate `verify` or `extract` is a + // read — repeating it costs nothing — so it must not carry the same warning + // as an indeterminate payment, or the warning stops meaning anything. + // + // Composing `replaySafety` here rather than re-listing step types keeps one + // definition of "mutating" across restoration and incident recovery. + const mutating = input.step ? replaySafety(input.step) === "mutating" : true; + const retryMayDuplicate = kind === "OUTCOME_UNKNOWN" && mutating; + + return { kind, ...base, retryMayDuplicate }; +} + +function classifyKind(input: ClassifyExceptionInput): ExceptionKind { + // Tier 1a: the recorded outcome. The engine already made this judgement with + // more context than a string match will ever have. + if (input.recordedOutcome === "UNKNOWN") return "OUTCOME_UNKNOWN"; + + const reason = input.reason ?? ""; + + // Tier 1b: Ghost's own structured prefixes. + for (const [prefix, kind] of PREFIX_KINDS) { + if (reason.startsWith(prefix)) return kind; + } + + // Phrases the engine writes without a prefix. Matched as whole phrases rather + // than keywords so an unrelated error mentioning "approval" is not swept up. + if (/^approval for step \d+ expired/i.test(reason)) return "APPROVAL_EXPIRED"; + if (/^verification failed at step \d+/i.test(reason)) return "VERIFICATION"; + + // "exhausted its retries" is the wrapper the step loop uses when it has no + // better message; on its own it says nothing about *why*, so it must not + // short-circuit the text rules below — the underlying driver error, when the + // loop managed to capture one, is the informative part. + const bare = /^step exhausted its retries$/i.test(reason); + if (bare) return "UNKNOWN"; + + // Tier 2: best-effort over third-party text. + for (const [pattern, kind] of TEXT_RULES) { + if (pattern.test(reason)) return kind; + } + + return "UNKNOWN"; +} + +/** Every kind, for exhaustive UI rendering and tests. */ +export const EXCEPTION_KINDS = Object.keys( + DISPOSITIONS, +) as ReadonlyArray; + +/** True when this kind is a defect in the workflow rather than in the world. */ +export function needsAuthoring(kind: ExceptionKind): boolean { + return DISPOSITIONS[kind].owner === "author"; +} From a10efd9f9998d58fe5b4e20ba2b9c7cc75ea52ea Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 18:01:41 +0000 Subject: [PATCH 3/5] fix(cloud): address Codex review on exception routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven of twelve findings were correct. Fixes, grouped by what was actually wrong: Fail closed on the gate's own input. The retry path read the recorded step outcome through `.catch(() => null)`, so a transient database error became "nothing to worry about" — the least restrictive answer available, on the single query that decides whether a possibly-completed payment needs acknowledgement. A lookup failure now returns 503 and refuses the retry instead of allowing it. Duplicate risk was being recomputed inline at three call sites as `disposition.retryMayDuplicate || kind === "OUTCOME_UNKNOWN" || recorded === "UNKNOWN"`. That reads as extra caution and was a bug: it forced the confirmation prompt on for an indeterminate *read* — a `verify` or `extract` — which the classifier deliberately reports as safe because repeating a read costs nothing. It contradicted the property the previous commit's own test pins. Collapsed into one exported `duplicateRiskFor` that unions the live verdict with the stored label and then still requires the step to be mutating per `replaySafety`. Semantic locators were misrouted. `selector.ts` resolves every preferred selector through `getByRole`/`getByTestId`/`getByText`, so Playwright's call log reads "waiting for getByRole(...)" and the word "locator" never appears. The TARGET_MISSING rule matched only /locator|selector/, so exactly the selectors Ghost prefers fell through to the generic timeout rule and were routed to an operator as transient — told to retry a step that needs re-authoring. The whole getBy* family is now matched. Assignment was not atomic. State change and audit append are now one transaction, the pattern the members route already uses. Ordering did not measure what it claimed. The queue sorted on `Run.createdAt`, so a week-old run failing now outranked yesterday's stuck run, and "stopped 6d ago" meant "the run is 6 days old." Adds `Run.incidentRaisedAt`, stamped at every INCIDENT transition, ordered and displayed on. Backfilled to NULL rather than createdAt — guessing would assert the exact misreading the column removes. Lifecycle leaks. Routing fields were cleared only by retry/skip: undo left them set through COMPENSATING, so a later reversal failure inherited the forward incident's kind, owner and wait time. Compensation's four INCIDENT transitions now re-route from scratch, and undo clears. Removing someone from an org unassigns their exceptions — the FK's SET NULL fires only on account deletion, not membership removal, so their name stayed on work they could no longer act on and vanished from every member's queue. Filters were unsound. `kind` filtered in SQL dropped every null-kind row before fallback classification could see it, and `owner` was applied after the 200-row cap, so ?owner=author could report zero while author-owned exceptions sat just past the cap. Owner is a pure function of kind, so it now becomes an IN list in SQL via `kindsForOwner`; both filters admit nulls and post-filter after classification. Docs. The comparison table claimed Ghost executes "browser + API" while `driver.ts` lists apiCall/sendEmail in UNIMPLEMENTED_ACTION_TYPES — rule 10, corrected to browser-only with the gap stated plainly. Adds the exception-routing contract to trust-pipeline.md and CURSOR_HANDOFF.md, which the previous commit changed behavior without documenting. Declined: nothing. The twelfth comment was Codex's own summary. Validation: typecheck, lint, build clean; 530 tests across 58 files against real Postgres + Redis (up from 515 — 13 classifier, 2 route). Migration applies from scratch, `prisma migrate diff` reports no drift. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1 --- .../web/src/app/(app)/exceptions/page.tsx | 22 +++-- .../apps/web/src/app/api/exceptions/route.ts | 64 ++++++++++--- .../[id]/incident/incident-routing.test.ts | 67 +++++++++++++ .../src/app/api/runs/[id]/incident/route.ts | 75 ++++++++++++--- .../web/src/app/api/runs/[id]/undo/route.ts | 13 ++- .../api/settings/members/[userId]/route.ts | 10 ++ cloud/apps/web/src/lib/run-view.ts | 18 +++- cloud/apps/worker/src/jobs/compensateRun.ts | 47 ++++++++++ cloud/apps/worker/src/jobs/runWorkflow.ts | 8 +- cloud/docs/CURSOR_HANDOFF.md | 37 ++++++++ .../migration.sql | 18 ++++ cloud/packages/core/prisma/schema.prisma | 18 +++- .../core/src/classifier/exception.test.ts | 93 +++++++++++++++++++ .../packages/core/src/classifier/exception.ts | 62 ++++++++++++- docs/competitive-landscape.md | 14 ++- docs/trust-pipeline.md | 39 ++++++++ 16 files changed, 559 insertions(+), 46 deletions(-) create mode 100644 cloud/packages/core/prisma/migrations/20260817175000_incident_raised_at/migration.sql diff --git a/cloud/apps/web/src/app/(app)/exceptions/page.tsx b/cloud/apps/web/src/app/(app)/exceptions/page.tsx index 76d8889..6a73b7f 100644 --- a/cloud/apps/web/src/app/(app)/exceptions/page.tsx +++ b/cloud/apps/web/src/app/(app)/exceptions/page.tsx @@ -5,6 +5,7 @@ import { Card, CardBody } from "@/components/ui/card"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyException, + duplicateRiskFor, type ExceptionKind, type ExceptionOwner, } from "@ghost/core/classifier/exception"; @@ -72,8 +73,9 @@ export default async function ExceptionsPage() { const runs = orgId ? await prisma.run.findMany({ where: { orgId, status: "INCIDENT" }, - // Oldest first: a parked run goes stale, it does not improve with age. - orderBy: { createdAt: "asc" }, + // Longest-parked first, on when the incident was raised — a run's own age + // says nothing about how long it has been waiting for someone. + orderBy: [{ incidentRaisedAt: "asc" }, { createdAt: "asc" }], take: 200, include: { workflowVersion: { @@ -140,14 +142,16 @@ export default async function ExceptionsPage() { owner: disposition.owner, headline: disposition.headline, guidance: disposition.guidance, - retryMayDuplicate: - disposition.retryMayDuplicate || - kind === "OUTCOME_UNKNOWN" || - recorded?.status === "UNKNOWN", + retryMayDuplicate: duplicateRiskFor({ + disposition, + storedKind: kind, + recordedOutcome: recorded?.status === "UNKNOWN" ? "UNKNOWN" : null, + step, + }), stepLabel: recorded?.label ?? step?.label ?? step?.type ?? null, - // When it stopped, from the step — `Run.endedAt` stays null for an - // INCIDENT because the run is not terminal. - stoppedAt: recorded?.endedAt ?? run.createdAt, + // When the incident was raised. `Run.endedAt` stays null for an INCIDENT + // (it is not terminal), and `createdAt` is the run's age, not its wait. + stoppedAt: run.incidentRaisedAt ?? recorded?.endedAt ?? run.createdAt, }; }); diff --git a/cloud/apps/web/src/app/api/exceptions/route.ts b/cloud/apps/web/src/app/api/exceptions/route.ts index 529803b..e6c08c1 100644 --- a/cloud/apps/web/src/app/api/exceptions/route.ts +++ b/cloud/apps/web/src/app/api/exceptions/route.ts @@ -4,6 +4,9 @@ import { prisma } from "@/lib/db"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyException, + duplicateRiskFor, + kindsForOwner, + EXCEPTION_KINDS, type ExceptionKind, type ExceptionOwner, } from "@ghost/core/classifier/exception"; @@ -70,7 +73,27 @@ export async function GET(req: Request) { // `mine=1` narrows to the caller's own assignments — the "my work" view. const mine = url.searchParams.get("mine") === "1"; const kindFilter = url.searchParams.get("kind"); - const ownerFilter = url.searchParams.get("owner"); + const ownerFilter = url.searchParams.get("owner") as ExceptionOwner | null; + + // Both filters are pushed into SQL, and both must admit `incidentKind: null`. + // + // Two bugs live here otherwise. Filtering `incidentKind = 'X'` in SQL silently + // drops every incident whose kind was never stored — rows from before the + // column existed — so those never reach the fallback classification below and + // `?kind=UNKNOWN` omits exactly the incidents it should return. And applying an + // *owner* filter after `take` means the cap selects the 200 longest-parked + // incidents first: if those are all operator-owned, `?owner=author` returns + // nothing while author-owned exceptions sit just past the cap, reporting a + // `total` of 0 that reads as "none exist". + // + // `owner` is a pure function of `kind` (see kindsForOwner), so it becomes an IN + // list rather than needing a stored owner column. Null-kind rows are admitted + // here and filtered after classification. + const wantedKinds: ExceptionKind[] | null = ownerFilter + ? kindsForOwner(ownerFilter).filter((k) => !kindFilter || k === kindFilter) + : kindFilter && (EXCEPTION_KINDS as readonly string[]).includes(kindFilter) + ? [kindFilter as ExceptionKind] + : null; const runs = await prisma.run.findMany({ where: { @@ -79,10 +102,13 @@ export async function GET(req: Request) { ...(mine && session.user.id ? { incidentAssigneeId: session.user.id } : {}), - ...(kindFilter ? { incidentKind: kindFilter } : {}), + ...(wantedKinds + ? { OR: [{ incidentKind: { in: wantedKinds } }, { incidentKind: null }] } + : {}), }, - // Oldest first — see the module comment. - orderBy: { createdAt: "asc" }, + // Longest-parked first, on when the incident was raised rather than when the + // run was created — see the module comment. + orderBy: [{ incidentRaisedAt: "asc" }, { createdAt: "asc" }], take: MAX_ROWS, include: { workflowVersion: { include: { workflow: { select: { name: true } } } }, @@ -144,15 +170,18 @@ export async function GET(req: Request) { : null, }); - // Trust the stored kind for the label, but take the *safety* flag from the - // live computation. If the two ever disagree about duplicate risk, the - // cautious answer wins: a stale stored kind must not be able to downgrade a - // warning that the current classifier would raise. + // Trust the stored kind for the label — it is what the engine decided when + // the run stopped. Duplicate risk comes from the shared helper, which unions + // the live verdict with the stored label and then still requires the step to + // reach outside the browser, so an indeterminate *read* is not flagged as a + // possibly-repeated effect. kind = kind ?? disposition.kind; - const retryMayDuplicate = - disposition.retryMayDuplicate || - kind === "OUTCOME_UNKNOWN" || - recorded?.status === "UNKNOWN"; + const retryMayDuplicate = duplicateRiskFor({ + disposition, + storedKind: kind, + recordedOutcome: recorded?.status === "UNKNOWN" ? "UNKNOWN" : null, + step, + }); const shaped: ExceptionRow = { runId: run.id, @@ -167,7 +196,13 @@ export async function GET(req: Request) { retryUseful: disposition.retryUseful, retryMayDuplicate, error: run.error, - stoppedAt: (recorded?.endedAt ?? run.createdAt).toISOString(), + // Prefer the recorded incident time; fall back to the step's end, then the + // run's creation for rows predating `incidentRaisedAt`. + stoppedAt: ( + run.incidentRaisedAt ?? + recorded?.endedAt ?? + run.createdAt + ).toISOString(), assignee: run.incidentAssignee ? { id: run.incidentAssignee.id, @@ -180,7 +215,10 @@ export async function GET(req: Request) { : null, }; + // Post-filter, needed only for the null-kind rows admitted above: their kind + // — and therefore their owner — is not known until classification. if (ownerFilter && shaped.owner !== ownerFilter) continue; + if (kindFilter && shaped.kind !== kindFilter) continue; rows.push(shaped); } diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts index 625c21f..2a06a5b 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts @@ -123,6 +123,7 @@ describe.skipIf(!hasDb)("incident routing (Postgres)", () => { cursor: 1, error: opts.error, incidentKind: opts.incidentKind, + incidentRaisedAt: new Date(), triggeredById: userId, steps: { create: { @@ -318,6 +319,72 @@ describe.skipIf(!hasDb)("incident routing (Postgres)", () => { expect(run?.incidentAssigneeId).toBeNull(); }); + it("does not warn about duplicate effects for an indeterminate read", async () => { + // A `verify` whose outcome is unknown is a read: repeating it costs nothing. + // Demanding acknowledgement here would train operators to click through the + // prompt that exists for payments. + const wf = await prisma.workflow.create({ + data: { + orgId, + name: `wf-${Math.random().toString(36).slice(2, 8)}`, + versions: { + create: { + version: 1, + steps: [ + { id: "nav", type: "navigate", url: "https://example.com" }, + { + id: "chk", + type: "verify", + assertion: { kind: "textPresent", expected: "Paid" }, + }, + ] as never, + }, + }, + }, + include: { versions: true }, + }); + const run = await prisma.run.create({ + data: { + orgId, + workflowVersionId: wf.versions[0]!.id, + status: "INCIDENT", + cursor: 1, + error: "OUTCOME_UNKNOWN: step 1 may or may not have taken effect", + incidentKind: "OUTCOME_UNKNOWN", + incidentRaisedAt: new Date(), + triggeredById: userId, + steps: { + create: { + index: 1, + type: "verify", + status: "UNKNOWN", + label: "Paid?", + }, + }, + }, + }); + + const res = await post(run.id, { action: "retry" }); + expect(res.status).toBe(200); + }); + + it("clears incidentRaisedAt when the incident resolves", async () => { + const runId = await incidentRun({ + stepStatus: "FAILED", + error: "net::ERR_CONNECTION_RESET", + incidentKind: "TRANSIENT", + }); + const before = await prisma.run.findUnique({ where: { id: runId } }); + expect(before?.incidentRaisedAt).not.toBeNull(); + + await post(runId, { action: "retry" }); + + // Leaving INCIDENT clears the parked-since stamp, or the next incident on + // this run would inherit an age from the previous one. + const after = await prisma.run.findUnique({ where: { id: runId } }); + expect(after?.incidentRaisedAt).toBeNull(); + }); + it("rejects an unknown action", async () => { const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); const res = await post(runId, { action: "resolve" }); diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts index 365666e..217223e 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts @@ -6,7 +6,7 @@ import { appendAuditEvent, appendRunEvent } from "@ghost/core/audit-log"; import { RUN_EVENT_TYPES } from "@ghost/core/run-events"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyStep } from "@ghost/core/classifier"; -import { classifyException } from "@ghost/core/classifier/exception"; +import { classifyException, duplicateRiskFor } from "@ghost/core/classifier/exception"; /** * Resolve an incident: retry the failed step, or skip it. @@ -91,12 +91,23 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str assigneeId = member.userId; } - await prisma.run.update({ where: { id }, data: { incidentAssigneeId: assigneeId } }); - await appendAuditEvent(orgId, userId, { - action: assigneeId ? "run.incident_assigned" : "run.incident_unassigned", - entityType: "Run", - entityId: id, - metadata: { stepIndex: run.cursor, assigneeId }, + // One transaction: a routing mutation that applied but was never recorded + // would leave the queue showing an owner with no audit trail explaining how + // they got it, while the caller was told the request failed. Either both + // land or neither does. `appendAuditEvent` takes the tx for exactly this. + await prisma.$transaction(async (tx) => { + await tx.run.update({ where: { id }, data: { incidentAssigneeId: assigneeId } }); + await appendAuditEvent( + orgId, + userId, + { + action: assigneeId ? "run.incident_assigned" : "run.incident_unassigned", + entityType: "Run", + entityId: id, + metadata: { stepIndex: run.cursor, assigneeId }, + }, + tx, + ); }); return NextResponse.json({ ok: true, assigneeId }); } @@ -173,6 +184,7 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str cursor: index + 1, incidentKind: null, incidentAssigneeId: null, + incidentRaisedAt: null, }, }); const { seq } = await appendRunEvent( @@ -197,9 +209,29 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // before, for any caller that says it understands the risk. What changes is // that it cannot happen *accidentally*, and the record shows the warning was // shown. - const recorded = await prisma.runStep - .findUnique({ where: { runId_index: { runId: id, index } }, select: { status: true } }) - .catch(() => null); + // Deliberately NOT `.catch(() => null)`. The recorded outcome is the signal + // that decides whether this retry needs an acknowledgement, so swallowing a + // transient database error would turn "I could not find out" into "there is + // nothing to worry about" — the least restrictive answer available, on the + // one query where being wrong repeats a payment. A lookup failure means the + // gate cannot be evaluated, so the retry is refused rather than allowed. + let recorded: { status: string } | null; + try { + recorded = await prisma.runStep.findUnique({ + where: { runId_index: { runId: id, index } }, + select: { status: true }, + }); + } catch { + return NextResponse.json( + { + error: + "could not determine whether this step's effect is known, so the retry was refused. " + + "This is a transient storage error — try again.", + }, + { status: 503 }, + ); + } + const disposition = classifyException({ reason: run.error ?? "", step, @@ -207,7 +239,16 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str recorded?.status === "UNKNOWN" ? "UNKNOWN" : recorded?.status === "FAILED" ? "FAILED" : null, }); - if (disposition.retryMayDuplicate && body.acknowledgeDuplicateRisk !== true) { + // Union of the live verdict and the stored label, still gated on the step + // actually reaching outside the browser. See duplicateRiskFor. + const mayDuplicate = duplicateRiskFor({ + disposition, + storedKind: run.incidentKind, + recordedOutcome: recorded?.status === "UNKNOWN" ? "UNKNOWN" : null, + step, + }); + + if (mayDuplicate && body.acknowledgeDuplicateRisk !== true) { return NextResponse.json( { error: @@ -220,7 +261,7 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str { status: 409 }, ); } - acknowledgedDuplicateRisk = disposition.retryMayDuplicate; + acknowledgedDuplicateRisk = mayDuplicate; // Retry: clear the recorded failure so the journal fold stops reporting it. // The step's own `step.started`/`step.failed` history stays in the chain — @@ -233,7 +274,13 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str await tx.run.update({ where: { id }, // See the skip branch: leaving INCIDENT clears the routing fields. - data: { status: "QUEUED", error: null, incidentKind: null, incidentAssigneeId: null }, + data: { + status: "QUEUED", + error: null, + incidentKind: null, + incidentAssigneeId: null, + incidentRaisedAt: null, + }, }); const { seq } = await appendRunEvent( id, @@ -247,7 +294,7 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // Present only when the retry carried duplicate risk, so its // presence in the chain is itself the evidence of an informed // decision rather than a flag that is always there. - ...(disposition.retryMayDuplicate ? { acknowledgedDuplicateRisk: true } : {}), + ...(mayDuplicate ? { acknowledgedDuplicateRisk: true } : {}), }, }, tx, diff --git a/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts b/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts index 04dbe3b..021e9f7 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts @@ -144,7 +144,18 @@ export async function POST(_req: Request, { params }: { params: Promise<{ id: st // Conditional update: a double-clicked Undo enqueues once. const { count } = await prisma.run.updateMany({ where: { id, orgId, status: { in: [...REVERSIBLE_STATES] as never } }, - data: { status: "COMPENSATING", error: null, endedAt: null }, + // Clear the exception routing along with the error. A run reversing is no + // longer an open exception, so it must leave the queue; and if the reversal + // itself later fails, compensateRun raises a *fresh* incident with its own + // kind, owner and timestamp rather than silently inheriting these. + data: { + status: "COMPENSATING", + error: null, + endedAt: null, + incidentKind: null, + incidentAssigneeId: null, + incidentRaisedAt: null, + }, }); if (count !== 1) { return NextResponse.json({ error: "run is already being reversed" }, { status: 409 }); diff --git a/cloud/apps/web/src/app/api/settings/members/[userId]/route.ts b/cloud/apps/web/src/app/api/settings/members/[userId]/route.ts index 863996c..3a9d207 100644 --- a/cloud/apps/web/src/app/api/settings/members/[userId]/route.ts +++ b/cloud/apps/web/src/app/api/settings/members/[userId]/route.ts @@ -124,6 +124,16 @@ export async function DELETE(_req: Request, { params }: Params): Promise): Promise undefined); @@ -143,6 +183,9 @@ export async function compensateRunJob(job: Job): Promise undefined); @@ -189,6 +232,9 @@ export async function compensateRunJob(job: Job): Promise { }); }); +describe("semantic Playwright locators route to the author, not to retry", () => { + // apps/worker/src/browser/selector.ts resolves every *preferred* selector + // through getByRole/getByTestId/getByText, and Playwright's call log then says + // "waiting for getByRole(...)" — the word "locator" never appears. Matching + // only /locator|selector/ missed exactly the selectors Ghost prefers and + // routed a changed page to an operator as a transient blip. + it.each([ + ['Timeout 30000ms exceeded.\nCall log:\n - waiting for getByRole(\'button\', { name: \'Pay\' })', "getByRole"], + ['Timeout 30000ms exceeded.\nCall log:\n - waiting for getByTestId(\'submit\')', "getByTestId"], + ['Timeout 30000ms exceeded.\nCall log:\n - waiting for getByText(\'Continue\')', "getByText"], + ['Timeout 5000ms exceeded.\nCall log:\n - waiting for getByLabel(\'Amount\')', "getByLabel"], + ['Timeout 5000ms exceeded.\nCall log:\n - waiting for getByPlaceholder(\'Search\')', "getByPlaceholder"], + ])("files a %s timeout as TARGET_MISSING", (reason) => { + expect(kindOf(reason, click)).toBe("TARGET_MISSING"); + }); + + it("routes those to the author rather than telling an operator to retry", () => { + const d = classifyException({ + reason: "Timeout 30000ms exceeded.\nCall log:\n - waiting for getByRole('button')", + step: click, + }); + expect(d.owner).toBe("author"); + expect(d.retryUseful).toBe(false); + }); +}); + +describe("duplicateRiskFor", () => { + it("is false when nothing is indeterminate", () => { + expect( + duplicateRiskFor({ + disposition: classifyException({ reason: "net::ERR_CONNECTION_RESET", step: click }), + storedKind: "TRANSIENT", + recordedOutcome: "FAILED", + step: click, + }), + ).toBe(false); + }); + + it("is true for an indeterminate mutating step", () => { + expect( + duplicateRiskFor({ storedKind: "OUTCOME_UNKNOWN", step: click }), + ).toBe(true); + }); + + it("stays FALSE for an indeterminate read, however the signal arrives", () => { + // The regression this helper exists for: three call sites each wrote + // `kind === "OUTCOME_UNKNOWN" || recorded === "UNKNOWN"`, which forced the + // duplicate-effect prompt on for a `verify` — a read that costs nothing to + // repeat. A prompt that fires on reads gets clicked through. + expect(duplicateRiskFor({ storedKind: "OUTCOME_UNKNOWN", step: verify })).toBe(false); + expect(duplicateRiskFor({ recordedOutcome: "UNKNOWN", step: verify })).toBe(false); + expect( + duplicateRiskFor({ + disposition: classifyException({ reason: "OUTCOME_UNKNOWN: ?", step: verify }), + step: verify, + }), + ).toBe(false); + }); + + it("lets a stale stored kind raise, but never lower, the risk", () => { + // Stored label says benign, recorded outcome says indeterminate: the union + // must still warn. + expect( + duplicateRiskFor({ storedKind: "TRANSIENT", recordedOutcome: "UNKNOWN", step: click }), + ).toBe(true); + // And the reverse — a stale OUTCOME_UNKNOWN label on a read stays quiet, + // because the step, not the label, decides whether an effect could repeat. + expect( + duplicateRiskFor({ storedKind: "OUTCOME_UNKNOWN", recordedOutcome: "FAILED", step: verify }), + ).toBe(false); + }); + + it("assumes risk when the step is unknown", () => { + expect(duplicateRiskFor({ storedKind: "OUTCOME_UNKNOWN" })).toBe(true); + }); +}); + +describe("kindsForOwner", () => { + it("partitions every kind across the three desks exactly once", () => { + const all = (["operator", "author", "administrator"] as const).flatMap((o) => kindsForOwner(o)); + expect(all.sort()).toEqual([...EXCEPTION_KINDS].sort()); + }); + + it("puts target changes on the author's desk", () => { + expect(kindsForOwner("author")).toContain("TARGET_MISSING"); + expect(kindsForOwner("administrator")).toEqual( + expect.arrayContaining(["AUTH", "APPROVAL_EXPIRED"]), + ); + }); +}); + describe("classifyException — fails closed", () => { it("leaves an unrecognized reason UNKNOWN rather than guessing", () => { const d = classifyException({ diff --git a/cloud/packages/core/src/classifier/exception.ts b/cloud/packages/core/src/classifier/exception.ts index 62918e3..703a67a 100644 --- a/cloud/packages/core/src/classifier/exception.ts +++ b/cloud/packages/core/src/classifier/exception.ts @@ -118,8 +118,18 @@ const TEXT_RULES: ReadonlyArray = [ // Playwright's locator failures. "strict mode violation" is the opposite // problem — too many matches — but lands on the same desk: the selector no // longer identifies one thing, so the workflow needs re-authoring. + // + // The `getBy*` forms are not optional extras: apps/worker/src/browser/selector.ts + // resolves every *preferred* selector through `getByRole`/`getByTestId`/ + // `getByText`, and Playwright's call log then reads "waiting for + // getByRole(...)" — the word "locator" never appears. Matching only + // `locator|selector` therefore missed exactly the selectors Ghost prefers, + // dropping them through to the generic timeout rule below and routing a + // changed page to an operator as a transient blip to retry forever. The rest + // of the family is listed too, so a workflow that starts using them is not a + // silent regression. [ - /waiting for (?:locator|selector)|strict mode violation|no element matches/i, + /waiting for (?:locator|selector|getBy(?:Role|TestId|Text|Label|Placeholder|AltText|Title))|strict mode violation|no element matches/i, "TARGET_MISSING", ], [ @@ -294,6 +304,44 @@ function classifyKind(input: ClassifyExceptionInput): ExceptionKind { return "UNKNOWN"; } +/** + * Duplicate-effect risk for a stopped step, as a standalone decision. + * + * Exported because three read paths (the retry gate, the run timeline, and the + * exception queue) all need it, and each had been re-deriving it as + * `disposition.retryMayDuplicate || kind === "OUTCOME_UNKNOWN" || recorded === + * "UNKNOWN"`. That looked like belt-and-braces caution and was actually a bug: + * it forced the warning on for an indeterminate **read** — a `verify` or + * `extract` — which `classifyException` deliberately reports as safe, because + * repeating a read costs nothing. A confirmation prompt that fires on reads is + * one operators learn to click through, which is precisely how the prompt stops + * protecting the payment it exists for. + * + * The caution those callers wanted is real, though: a *stored* `incidentKind` + * must never be able to talk the risk down. So this takes the union of the + * live verdict and the stored label, and then — the part the callers dropped — + * still requires the step to actually reach outside the browser. + * + * Fails closed: an unknown step is assumed mutating. + */ +export function duplicateRiskFor(input: { + /** Live classifier verdict, when already computed. */ + disposition?: ExceptionDisposition; + /** The stored `Run.incidentKind`, which may be stale or absent. */ + storedKind?: ExceptionKind | string | null; + /** Recorded outcome of the stopped step. */ + recordedOutcome?: "UNKNOWN" | "FAILED" | null; + /** The stopped-on step, when known. */ + step?: WorkflowStep; +}): boolean { + const indeterminate = + input.disposition?.kind === "OUTCOME_UNKNOWN" || + input.storedKind === "OUTCOME_UNKNOWN" || + input.recordedOutcome === "UNKNOWN"; + if (!indeterminate) return false; + return input.step ? replaySafety(input.step) === "mutating" : true; +} + /** Every kind, for exhaustive UI rendering and tests. */ export const EXCEPTION_KINDS = Object.keys( DISPOSITIONS, @@ -303,3 +351,15 @@ export const EXCEPTION_KINDS = Object.keys( export function needsAuthoring(kind: ExceptionKind): boolean { return DISPOSITIONS[kind].owner === "author"; } + +/** + * Every kind that routes to a given desk. + * + * `owner` is a pure function of `kind`, so an owner filter can be pushed into + * SQL as `incidentKind IN (...)` instead of being applied to an + * already-capped page of rows — which silently returned "no author-owned + * exceptions" whenever the oldest N happened to be operator-owned. + */ +export function kindsForOwner(owner: ExceptionOwner): ExceptionKind[] { + return EXCEPTION_KINDS.filter((k) => DISPOSITIONS[k].owner === owner); +} diff --git a/docs/competitive-landscape.md b/docs/competitive-landscape.md index 8f5c859..4de2e58 100644 --- a/docs/competitive-landscape.md +++ b/docs/competitive-landscape.md @@ -70,7 +70,7 @@ Set against Ghost's pipeline, the overlap is thin and the gap is structural: | Persistent work memory + Q&A | **Yes**, local | No | | Scheduled recurring prompts | **Yes** ("Routines") | **No trigger surface at all** | | Records a demonstrated workflow | Observes, doesn't compile to steps | Yes — Phase 2 Chrome extension → typed steps | -| Executes multi-step work across apps | Limited, via email/calendar connectors | **Yes** — browser + API, typed steps | +| Executes multi-step work across apps | Limited, via email/calendar connectors | **Browser only** — typed steps; `apiCall`/`sendEmail` are declared but unimplemented ([`driver.ts`](../cloud/apps/worker/src/browser/driver.ts) `UNIMPLEMENTED_ACTION_TYPES`) | | Deny-by-default gate on send/pay/delete | Not evidenced | **Yes** — deterministic `classifyStep` | | Human approval before sensitive actions | Not evidenced | **Yes**, with expiry, single-use | | Per-step outcome verification | No | **Yes** | @@ -79,12 +79,22 @@ Set against Ghost's pipeline, the overlap is thin and the gap is structural: | Multi-tenant org isolation, RBAC | Single-user Mac app | **Yes** | | SOC 2 | **Yes** | **No** | | Self-serve free tier | **Yes** | **No** — implementation-led | -| Platform | macOS only | Cloud (browser + API) | +| Platform | macOS only | Cloud (browser; API execution is future work) | Ghost wins every row that describes *doing the work under controls*. Littlebird wins every row that describes *knowing about the work* — plus, notably, the two rows that are about being a real company you can buy from today: SOC 2 and a free tier. +One correction to Ghost's own column, because rule 10 cuts both ways. Ghost's execution +surface today is **the browser only**. `apiCall` and `sendEmail` exist in the step schema, +are classified by the approval gate, and are classified for replay safety — but their +executors are no-ops listed in `UNIMPLEMENTED_ACTION_TYPES`, and a guard keeps them out +of the step editor so nobody can author a step that silently does nothing. So Ghost +cannot currently send an email or call an API, and the strategy's "APIs first, then +browser" ordering describes the intended preference, not shipped behavior. That gap is +the single largest one in this comparison, and it is not in Littlebird's favour — it is +in the favour of any connector-backed automation platform. + They are also not competing for the same dollar. $17/mo personal productivity versus $500–2,000/mo team plus $2.5k–15k implementation are different budgets, different buyers, and different sales motions. diff --git a/docs/trust-pipeline.md b/docs/trust-pipeline.md index 5766eab..d60bcf9 100644 --- a/docs/trust-pipeline.md +++ b/docs/trust-pipeline.md @@ -48,6 +48,45 @@ Compensation has known gaps, listed in `cloud/docs/CURSOR_HANDOFF.md` under "Known gaps in compensation". The one that limits what it is useful for today: reversals run in a fresh, unauthenticated browser context. +## Exception routing + +A run that stops does not just sit in `INCIDENT` waiting to be found by run id. +The stop is **classified when it is raised** by a third deterministic classifier, +`classifyException` — sibling to `classifyStep` (must a human approve this?) and +`replaySafety` (is it safe to re-apply silently?). It answers a different +question: *this stopped — whose desk, and is retrying safe?* + +Nine kinds each route to one **owner**, and the owner is the point: a changed +selector (`TARGET_MISSING`) belongs to whoever maintains the workflow, an expired +credential (`AUTH`) or a lapsed gate (`APPROVAL_EXPIRED`) to an administrator, a +rejected value (`DATA`) or a wrong outcome (`VERIFICATION`) to the operator +running the work. "A run stopped" is not actionable; "the portal changed and step +4 needs re-recording" is. + +Like its siblings it is a rule table, not a model call, and it fails closed: an +unrecognized reason stays `UNKNOWN` rather than being forced into the nearest +bucket, because a confidently wrong label defeats the purpose of routing. + +Two properties carry the trust weight: + +- **Classification never changes control flow.** It decides what a human is + *shown* and where the work lands. The state transition is identical either way. +- **`OUTCOME_UNKNOWN` retries require acknowledgement.** A step that started and + never reported back may already have taken effect. The engine still lets a + human retry it — only a person can check the target system — but the route + refuses an unacknowledged retry and records the acknowledgement in the run + journal and the audit log. Duplicate risk additionally requires the step to + reach *outside the browser*: an indeterminate `verify` is a read, and a prompt + that fires on reads is one operators learn to click through. + +Resolution (`retry` / `skip` / `assign`) stays on the run's own incident route +under its own authorization; the queue at `/exceptions` and `GET /api/exceptions` +is read-only. Assignment is open to any member — deciding who looks at a problem +is not authorizing the action that caused it — and is tenant-scoped to members of +the org that owns the run. Routing fields are cleared whenever a run leaves +`INCIDENT`, and a failed *reversal* is re-routed from scratch rather than +inheriting the forward incident's kind, owner, or waiting time. + ## Execution limits A workflow may cap how many of its runs are in flight at once From a96a28fd230ca41c1f657c3ce6685970511362f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 18:20:07 +0000 Subject: [PATCH 4/5] fix(cloud): address second Codex review on exception routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All seven findings were correct. Two are real double-execution paths. A failed verification on a mutating step now carries duplicate risk. This is the sharper of the two. When a click succeeds and its assertion does not, the engine records VERIFICATION — and the worker is careful within one attempt, re-running the assertion only, never the action. But an incident retry resets the step to PENDING and re-executes the whole step under the original approval. For a click on Pay that is a second payment with no warning. VERIFICATION is not uncertainty like OUTCOME_UNKNOWN; it is stronger, because there was something to assert precisely because the action landed. Both kinds now sit in EFFECT_MAY_HAVE_LANDED, still conjoined with replaySafety so an indeterminate read stays unflagged. A timed-out reversal is no longer read as safely retryable. The compensation incident classified from the raw message, so a reversal click that timed out — possibly after reaching the server — came back TRANSIENT, "just retry". Pressing Undo again would re-execute it under the still-valid approval with no acknowledgement: the same double-execution, reached from the reversal side. Now tracked with a flag set *before* applyStep, so an in-flight action counts; only a failure before the first action starts is classified from text. Display shaping took the kind from storage and every other field from a fresh classification. Those disagree immediately for compensation incidents, whose kind is asserted at the call site while their reason text carries no classifier prefix — so the queue rendered "OUTCOME_UNKNOWN" beside "Unclassified failure" guidance. Adds dispositionForKind; all three read paths now derive owner, headline, guidance and retry recommendation from the kind actually shown. Assignment had a TOCTOU. The status check was a read and the write was by id alone, so a retry/skip/cancel/undo landing in between assigned an owner to a run that was no longer an exception, reported success, and left a stale assignee to surface on the run's next incident. The update is now conditional inside the transaction and 409s when it matches nothing; raiseIncident also clears the assignee, so a fresh incident always starts unowned. Undo's rollback restored status, error and endedAt but not the routing fields it had just cleared, so a scheduling failure returned the run to INCIDENT stripped of its classification and owner — "restore exactly what was there" now means all of it. The stopped-step lookup used two independent IN predicates, which is a cross product: 200 runs at 200 distinct cursors could load 40,000 RunStep rows to shape 200 exceptions. Now an OR of {runId, index} pairs, in both the API and the server-rendered page. Docs: the comparison marked undo "designed, not built" while compensation ships with preview, approval-gated execution, audit events and retryable partial reversals — corrected with its real limitation rather than understated. Validation: typecheck, lint, build clean; 536 tests across 58 files against real Postgres + Redis (up from 530). One run showed an auditLog.test.ts failure that was Postgres dying mid-run in the sandbox, not a regression — re-run green after restarting it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1 --- .../web/src/app/(app)/exceptions/page.tsx | 17 +++-- .../apps/web/src/app/api/exceptions/route.ts | 30 +++++---- .../src/app/api/runs/[id]/incident/route.ts | 20 +++++- .../web/src/app/api/runs/[id]/undo/route.ts | 21 +++++- cloud/apps/web/src/lib/run-view.ts | 12 ++-- cloud/apps/worker/src/jobs/compensateRun.ts | 21 +++++- cloud/apps/worker/src/jobs/runWorkflow.ts | 5 ++ .../core/src/classifier/exception.test.ts | 64 +++++++++++++++++++ .../packages/core/src/classifier/exception.ts | 48 ++++++++++++-- docs/competitive-landscape.md | 2 +- 10 files changed, 209 insertions(+), 31 deletions(-) diff --git a/cloud/apps/web/src/app/(app)/exceptions/page.tsx b/cloud/apps/web/src/app/(app)/exceptions/page.tsx index 6a73b7f..e851b89 100644 --- a/cloud/apps/web/src/app/(app)/exceptions/page.tsx +++ b/cloud/apps/web/src/app/(app)/exceptions/page.tsx @@ -5,6 +5,7 @@ import { Card, CardBody } from "@/components/ui/card"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyException, + dispositionForKind, duplicateRiskFor, type ExceptionKind, type ExceptionOwner, @@ -91,10 +92,9 @@ export default async function ExceptionsPage() { const stepRows = runs.length > 0 ? await prisma.runStep.findMany({ - where: { - runId: { in: runs.map((r) => r.id) }, - index: { in: [...new Set(runs.map((r) => r.cursor))] }, - }, + // Paired, not two independent `IN`s — that is a cross product. See + // the exceptions API route. + where: { OR: runs.map((r) => ({ runId: r.id, index: r.cursor })) }, select: { runId: true, index: true, @@ -136,12 +136,15 @@ export default async function ExceptionsPage() { // should see; fall back to the live computation only for runs raised before // the column existed. Duplicate risk always takes the cautious answer. const kind = (run.incidentKind as ExceptionKind | null) ?? disposition.kind; + // Display fields come from the kind actually shown, never a mix of stored + // label and live classification — see the exceptions API route. + const shown = dispositionForKind(kind); return { run, kind, - owner: disposition.owner, - headline: disposition.headline, - guidance: disposition.guidance, + owner: shown.owner, + headline: shown.headline, + guidance: shown.guidance, retryMayDuplicate: duplicateRiskFor({ disposition, storedKind: kind, diff --git a/cloud/apps/web/src/app/api/exceptions/route.ts b/cloud/apps/web/src/app/api/exceptions/route.ts index e6c08c1..fcee4c6 100644 --- a/cloud/apps/web/src/app/api/exceptions/route.ts +++ b/cloud/apps/web/src/app/api/exceptions/route.ts @@ -4,6 +4,7 @@ import { prisma } from "@/lib/db"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyException, + dispositionForKind, duplicateRiskFor, kindsForOwner, EXCEPTION_KINDS, @@ -121,10 +122,13 @@ export async function GET(req: Request) { // per row. Prisma cannot filter a relation by a column of the parent row // (`index = run.cursor`), so the pairing is done here instead of in SQL — but // it is still a single round trip, which is the part that matters. + // + // Paired, not two independent `IN`s: `runId IN (…) AND index IN (…)` is a + // cross product, so 200 runs stopped at 200 distinct cursors would fetch up to + // 40,000 rows to shape 200 exceptions. const stepRows = await prisma.runStep.findMany({ where: { - runId: { in: runs.map((r) => r.id) }, - index: { in: [...new Set(runs.map((r) => r.cursor))] }, + OR: runs.map((r) => ({ runId: r.id, index: r.cursor })), }, select: { runId: true, @@ -170,12 +174,16 @@ export async function GET(req: Request) { : null, }); - // Trust the stored kind for the label — it is what the engine decided when - // the run stopped. Duplicate risk comes from the shared helper, which unions - // the live verdict with the stored label and then still requires the step to - // reach outside the browser, so an indeterminate *read* is not flagged as a - // possibly-repeated effect. + // The stored kind is what the engine decided when the run stopped, and it is + // what an auditor should see — so every *other* displayed field has to come + // from that same kind. Mixing a stored label with a freshly-classified + // owner/headline/guidance produced visible nonsense as soon as the two + // disagreed, which is immediately for compensation incidents: their kind is + // asserted at the call site while their reason text carries no prefix, so + // live classification says UNKNOWN and the row read "OUTCOME_UNKNOWN" beside + // "Unclassified failure". kind = kind ?? disposition.kind; + const shown = dispositionForKind(kind); const retryMayDuplicate = duplicateRiskFor({ disposition, storedKind: kind, @@ -190,10 +198,10 @@ export async function GET(req: Request) { stepIndex: run.cursor, stepLabel: recorded?.label ?? step?.label ?? step?.type ?? null, kind, - owner: disposition.owner, - headline: disposition.headline, - guidance: disposition.guidance, - retryUseful: disposition.retryUseful, + owner: shown.owner, + headline: shown.headline, + guidance: shown.guidance, + retryUseful: shown.retryUseful, retryMayDuplicate, error: run.error, // Prefer the recorded incident time; fall back to the step's end, then the diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts index 217223e..e3575f8 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts @@ -95,8 +95,20 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // would leave the queue showing an owner with no audit trail explaining how // they got it, while the caller was told the request failed. Either both // land or neither does. `appendAuditEvent` takes the tx for exactly this. + // Conditional on the run still being an INCIDENT, inside the transaction. + // The `findFirst` above is a read: a retry, skip, cancel or undo can resolve + // the incident between it and this write, and an id-only update would then + // assign an owner to a run that is no longer an exception — reporting + // success, and leaving a stale assignee to surface on the run's *next* + // incident. + let assigned = 0; await prisma.$transaction(async (tx) => { - await tx.run.update({ where: { id }, data: { incidentAssigneeId: assigneeId } }); + const res = await tx.run.updateMany({ + where: { id, orgId, status: "INCIDENT" }, + data: { incidentAssigneeId: assigneeId }, + }); + assigned = res.count; + if (assigned !== 1) return; await appendAuditEvent( orgId, userId, @@ -109,6 +121,12 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str tx, ); }); + if (assigned !== 1) { + return NextResponse.json( + { error: "this run is no longer an open exception" }, + { status: 409 }, + ); + } return NextResponse.json({ ok: true, assigneeId }); } diff --git a/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts b/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts index 021e9f7..dc13817 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts @@ -141,6 +141,16 @@ export async function POST(_req: Request, { params }: { params: Promise<{ id: st ); } + // Snapshot the routing fields before clearing them. The catch below promises + // to give the claim back if scheduling fails, and a run returned to INCIDENT + // without its classification and owner has silently lost work — the exception + // reappears in the queue unassigned and labelled "Unclassified failure". + const priorRouting = { + incidentKind: run.incidentKind, + incidentAssigneeId: run.incidentAssigneeId, + incidentRaisedAt: run.incidentRaisedAt, + }; + // Conditional update: a double-clicked Undo enqueues once. const { count } = await prisma.run.updateMany({ where: { id, orgId, status: { in: [...REVERSIBLE_STATES] as never } }, @@ -192,7 +202,16 @@ export async function POST(_req: Request, { params }: { params: Promise<{ id: st await prisma.run .updateMany({ where: { id, orgId, status: "COMPENSATING" }, - data: { status: run.status, error: run.error, endedAt: run.endedAt }, + // Including the routing fields the update above cleared. Restoring only + // status/error/endedAt returned an INCIDENT run to the queue stripped of + // its classification and its owner — "restore exactly what was there" + // has to mean all of it. + data: { + status: run.status, + error: run.error, + endedAt: run.endedAt, + ...priorRouting, + }, }) .catch(() => undefined); return NextResponse.json( diff --git a/cloud/apps/web/src/lib/run-view.ts b/cloud/apps/web/src/lib/run-view.ts index bde38c4..c3042d4 100644 --- a/cloud/apps/web/src/lib/run-view.ts +++ b/cloud/apps/web/src/lib/run-view.ts @@ -5,6 +5,7 @@ import { loadActor } from "@/lib/members"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; import { classifyException, + dispositionForKind, duplicateRiskFor, type ExceptionKind, } from "@ghost/core/classifier/exception"; @@ -146,12 +147,15 @@ export async function buildRunView(orgId: string, viewerId: string, runId: strin recorded?.status === "UNKNOWN" ? "UNKNOWN" : recorded?.status === "FAILED" ? "FAILED" : null, }); const kind = (run.incidentKind as ExceptionKind | null) ?? d.kind; + // Every displayed field comes from the kind actually shown — see the + // exceptions route for why mixing the two sources produced contradictions. + const shown = dispositionForKind(kind); exception = { kind, - owner: d.owner, - headline: d.headline, - guidance: d.guidance, - retryUseful: d.retryUseful, + owner: shown.owner, + headline: shown.headline, + guidance: shown.guidance, + retryUseful: shown.retryUseful, // Shared helper rather than an inline OR: forcing this true whenever the // kind or recorded status is UNKNOWN also fired it for an indeterminate // `verify` or `extract`, which are reads that cost nothing to repeat. The diff --git a/cloud/apps/worker/src/jobs/compensateRun.ts b/cloud/apps/worker/src/jobs/compensateRun.ts index 3280822..e0abf23 100644 --- a/cloud/apps/worker/src/jobs/compensateRun.ts +++ b/cloud/apps/worker/src/jobs/compensateRun.ts @@ -499,9 +499,20 @@ async function reverseOne({ payload: { stepId: entry.stepId, actions: describeActions(entry.compensation) }, }); + // Has any reversal action landed, or been in flight, by the time we fail? + // + // This decides whether a failed reversal is safely re-runnable. Set *before* + // `applyStep` so an action that timed out mid-flight counts: a reversal click + // that timed out may well have reached the server, and pressing Undo again + // would send it twice. Only a failure before the very first action starts — + // an unresolvable expression on action 1 — leaves the reversal provably + // untouched. + let appliedOrInFlight = false; + try { for (const action of entry.compensation.actions) { const step = resolveStep(actionAsStep(action, entry.stepId), scope); + appliedOrInFlight = true; await applyStep(session.page, step, { timeoutMs }); } @@ -577,7 +588,15 @@ async function reverseOne({ status: "INCIDENT", cursor: entry.stepIndex, error: `reversal of step ${entry.stepIndex} failed: ${message}`, - ...freshIncidentRouting({ reason: message }), + // Classifying this from the message alone read a timed-out reversal as + // TRANSIENT — "just retry" — when a mutating reversal action that timed + // out may already have taken effect. Pressing Undo again would then + // re-execute it under the still-valid approval with no duplicate-risk + // acknowledgement, which is the exact double-execution the gate exists + // to prevent, reached from the reversal side. + ...freshIncidentRouting( + appliedOrInFlight ? { kind: "OUTCOME_UNKNOWN" } : { reason: message }, + ), }, }); await appendAuditEvent(orgId, actorId, { diff --git a/cloud/apps/worker/src/jobs/runWorkflow.ts b/cloud/apps/worker/src/jobs/runWorkflow.ts index 995cc62..ab59a7c 100644 --- a/cloud/apps/worker/src/jobs/runWorkflow.ts +++ b/cloud/apps/worker/src/jobs/runWorkflow.ts @@ -979,6 +979,11 @@ async function raiseIncident( cursor: index, incidentKind: disposition.kind, incidentRaisedAt: new Date(), + // A newly raised incident is unassigned. Resolution already clears the + // assignee, so in the normal path this is a no-op; it closes the window + // where an assignment raced a resolution and left an owner attached to a + // run that then stopped again for an unrelated reason. + incidentAssigneeId: null, }, }); await appendAuditEvent(orgId, actorId, { diff --git a/cloud/packages/core/src/classifier/exception.test.ts b/cloud/packages/core/src/classifier/exception.test.ts index 7e8f882..e7eb01b 100644 --- a/cloud/packages/core/src/classifier/exception.test.ts +++ b/cloud/packages/core/src/classifier/exception.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { EXCEPTION_KINDS, classifyException, + dispositionForKind, duplicateRiskFor, kindsForOwner, needsAuthoring, @@ -187,6 +188,69 @@ describe("duplicateRiskFor", () => { }); }); +describe("a failed verification on a mutating step is duplicate risk", () => { + // The action ran — that is *why* there was something to assert — and the + // incident route's retry resets the step to PENDING and re-executes the whole + // step under the original approval. For a click on Pay that is a second + // payment, and nothing was warning about it. Stronger than uncertainty: here + // the effect is known to have landed. + it("flags a mutating step whose verification failed", () => { + expect( + duplicateRiskFor({ + disposition: classifyException({ reason: "verification failed at step 3", step: click }), + step: click, + }), + ).toBe(true); + }); + + it("does not flag a verification failure on a read", () => { + expect( + duplicateRiskFor({ + disposition: classifyException({ reason: "verification failed at step 3", step: verify }), + step: verify, + }), + ).toBe(false); + }); + + it("flags it from the stored kind too", () => { + expect(duplicateRiskFor({ storedKind: "VERIFICATION", step: click })).toBe(true); + }); + + it("still leaves genuinely safe kinds unflagged", () => { + for (const kind of ["TRANSIENT", "TARGET_MISSING", "AUTH", "DATA", "UNKNOWN"] as const) { + expect(duplicateRiskFor({ storedKind: kind, step: click })).toBe(false); + } + }); +}); + +describe("dispositionForKind", () => { + it("returns that kind's own owner, headline and guidance", () => { + const d = dispositionForKind("OUTCOME_UNKNOWN"); + expect(d.kind).toBe("OUTCOME_UNKNOWN"); + expect(d.owner).toBe("operator"); + expect(d.headline).toMatch(/already/i); + expect(d.retryUseful).toBe(false); + }); + + it("never mixes one kind's label with another's guidance", () => { + // The display bug this exists to prevent: a compensation incident stores an + // explicit kind whose reason text carries no classifier prefix, so a live + // re-classification returns UNKNOWN and the row showed "OUTCOME_UNKNOWN" + // beside "Unclassified failure". + for (const kind of EXCEPTION_KINDS) { + const d = dispositionForKind(kind); + expect(d).toEqual( + expect.objectContaining({ kind, ...({} as Record) }), + ); + expect(d.headline).toBe(dispositionForKind(kind).headline); + expect(d.guidance).not.toBe(""); + } + expect(dispositionForKind("UNKNOWN").headline).not.toBe( + dispositionForKind("OUTCOME_UNKNOWN").headline, + ); + }); +}); + describe("kindsForOwner", () => { it("partitions every kind across the three desks exactly once", () => { const all = (["operator", "author", "administrator"] as const).flatMap((o) => kindsForOwner(o)); diff --git a/cloud/packages/core/src/classifier/exception.ts b/cloud/packages/core/src/classifier/exception.ts index 703a67a..0493b57 100644 --- a/cloud/packages/core/src/classifier/exception.ts +++ b/cloud/packages/core/src/classifier/exception.ts @@ -198,7 +198,7 @@ const DISPOSITIONS: Record< owner: "operator", headline: "Outcome did not match expectation", guidance: - "The action ran but the result was not what the workflow asserted. Check the target system: if the outcome is actually acceptable, skip the check; if not, the run did the wrong thing and should be reversed.", + "The action ran but the result was not what the workflow asserted — so the effect has already happened, and retrying re-runs the action rather than just the check. Confirm in the target system: if the outcome is acceptable, skip the check; if not, reverse the run.", retryUseful: false, }, OUTCOME_UNKNOWN: { @@ -324,6 +324,24 @@ function classifyKind(input: ClassifyExceptionInput): ExceptionKind { * * Fails closed: an unknown step is assumed mutating. */ +/** + * Kinds where a completed effect may already exist for the stopped step. + * + * `OUTCOME_UNKNOWN` is the obvious one: the step started and never reported. + * + * `VERIFICATION` is the subtle one, and it is *stronger* than uncertainty. A + * verification failure means the action ran and its assertion did not hold — + * there was something to check precisely because the click or fill went through. + * The worker is careful about this within one attempt (it re-runs the assertion + * only, never the action), but an incident retry resets the step to PENDING and + * re-executes the whole thing under the original approval. For a mutating step + * that is a second Pay, and nothing was warning about it. + */ +const EFFECT_MAY_HAVE_LANDED: ReadonlySet = new Set([ + "OUTCOME_UNKNOWN", + "VERIFICATION", +]); + export function duplicateRiskFor(input: { /** Live classifier verdict, when already computed. */ disposition?: ExceptionDisposition; @@ -334,14 +352,34 @@ export function duplicateRiskFor(input: { /** The stopped-on step, when known. */ step?: WorkflowStep; }): boolean { - const indeterminate = - input.disposition?.kind === "OUTCOME_UNKNOWN" || - input.storedKind === "OUTCOME_UNKNOWN" || + const landed = + (input.disposition && EFFECT_MAY_HAVE_LANDED.has(input.disposition.kind)) || + (typeof input.storedKind === "string" && + EFFECT_MAY_HAVE_LANDED.has(input.storedKind as ExceptionKind)) || input.recordedOutcome === "UNKNOWN"; - if (!indeterminate) return false; + if (!landed) return false; return input.step ? replaySafety(input.step) === "mutating" : true; } +/** + * The full disposition for an explicitly known kind. + * + * Read paths store the engine's verdict in `Run.incidentKind` and show that as + * the label, but they also need the owner, headline, guidance and retry + * recommendation that belong *to that kind*. Re-deriving those from a fresh + * `classifyException` over the same reason text produces a mismatch whenever the + * two disagree — and they disagree immediately for compensation incidents, whose + * kind is asserted directly at the call site while their reason text carries no + * classifier prefix. The queue then showed `OUTCOME_UNKNOWN` next to + * "Unclassified failure" guidance, which is worse than either alone. + * + * `retryMayDuplicate` still needs the step, so it is not decided here — callers + * pass the result to `duplicateRiskFor`. + */ +export function dispositionForKind(kind: ExceptionKind): ExceptionDisposition { + return { kind, ...DISPOSITIONS[kind], retryMayDuplicate: false }; +} + /** Every kind, for exhaustive UI rendering and tests. */ export const EXCEPTION_KINDS = Object.keys( DISPOSITIONS, diff --git a/docs/competitive-landscape.md b/docs/competitive-landscape.md index 4de2e58..76c6c37 100644 --- a/docs/competitive-landscape.md +++ b/docs/competitive-landscape.md @@ -75,7 +75,7 @@ Set against Ghost's pipeline, the overlap is thin and the gap is structural: | Human approval before sensitive actions | Not evidenced | **Yes**, with expiry, single-use | | Per-step outcome verification | No | **Yes** | | Hash-chained audit log | No | **Yes**, two-level | -| Incidents / undo / durable resume | No | **Yes** (undo designed, not built) | +| Incidents / undo / durable resume | No | **Yes** — undo ships as approval-gated compensation, with documented limits (reversals run in a fresh unauthenticated context) | | Multi-tenant org isolation, RBAC | Single-user Mac app | **Yes** | | SOC 2 | **Yes** | **No** | | Self-serve free tier | **Yes** | **No** — implementation-led | From feb1e9cf359356045945247e0b37c36293654ba3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 18:40:47 +0000 Subject: [PATCH 5/5] fix(cloud): address third Codex review on exception routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All nine findings correct. The two structural ones: A classification with no enforcement path. The previous commit started storing OUTCOME_UNKNOWN when a reversal action was in flight as it failed, but nothing read it. The undo route accepted the resulting INCIDENT and rescheduled the same reversal under a compensation approval that is still APPROVED — no acknowledgement, no gate. Storing the verdict was the easy half; this adds the refusal. Undo now 409s on an indeterminate prior reversal unless the caller acknowledges, mirroring the forward retry gate. The step is deliberately not passed to duplicateRiskFor there: the risky effect is the reversal's action, not the forward step at that cursor, and omitting it fails closed. The acknowledgement was not bound to anything. It was a bare boolean, so it could outlive the incident it was shown for: the timeline is fed by SSE, and if another operator resolved the displayed incident while the confirm dialog sat open and the run then parked on a new risky step, clicking through would acknowledge a step its clicker never read. The request now carries the incident identity the human actually saw — step index and incidentRaisedAt — and the route refuses a mismatch. Same principle as approving the resolved action rather than the template: the confirmation has to be about the thing that runs. The dialog also closes itself when it stops describing what is on screen. Audit atomicity, applied where it was missed. The assign path was fixed last round; retry and skip still appended their org audit event after their transaction committed. A failure there left the run out of INCIDENT, routing cleared, acknowledgement recorded only in the run journal — and the stalled-run reclaimer could later drive the retry with no org-level record of who accepted the risk. Both now commit inside their transaction. Two incident transitions were never stamped: reclaimRuns and the journal-tamper quarantine in runWorkflow set INCIDENT directly rather than through raiseIncident. Postgres sorts nulls last in ASC, so once an org accumulated 200 timestamped incidents those runs would be pushed past the row cap and become permanently invisible in the queue — a stalled run and a tampered journal being exactly the two you least want hidden. Both now carry kind, assignee and raisedAt. This is also the root fix for null-kind rows escaping the filtered cap. Membership is now re-checked inside the assignment transaction. The prior lookup is only an early 404: member removal runs its own unassignment in a separate transaction, and the foreign key references User rather than Membership, so a removal landing in between would still let the write re-attach a non-member. The assignee select stayed enabled during its own request, because useTransition's pending only begins after the fetch resolves. Two quick changes raced and the slower could win on the server while the control showed the faster. Tracks the request itself, with a version counter so a superseded response touches no state. Docs: one more present-tense API-execution claim survived the table fix further down the same file. And trust-pipeline.md asserted "classification never changes control flow" two paragraphs above describing the gate it feeds — narrowed to what is true, that it never changes how a run *stops* while deliberately shaping recovery. Validation: typecheck, lint, build clean; 540 tests across 58 files against real Postgres + Redis (up from 536). No schema drift. Postgres died mid-run a third time in this sandbox; restarted and re-ran green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1 --- .../[id]/incident/incident-routing.test.ts | 78 +++++++++++++ .../src/app/api/runs/[id]/incident/route.ts | 103 +++++++++++++++--- .../web/src/app/api/runs/[id]/undo/route.ts | 42 ++++++- .../web/src/components/exception-assignee.tsx | 19 +++- .../apps/web/src/components/run-timeline.tsx | 37 +++++-- cloud/apps/web/src/lib/run-view.ts | 4 + cloud/apps/worker/src/jobs/reclaimRuns.ts | 10 ++ cloud/apps/worker/src/jobs/runWorkflow.ts | 15 ++- docs/competitive-landscape.md | 6 +- docs/trust-pipeline.md | 10 +- 10 files changed, 291 insertions(+), 33 deletions(-) diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts index 2a06a5b..7065900 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/incident-routing.test.ts @@ -385,6 +385,84 @@ describe.skipIf(!hasDb)("incident routing (Postgres)", () => { expect(after?.incidentRaisedAt).toBeNull(); }); + it("refuses an acknowledgement raised for a different incident", async () => { + // The confirmation must be about the incident the human actually read. If + // someone else resolves it and the run parks on a new risky step, a stale + // open dialog must not be able to acknowledge the new one. + const runId = await incidentRun({ + stepStatus: "UNKNOWN", + error: "OUTCOME_UNKNOWN: step 1 may or may not have taken effect", + }); + + const stale = await post(runId, { + action: "retry", + acknowledgeDuplicateRisk: true, + expectStepIndex: 0, // the run is parked on step 1 + }); + expect(stale.status).toBe(409); + const body = (await stale.json()) as { staleAcknowledgement?: boolean }; + expect(body.staleAcknowledgement).toBe(true); + expect(enqueueRunWorkflow).not.toHaveBeenCalled(); + + // The same call naming the incident actually on screen is accepted. + const run = await prisma.run.findUnique({ where: { id: runId } }); + const ok = await post(runId, { + action: "retry", + acknowledgeDuplicateRisk: true, + expectStepIndex: 1, + expectIncidentRaisedAt: run!.incidentRaisedAt!.toISOString(), + }); + expect(ok.status).toBe(200); + }); + + it("refuses an acknowledgement whose incident timestamp has moved on", async () => { + const runId = await incidentRun({ + stepStatus: "UNKNOWN", + error: "OUTCOME_UNKNOWN: step 1 may or may not have taken effect", + }); + + const res = await post(runId, { + action: "retry", + acknowledgeDuplicateRisk: true, + expectStepIndex: 1, + expectIncidentRaisedAt: new Date(Date.now() - 86_400_000).toISOString(), + }); + expect(res.status).toBe(409); + expect(enqueueRunWorkflow).not.toHaveBeenCalled(); + }); + + it("records the retry audit in the same transaction as the state change", async () => { + const runId = await incidentRun({ + stepStatus: "FAILED", + error: "net::ERR_CONNECTION_RESET", + incidentKind: "TRANSIENT", + }); + await post(runId, { action: "retry" }); + + // Both must exist together: a run that left INCIDENT with no org audit + // record would let the reclaimer drive the retry unaccounted for. + const run = await prisma.run.findUnique({ where: { id: runId } }); + expect(run?.status).toBe("QUEUED"); + const audit = await prisma.auditEvent.findFirst({ + where: { orgId, entityId: runId, action: "run.incident_retried" }, + }); + expect(audit).not.toBeNull(); + }); + + it("still audits a skip", async () => { + const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); + // Step 1 is a click on "Pay invoice" — sensitive, so skip is refused. Use + // step 0 (navigate) instead by parking the run there. + await prisma.run.update({ where: { id: runId }, data: { cursor: 0 } }); + + const res = await post(runId, { action: "skip" }); + expect(res.status).toBe(200); + const audit = await prisma.auditEvent.findFirst({ + where: { orgId, entityId: runId, action: "run.incident_skipped" }, + }); + expect(audit).not.toBeNull(); + }); + it("rejects an unknown action", async () => { const runId = await incidentRun({ stepStatus: "FAILED", error: "boom" }); const res = await post(runId, { action: "resolve" }); diff --git a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts index e3575f8..d6d43e9 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/incident/route.ts @@ -51,6 +51,10 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str action?: string; assigneeId?: string | null; acknowledgeDuplicateRisk?: boolean; + // Identity of the incident the caller was looking at when they confirmed. + // See the acknowledgement check below. + expectStepIndex?: number; + expectIncidentRaisedAt?: string; }; if (body.action !== "retry" && body.action !== "skip" && body.action !== "assign") { return NextResponse.json({ error: "action must be retry|skip|assign" }, { status: 400 }); @@ -95,6 +99,13 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // would leave the queue showing an owner with no audit trail explaining how // they got it, while the caller was told the request failed. Either both // land or neither does. `appendAuditEvent` takes the tx for exactly this. + // Both the membership check and the run state are re-verified *inside* the + // transaction below. The lookup above is only an early, friendly 404: member + // removal runs its own unassignment cleanup in a transaction of its own, and + // between that read and this write the membership can vanish — the foreign + // key references `User`, not `Membership`, so the write would still succeed + // and re-attach a non-member to this org's queue. + // // Conditional on the run still being an INCIDENT, inside the transaction. // The `findFirst` above is a read: a retry, skip, cancel or undo can resolve // the incident between it and this write, and an id-only update would then @@ -103,6 +114,13 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // incident. let assigned = 0; await prisma.$transaction(async (tx) => { + if (assigneeId !== null) { + const stillMember = await tx.membership.findFirst({ + where: { orgId, userId: assigneeId }, + select: { userId: true }, + }); + if (!stillMember) return; + } const res = await tx.run.updateMany({ where: { id, orgId, status: "INCIDENT" }, data: { incidentAssigneeId: assigneeId }, @@ -123,7 +141,11 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str }); if (assigned !== 1) { return NextResponse.json( - { error: "this run is no longer an open exception" }, + { + error: + "could not assign: the run is no longer an open exception, or that " + + "person is no longer a member of this organization", + }, { status: 409 }, ); } @@ -161,9 +183,6 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str // up. The journal sequence is already monotonic per run, so it distinguishes // them without inventing a second counter. let resumeSeq = 0; - // Whether this resolution retried a step whose effect may already have - // happened. Hoisted so the audit event below can record the decision. - let acknowledgedDuplicateRisk = false; if (body.action === "skip") { if (classifyStep(step).sensitive) { @@ -215,6 +234,19 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str tx, ); resumeSeq = seq; + // Same atomicity as the retry branch: the state change, the journal + // event and the org audit record commit together or not at all. + await appendAuditEvent( + orgId, + userId, + { + action: "run.incident_skipped", + entityType: "Run", + entityId: id, + metadata: { stepIndex: index, kind: run.incidentKind }, + }, + tx, + ); }); } else { // Retrying a step whose effect may already have happened is a decision the @@ -266,6 +298,35 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str step, }); + // An acknowledgement has to be *about something*. A bare boolean can + // outlive the incident it was shown for: if another operator resolves this + // incident while the confirmation is open and the run then parks on a new + // risky step, the still-open dialog would acknowledge a step its clicker + // never looked at. So the caller states which incident it was shown — step + // index and raised-at — and a mismatch is refused. Same principle as + // approving the *resolved* action rather than the template: the human must + // be confirming the thing that actually runs. + if (mayDuplicate && body.acknowledgeDuplicateRisk === true) { + const sameStep = + body.expectStepIndex === undefined || body.expectStepIndex === index; + const sameIncident = + body.expectIncidentRaisedAt === undefined || + (run.incidentRaisedAt !== null && + new Date(body.expectIncidentRaisedAt).getTime() === run.incidentRaisedAt.getTime()); + if (!sameStep || !sameIncident) { + return NextResponse.json( + { + error: + "this run has stopped on a different step since that confirmation was shown. " + + "Re-read the current exception before retrying.", + requiresAcknowledgement: true, + staleAcknowledgement: true, + }, + { status: 409 }, + ); + } + } + if (mayDuplicate && body.acknowledgeDuplicateRisk !== true) { return NextResponse.json( { @@ -279,8 +340,6 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str { status: 409 }, ); } - acknowledgedDuplicateRisk = mayDuplicate; - // Retry: clear the recorded failure so the journal fold stops reporting it. // The step's own `step.started`/`step.failed` history stays in the chain — // this appends, it never rewrites. @@ -318,20 +377,30 @@ export async function POST(req: Request, { params }: { params: Promise<{ id: str tx, ); resumeSeq = seq; + // Inside the transaction, with the state change and the journal event. + // Appended after them so it is ordered last in the chain, but committed + // atomically: if this were left outside and failed, the run would already + // have left INCIDENT carrying an acknowledgement recorded only in the run + // journal, and the stalled-run reclaimer could later drive the retry with + // no org-level audit record of who accepted the duplicate risk. + await appendAuditEvent( + orgId, + userId, + { + action: "run.incident_retried", + entityType: "Run", + entityId: id, + metadata: { + stepIndex: index, + kind: run.incidentKind, + ...(mayDuplicate ? { acknowledgedDuplicateRisk: true } : {}), + }, + }, + tx, + ); }); } - await appendAuditEvent(orgId, userId, { - action: body.action === "skip" ? "run.incident_skipped" : "run.incident_retried", - entityType: "Run", - entityId: id, - metadata: { - stepIndex: index, - kind: run.incidentKind, - ...(acknowledgedDuplicateRisk ? { acknowledgedDuplicateRisk: true } : {}), - }, - }); - await enqueueRunWorkflow({ runId: id, orgId, diff --git a/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts b/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts index dc13817..d011aea 100644 --- a/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts +++ b/cloud/apps/web/src/app/api/runs/[id]/undo/route.ts @@ -4,6 +4,7 @@ import { prisma } from "@/lib/db"; import { enqueueCompensateRun } from "@/lib/queue"; import { appendAuditEvent } from "@ghost/core/audit-log"; import { parseWorkflowSteps } from "@ghost/core/schema/step"; +import { duplicateRiskFor } from "@ghost/core/classifier/exception"; import { describeActions, planCompensation, remainingEntries } from "@ghost/core/compensate"; import { journalFromEvents, journalFromLegacyRunSteps } from "@ghost/core/journal"; import { RUN_EVENT_TYPES } from "@ghost/core/run-events"; @@ -96,7 +97,7 @@ export async function GET(_req: Request, { params }: { params: Promise<{ id: str }); } -export async function POST(_req: Request, { params }: { params: Promise<{ id: string }> }) { +export async function POST(req: Request, { params }: { params: Promise<{ id: string }> }) { const session = await auth(); if (!session?.user?.orgId) { return NextResponse.json({ error: "unauthorized" }, { status: 401 }); @@ -105,6 +106,13 @@ export async function POST(_req: Request, { params }: { params: Promise<{ id: st const userId = session.user.id ?? null; const { id } = await params; + // Undo took no body until the duplicate-risk gate below needed one. Tolerant + // of an absent or malformed body so an existing caller that sends nothing + // still works — it simply will not be treated as having acknowledged. + const body = (await req.json().catch(() => ({}))) as { + acknowledgeDuplicateRisk?: boolean; + }; + const loaded = await loadPlan(id, orgId); if (!loaded) return NextResponse.json({ error: "not found" }, { status: 404 }); @@ -145,6 +153,38 @@ export async function POST(_req: Request, { params }: { params: Promise<{ id: st // to give the claim back if scheduling fails, and a run returned to INCIDENT // without its classification and owner has silently lost work — the exception // reappears in the queue unassigned and labelled "Unclassified failure". + // A reversal that already failed indeterminately must not be re-scheduled on + // a single click. + // + // `compensateRun` records OUTCOME_UNKNOWN when a reversal action was in + // flight — a Cancel or Refund that timed out may well have reached the target. + // Storing that classification is worthless without an enforcement path, and + // this route was the gap: it accepted the resulting INCIDENT and rescheduled + // the same remaining reversal, under a compensation approval that is still + // APPROVED, with no acknowledgement. That is the forward gate's exact failure + // mode reached from the reversal side, so it gets the same treatment. + // + // The step is not passed to `duplicateRiskFor` deliberately: the risky effect + // here is the *reversal's* action, not the forward step at this cursor, and + // omitting it fails closed. + if ( + run.status === "INCIDENT" && + duplicateRiskFor({ storedKind: run.incidentKind }) && + body.acknowledgeDuplicateRisk !== true + ) { + return NextResponse.json( + { + error: + "the previous reversal of this run may already have taken effect before it failed, " + + "so reversing again could repeat it. Confirm in the target system first, then retry " + + "with acknowledgeDuplicateRisk: true.", + kind: run.incidentKind, + requiresAcknowledgement: true, + }, + { status: 409 }, + ); + } + const priorRouting = { incidentKind: run.incidentKind, incidentAssigneeId: run.incidentAssigneeId, diff --git a/cloud/apps/web/src/components/exception-assignee.tsx b/cloud/apps/web/src/components/exception-assignee.tsx index 5bfb124..67d63c3 100644 --- a/cloud/apps/web/src/components/exception-assignee.tsx +++ b/cloud/apps/web/src/components/exception-assignee.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState, useTransition } from "react"; +import { useRef, useState, useTransition } from "react"; import { useRouter } from "next/navigation"; interface Member { @@ -33,10 +33,19 @@ export function ExceptionAssignee({ const [pending, startTransition] = useTransition(); const [error, setError] = useState(null); const [value, setValue] = useState(assignee?.id ?? ""); + // `pending` from useTransition only becomes true *after* the fetch resolves, + // so it leaves the select enabled for the whole request. Two quick changes + // then race, and the slower one can win on the server while the control shows + // the faster one. This tracks the request itself, and a version counter makes + // a superseded response a no-op rather than something that rewrites state. + const [sending, setSending] = useState(false); + const latest = useRef(0); async function assign(next: string) { + const ticket = ++latest.current; setError(null); setValue(next); + setSending(true); const res = await fetch(`/api/runs/${runId}/incident`, { method: "POST", headers: { "content-type": "application/json" }, @@ -45,8 +54,14 @@ export function ExceptionAssignee({ assigneeId: next === "" ? null : next, }), }); + // A response that has been overtaken must not touch state at all — not the + // value, not the error. + if (ticket !== latest.current) return; + setSending(false); + if (!res.ok) { const body = (await res.json().catch(() => ({}))) as { error?: string }; + if (ticket !== latest.current) return; setError(body.error ?? "could not assign"); // Put the control back to the truth, so it never shows an assignment the // server rejected. @@ -67,7 +82,7 @@ export function ExceptionAssignee({