feat(cloud): exception routing, plus a competitive-landscape doc - #426
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1
mohabbis
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3d0dff0c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const recorded = await prisma.runStep | ||
| .findUnique({ where: { runId_index: { runId: id, index } }, select: { status: true } }) | ||
| .catch(() => null); |
There was a problem hiding this comment.
If this safety-critical query fails transiently, .catch(() => null) makes the route treat the outcome as absent. For an UNKNOWN mutating step whose error text resembles an ordinary timeout, the retry can then proceed without acknowledgeDuplicateRisk, potentially repeating an external effect; propagate the database error instead of converting it into the least restrictive classification.
AGENTS.md reference: AGENTS.md:L121-L130
Useful? React with 👍 / 👎.
| await prisma.run.update({ where: { id }, data: { incidentAssigneeId: assigneeId } }); | ||
| await appendAuditEvent(orgId, userId, { |
There was a problem hiding this comment.
When appendAuditEvent fails after the run update succeeds, the exception remains assigned even though the request reports failure and no durable audit record exists. Put the assignment update and audit append in one Prisma transaction so this routing mutation is either both applied and recorded or neither.
AGENTS.md reference: AGENTS.md:L101-L115
Useful? React with 👍 / 👎.
| -- 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; |
There was a problem hiding this comment.
This foreign key references User, so ON DELETE SET NULL runs only when the user account is deleted, not when cloud/apps/web/src/app/api/settings/members/[userId]/route.ts:126 deletes that user's organization membership. Removing a member therefore leaves open incidents assigned to a non-member and absent from every current member's personal queue; explicitly unassign that organization's incidents during membership removal or model the relation through membership.
Useful? React with 👍 / 👎.
| ...(mine && session.user.id | ||
| ? { incidentAssigneeId: session.user.id } | ||
| : {}), | ||
| ...(kindFilter ? { incidentKind: kindFilter } : {}), |
There was a problem hiding this comment.
When kind is supplied, filtering on incidentKind in SQL discards all pre-migration incidents and incidents raised by paths such as reclaim or compensation, because those rows have incidentKind = NULL. The fallback classification below never sees them, so requests such as ?kind=UNKNOWN omit matching open exceptions; include null kinds and apply the requested filter after fallback classification, or backfill/store classifications for every incident source.
Useful? React with 👍 / 👎.
| incidentKind String? | ||
| incidentAssigneeId String? |
There was a problem hiding this comment.
These new fields are cleared only by retry/skip. If an INCIDENT run is sent through the existing undo route, it becomes COMPENSATING without clearing them, and a subsequent compensation failure returns it to INCIDENT without overwriting them; the failed reversal then inherits the original incident's kind and assignee and is misrouted in the queue. Clear the fields whenever a run leaves INCIDENT, and set fresh routing data when compensation raises a new incident.
Useful? React with 👍 / 👎.
| ...(kindFilter ? { incidentKind: kindFilter } : {}), | ||
| }, | ||
| // Oldest first — see the module comment. | ||
| orderBy: { createdAt: "asc" }, |
There was a problem hiding this comment.
Ordering by Run.createdAt does not order exceptions by how long they have been parked. A week-old run that fails now is placed ahead of a run that entered INCIDENT yesterday, and the createdAt fallback used for stoppedAt can also report that the new incident has been waiting for a week. Store an incident timestamp whenever the run enters INCIDENT, then order by and display that value.
Useful? React with 👍 / 👎.
| if (ownerFilter && shaped.owner !== ownerFilter) continue; | ||
| rows.push(shaped); |
There was a problem hiding this comment.
The owner filter runs only after the query has already selected the oldest 200 incidents. If those 200 are operator-owned while an author-owned incident appears later, ?owner=author returns an empty result even though matching incidents exist, with misleading totals and truncation metadata. Store the owner for SQL filtering or continue paging and classifying until the filtered page is filled.
Useful? React with 👍 / 👎.
| [ | ||
| /waiting for (?:locator|selector)|strict mode violation|no element matches/i, | ||
| "TARGET_MISSING", | ||
| ], |
There was a problem hiding this comment.
The worker resolves preferred selectors through getByRole, getByTestId, and getByText in cloud/apps/worker/src/browser/selector.ts, so their timeout call logs say they are waiting for those locator forms rather than literally locator or selector. These target-change failures fall through to the generic timeout rule and are routed as transient, leading operators to retry a step that needs re-authoring; include the semantic locator names in this rule.
Useful? React with 👍 / 👎.
| | 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 | |
There was a problem hiding this comment.
The comparison says Ghost executes typed steps through browser and API, but cloud/apps/worker/src/browser/driver.ts:87 explicitly lists apiCall as unimplemented and its executor currently performs no action. This presents a planned connector capability as shipped product behavior; describe the current browser-only execution surface or clearly mark API execution as future work.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
| retryMayDuplicate: | ||
| d.retryMayDuplicate || kind === "OUTCOME_UNKNOWN" || recorded?.status === "UNKNOWN", |
There was a problem hiding this comment.
For an UNKNOWN read-only step such as verify or extract, classifyException deliberately returns retryMayDuplicate: false, but these extra conditions force it back to true solely because the kind or recorded status is UNKNOWN. The run timeline then requires a duplicate-effect confirmation and the queue warns that an external effect may have happened even though repeating the step is only a read; retain the classifier's step-aware result, or combine any stale stored kind with replaySafety rather than treating OUTCOME_UNKNOWN as synonymous with mutation.
Useful? React with 👍 / 👎.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a10efd9f99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| status: "INCIDENT", | ||
| cursor: entry.stepIndex, | ||
| error: `reversal of step ${entry.stepIndex} failed: ${message}`, | ||
| ...freshIncidentRouting({ reason: message }), |
There was a problem hiding this comment.
When a compensation action such as click times out after possibly taking effect, passing only the raw message makes classifyException label the incident TRANSIENT; the next Undo request treats the recorded step.compensation_failed as retryable and can execute the reversal again using the still-approved gate, without duplicate-risk acknowledgement. Preserve the compensation action context and classify a mutating action with an uncertain outcome as OUTCOME_UNKNOWN rather than inferring safety from timeout text.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| incidentKind: null, | ||
| incidentAssigneeId: null, | ||
| incidentRaisedAt: null, |
There was a problem hiding this comment.
When Undo starts from an INCIDENT run and either the audit append or queue enqueue throws, this update has already cleared the kind, assignee, and raised timestamp, but the catch restores only status, error, and endedAt. The run returns to INCIDENT with its assignment and recorded classification lost, contrary to the catch's promise to restore the previous state; save and restore these three fields as well.
Useful? React with 👍 / 👎.
| // 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 } }); |
There was a problem hiding this comment.
If a retry, skip, cancel, or undo resolves the incident after the initial findFirst but before this transaction runs, the ID-only update still assigns the now-non-incident run and records a successful incident assignment. Because a later raiseIncident does not clear an existing assignee, that stale owner can also leak into a future incident; make the transactional update conditional on orgId and status: "INCIDENT" and reject when no row is updated.
Useful? React with 👍 / 👎.
| kind, | ||
| owner: disposition.owner, | ||
| headline: disposition.headline, | ||
| guidance: disposition.guidance, | ||
| retryUseful: disposition.retryUseful, |
There was a problem hiding this comment.
When the stored kind differs from live reclassification, this response combines the stored label with another kind's owner, headline, guidance, and retry recommendation. This occurs immediately for explicitly classified compensation incidents such as OUTCOME_UNKNOWN: their reason lacks the classifier prefix and the original run step is not UNKNOWN, so live classification returns UNKNOWN and the queue displays “Unclassified failure” guidance beside an OUTCOME_UNKNOWN kind; derive all disposition fields from the selected stored kind, as should the parallel page and run-view shaping.
Useful? React with 👍 / 👎.
| runId: { in: runs.map((r) => r.id) }, | ||
| index: { in: [...new Set(runs.map((r) => r.cursor))] }, |
There was a problem hiding this comment.
When the returned runs are stopped at different cursor indexes, these two independent IN predicates fetch every matching run/index combination rather than one stopped step per run. A full page with 200 distinct cursors can therefore load up to 40,000 RunStep rows to shape 200 exceptions, and the server-rendered queue repeats the same pattern; query an OR of {runId, index} pairs or otherwise constrain each cursor to its own run.
Useful? React with 👍 / 👎.
| | 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) | |
There was a problem hiding this comment.
This cell simultaneously marks incidents/undo as supported and says undo is only designed, but the repository already implements preview, approval-gated compensation execution, audit events, and retryable partial reversals in the undo route and compensation worker. Correct the comparison to describe undo as built with its documented limitations so the product-positioning document reflects the current cloud behavior.
AGENTS.md reference: AGENTS.md:L235-L236
Useful? React with 👍 / 👎.
| // 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; |
There was a problem hiding this comment.
For a mutating step whose action succeeds but whose post-action verification fails, the engine records VERIFICATION; this condition then reports no duplicate risk, so the incident route resets the step to pending and re-executes the action using its existing approval without acknowledgement. A failed assertion after clicking Pay or Submit is evidence that the action already ran, so retrying the whole step can duplicate the effect; either resume with verification only or require the same duplicate-risk confirmation before re-running a mutating step.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1
CodeQL (rust) is red for an upstream reason, not a findingThe analysis succeeded. It is the upload of the results that failed, because GitHub's code-scanning API is returning From the job log on Queries ran to completion and the SARIF was produced and fingerprinted. Across the 401 log records there is exactly one error, and it is that upstream message. No alerts were raised. This is the second occurrence. On the previous commit Worth noting for anyone triaging: this PR changes no Rust. The diff is TypeScript under I could not re-run immediately — For the record, one pre-existing observation unrelated to this PR: the Rust extractor reports Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a96a28fd23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...freshIncidentRouting( | ||
| appliedOrInFlight ? { kind: "OUTCOME_UNKNOWN" } : { reason: message }, | ||
| ), |
There was a problem hiding this comment.
Fresh evidence in this revision is that the compensation failure is now stored as OUTCOME_UNKNOWN, but that classification still has no enforcement path: after a Cancel/Refund action times out, POST /api/runs/[id]/undo accepts the resulting INCIDENT and reschedules the same remaining reversal without inspecting incidentKind or requiring acknowledgement, while its prior compensation approval remains APPROVED. The reversal can therefore execute twice; require and audit duplicate-risk acknowledgement before rescheduling an indeterminate compensation.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| metadata: { | ||
| stepIndex: index, | ||
| kind: run.incidentKind, | ||
| ...(acknowledgedDuplicateRisk ? { acknowledgedDuplicateRisk: true } : {}), |
There was a problem hiding this comment.
If this appendAuditEvent fails after the preceding transaction commits, the run has already left INCIDENT, cleared its routing fields, and recorded the duplicate-risk acknowledgement only in the run journal; the request fails before enqueueing, but the stalled-run reclaimer can later execute the retry without the promised organization audit record. Include the audit append in the same transaction as the retry state and journal event so a risky retry cannot persist without its durable acknowledgement.
AGENTS.md reference: AGENTS.md:L101-L115
Useful? React with 👍 / 👎.
| }, | ||
| // 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" }], |
There was a problem hiding this comment.
Fresh evidence after adding incidentRaisedAt is that it remains null for pre-migration incidents and for current paths such as reclaimRuns.ts:88 and the journal-tamper transition in runWorkflow.ts:219. PostgreSQL ascending order places nulls last, so once an organization has 200 timestamped incidents, these fallback incidents are omitted indefinitely by take: 200, despite the response later claiming to fall back to createdAt. Set the timestamp on every incident transition or order using a database-side fallback before applying the cap.
Useful? React with 👍 / 👎.
| ? { incidentAssigneeId: session.user.id } | ||
| : {}), | ||
| ...(wantedKinds | ||
| ? { OR: [{ incidentKind: { in: wantedKinds } }, { incidentKind: null }] } |
There was a problem hiding this comment.
The new OR admits null-kind incidents but still does not make filtering happen before take: all null rows enter the SQL result regardless of their eventual kind, and only after the first 200 are fetched does the loop discard owner/kind mismatches. For example, 200 legacy or reclaim incidents that classify to operator can still make ?owner=author return empty while a null-kind author incident exists later. Backfill/store classifications for every incident source, or continue paging null rows until the requested filtered page is filled.
Useful? React with 👍 / 👎.
| **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. |
There was a problem hiding this comment.
Although the earlier comparison rows were corrected to call API execution future work, fresh evidence remains here: this present-tense statement still says Ghost executes against APIs on servers. cloud/apps/worker/src/browser/driver.ts lists both apiCall and sendEmail as unimplemented no-ops, so this product-positioning section continues to promise an unsupported execution surface; describe the current browser-only implementation instead.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
| void post(`/api/runs/${run.id}/incident`, { | ||
| action: "retry", | ||
| acknowledgeDuplicateRisk: true, | ||
| }); |
There was a problem hiding this comment.
The acknowledgement is only an unversioned boolean, so it can outlive the incident the user reviewed. If another operator resolves the displayed incident while this confirmation is open and the run subsequently parks on a new risky step, SSE updates run but leaves confirmRetry true; clicking this button then acknowledges and retries the new incident without the user checking its target effect. Send the displayed cursor plus an incident identity such as incidentRaisedAt, and reject the request unless they still match transactionally.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| const member = await prisma.membership.findFirst({ | ||
| where: { orgId, userId: body.assigneeId }, | ||
| select: { userId: true }, | ||
| }); |
There was a problem hiding this comment.
The membership check occurs before the assignment transaction. If member removal deletes the membership and runs its new unassignment cleanup after this lookup but before the assignment transaction, the later run update still succeeds because the foreign key references User, not Membership, leaving the incident assigned to a non-member again. Lock or revalidate the membership while serializing against removal, or model the assignee relation through organization membership.
Useful? React with 👍 / 👎.
| <select | ||
| id={`assignee-${runId}`} | ||
| value={value} | ||
| disabled={pending} | ||
| onChange={(e) => void assign(e.target.value)} |
There was a problem hiding this comment.
pending is supplied by useTransition, but the transition starts only after fetch succeeds, so the select remains enabled throughout the network request. Rapidly choosing two owners sends concurrent assignments whose completion order can differ from the user's choice order; the older request may become the final server value while local value still displays the newer choice. Track the fetch itself as pending or abort/version superseded requests before accepting their results.
Useful? React with 👍 / 👎.
| - **Classification never changes control flow.** It decides what a human is | ||
| *shown* and where the work lands. The state transition is identical either way. |
There was a problem hiding this comment.
This stated invariant contradicts the implementation and the immediately following paragraph: classifyException supplies the disposition used by duplicateRiskFor, and that result determines whether the incident route returns 409 or proceeds with a retry. Classification therefore does affect recovery control flow even if it does not alter the worker's original incident transition; narrow this claim so the trust documentation accurately describes the acknowledgement gate.
AGENTS.md reference: AGENTS.md:L225-L227
Useful? React with 👍 / 👎.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pficbd9mpRYCyzpzsrrpP1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feb1e9cf35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // incidents. UNKNOWN rather than a guess: a run that stalled | ||
| // repeatedly may have had a step in flight, so its effect is not | ||
| // something this path can assert. | ||
| incidentKind: "UNKNOWN", |
There was a problem hiding this comment.
When the fifth worker attempt dies after appending step.started, the stalled run can still have a mutating click in flight, but this path stores UNKNOWN; duplicateRiskFor only gates OUTCOME_UNKNOWN/VERIFICATION, and the RunStep remains RUNNING, so the incident route permits a one-click retry that may repeat the external effect. Inspect the journal/step state here and classify any in-flight effect as OUTCOME_UNKNOWN rather than the non-gating generic kind.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| 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()); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new identity check explicitly accepts omitted fields—its own test at incident-routing.test.ts:178-188 still sends only the boolean—and it compares against the initial run snapshot before an ID-only update. A caller can therefore send a bare acknowledgement, or incident A can be replaced by incident B between this check and the transaction, and the stale decision will retry B; require both identity fields and make the state transition conditional on the same cursor and incidentRaisedAt.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| const body = (await req.json().catch(() => ({}))) as { | ||
| acknowledgeDuplicateRisk?: boolean; | ||
| }; |
There was a problem hiding this comment.
For an OUTCOME_UNKNOWN compensation incident, this request carries only an unversioned boolean. If another operator retries incident A and the reversal parks again before this request reaches the conditional update, the run is once more INCIDENT and the acknowledgement for A schedules the actions for incident B without the caller reviewing their outcome; include the cursor and incidentRaisedAt and verify them atomically with the COMPENSATING transition.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| if ( | ||
| run.status === "INCIDENT" && | ||
| duplicateRiskFor({ storedKind: run.incidentKind }) && | ||
| body.acknowledgeDuplicateRisk !== true | ||
| ) { |
There was a problem hiding this comment.
When a compensation action times out and the worker stores OUTCOME_UNKNOWN, this branch correctly returns 409, but the inspected run timeline's Undo button at run-timeline.tsx:463-468 always posts {} and has no handler that can resend acknowledgeDuplicateRisk: true. A browser user is therefore permanently unable to resume a partially completed reversal; add the same explicit confirmation flow used for forward retries.
AGENTS.md reference: AGENTS.md:L101-L115
Useful? React with 👍 / 👎.
| if ( | ||
| run.status === "INCIDENT" && | ||
| duplicateRiskFor({ storedKind: run.incidentKind }) && | ||
| body.acknowledgeDuplicateRisk !== true | ||
| ) { |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new gate consumes acknowledgeDuplicateRisk, but an accepted request records only the unchanged run.compensation_requested event at lines 220-229, whose metadata does not include the acknowledgement. Consequently the audit trail cannot distinguish an ordinary undo from a human knowingly repeating an indeterminate Cancel/Refund action; persist the acknowledgement and incident identity in the durable audit event.
AGENTS.md reference: AGENTS.md:L101-L115
Useful? React with 👍 / 👎.
| setError(null); | ||
| setValue(next); | ||
| setSending(true); | ||
| const res = await fetch(`/api/runs/${runId}/incident`, { |
There was a problem hiding this comment.
If fetch rejects rather than returning an HTTP response—for example when the connection drops during triage—the exception escapes this async handler before setSending(false), leaving the select permanently disabled and producing an unhandled promise rejection until the user reloads the page. Catch transport errors and reset sending in a finally block while restoring the displayed server value.
Useful? React with 👍 / 👎.
| metadata: { error: message }, | ||
| }); | ||
| await raiseIncident(runId, orgId, actorId, index, message); | ||
| await raiseIncident(runId, orgId, actorId, index, message, step); |
There was a problem hiding this comment.
When a mutating action succeeds but a later operation inside the same try fails—such as verifyStep, page.screenshot(), or the artifact upload at runWorkflow.ts:602-608—this path classifies only the later error text and can record the step as an ordinary FAILED/AUTH/UNKNOWN incident. The retry gate then sees no landed-effect kind and re-executes the click, potentially duplicating a payment; once runStep has begun a mutating action, failures from subsequent verification or evidence capture must be recorded as OUTCOME_UNKNOWN.
AGENTS.md reference: AGENTS.md:L126-L130
Useful? React with 👍 / 👎.
| headline: "Outcome did not match expectation", | ||
| guidance: | ||
| "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, |
There was a problem hiding this comment.
For a mutating step whose action lands but verification fails, this guidance tells the operator to reverse the run, yet the worker records only step.failed; journalFromEvents therefore leaves the index out of completed, and planCompensation skips it at runtime/compensate.ts:84. Undo consequently omits exactly the effect the guidance says it will reverse, so either record the applied action in a form compensation can consume or direct the operator to reverse it manually.
AGENTS.md reference: AGENTS.md:L101-L115
Useful? React with 👍 / 👎.
2 participants
Summary
Five commits: one docs, one feature, three rounds of fixes for automated review. This is not a docs-only change — it adds a schema migration, new endpoints, and changes how a risky retry is authorized. Please review it as code.
82af256docs/competitive-landscape.md— commercial-competitor analysis, first entry Littlebirdb3d0dffa10efd9a96a28ffeb1e9cThe feature
An incident was a state, not a product: a stopped run was reachable only by knowing its id, so the failure mode of a 40-invoice batch was twelve parked runs nobody knew about. This makes the work that needs a human findable, owned, and classified.
classifyExceptionis a third deterministic classifier besideclassifyStep("must a human approve this?") andreplaySafety("safe to re-apply silently?"). It answers a different question: this stopped — whose desk, and is retrying safe? Nine kinds each map to an owner (operator/author/administrator), and that mapping is the routing decision: a changed selector belongs to whoever maintains the workflow, an expired credential to an administrator.Plus the queue (
GET /api/exceptions,/exceptions), assignment, andRun.incidentKind/incidentAssigneeId/incidentRaisedAt.The part with teeth
Some incidents mean an effect may already have landed, and retrying re-runs the whole step under the original approval:
OUTCOME_UNKNOWN— the step started and never reported back.VERIFICATION— stronger. The action ran; only its assertion failed. There was something to assert precisely because the click went through.For a mutating step, either means a retry can be a second payment. The route refuses an unacknowledged retry with 409 and requires
acknowledgeDuplicateRisk, recorded in the run journal and the audit log. The engine still lets a human retry — only a person can check the target system — it just cannot happen by accident, and it is attributable.Two properties keep that gate meaningful rather than decorative:
replaySafety, so an indeterminate read (verify/extract) is not flagged. A prompt that fires on reads is one operators learn to click through.incidentRaisedAtthe human actually saw; a mismatch is refused. Same principle as approving the resolved action rather than the template.Trust & safety
Docs commits touch nothing. The feature:
GET /api/exceptionsand anassignaction, both session-authenticatedTrust-relevant properties, deliberate:
INCIDENTis identical whatever the verdict. It does deliberately shape recovery: the disposition feedsduplicateRiskFor, which is what makes the route refuse an unacknowledged retry. (An earlier version of this body claimed classification never affects control flow at all. That was wrong and is corrected here and intrust-pipeline.md.)UNKNOWN; unknown step assumes duplicate risk; the outcome lookup that decides the gate returns 503 rather than defaulting to permissive; a stale stored kind can raise but never lower the riskChanges
New:
classifier/exception.ts(+ tests) ·api/exceptions/route.ts·(app)/exceptions/page.tsx·components/exception-assignee.tsx· two migrations (run_exception_routing,incident_raised_at) ·incident-routing.test.tsModified:
runWorkflow.ts(classify at raise; all incident transitions stamped) ·compensateRun.ts(failed reversals re-routed from scratch; indeterminate reversals marked) ·incident/route.ts(assign, acknowledgement gate, atomicity) ·undo/route.ts(routing lifecycle + acknowledgement gate) ·settings/members/[userId]/route.ts(unassign on removal) ·reclaimRuns.ts·run-view.ts·run-timeline.tsx·middleware.ts(auth matcher) ·schema.prisma·trust-pipeline.md·CURSOR_HANDOFF.mdValidation
From
cloud/:pnpm typecheck,pnpm lint,pnpm buildclean.pnpm test— 540 tests across 58 files, against a real Postgres 16 + Redis started locally so the ~90DATABASE_URL-gated tests actually ran rather than skipping silently (REDIS_URLandGHOST_SESSION_KEYset too, perCONTRIBUTING.md's two env traps). Migrations apply from scratch;prisma migrate diffreports no drift.Rust checks not applicable and not run —
src-tauri/untouched, andrust.ymlcorrectly skips every Rust job by path filter.CodeQL (rust) has failed twice on this PR for an upstream reason, not a finding — GitHub's code-scanning API returning
No server is currently available, once atInitializeand once atUploading resultsafter the analysis fully completed. Detail in this comment.A repo test caught a real bug mid-build:
middleware.test.tsfailed because/exceptionswas added as a page with no auth matcher entry, so it would have rendered unauthenticated.Risks / follow-up
This PR took three rounds of review fixes — 28 findings, 27 of them correct. They were not repeats; each round went a layer deeper (correctness → concurrency → enforcement-vs-declaration). The recurring theme is worth stating for a reviewer: several were cases where a safety property was represented but not enforced — a classification with no gate reading it, an acknowledgement with no identity bound to it, an audit outside its transaction. Weight the review accordingly.
Known gaps, all deliberate:
PRIOR_ART.mditems (multiple approvers, Slack/email approval, scheduled runs). Recommend a separate PR — this one is already 5 commits and 24 files.DATAexceptions are classified but not fixable. Retry re-sends the same rejected value. Fix-and-resume must re-run classification on the corrected value, per the rule that a resolved value re-triggers the gate.UNKNOWN— the safe direction, but silent. Rule ordering is load-bearing and pinned by a test.incidentKind; deliberately not backfilled, since the reason a backfill would guess from may already have been superseded. Every current incident path now stamps kind and timestamp.littlebird.ai,techcrunch.comandefficient.appare blocked by this environment's egress proxy. Nothing is quoted from the vendor's own site and the doc carries a caveat to re-verify before any of it reaches a pitch. Most important unknown: whether Littlebird's connectors write or only read.^3.4.2to 3.9.6, and the repo is not Prettier-clean at any version since CI never checks it). Cosmetic, butpnpm formatcurrently rewrites ~200 unrelated files — worth pinning exactly or addingformat:check.