Repository navigation
Attach PRs to error issues and verify the fix after merge - #606
Merged
Merged
Conversation
An error issue could carry a fix proposal but never a fix. `propose_fix` accepted a `pr_url` as free text on an event payload — nothing resolved it, nothing watched it, and nothing confirmed the fix worked, so issues sat in `in_review` until someone remembered them. A pull request is now a durable link on an issue. When it merges, the issue enters a new `verifying` state with a quiet window sized from its severity and its own pre-merge occurrence rate. When the window closes, a verification agent runs and either closes the issue (low/medium severity) or posts a verdict for a human (high/critical). Two pieces of existing machinery made this cheap: - The GitHub App integration already had a signature-verified webhook receiver and a sync queue. `pull_request` was the one event class it ignored. - `isRegression` already decides whether an occurrence is a real regression by asking whether its build was in the set seen at resolution time. That is exactly the "did the fix work" predicate — a merge just needs the same snapshot taken at merge instead of at close. So the verdict rests on a deterministic signal the agent corroborates rather than one it invents. Notable behaviour: - If the error fires from a post-merge build while the window is still open, the fix demonstrably failed and the error tick settles it immediately, with no agent pass. Occurrences from builds that predate the merge are discounted as old clients still in the wild. - Verification reads an absent build signal as no evidence, unlike `isRegression` which errs toward reopening. Erring the other way would refute every fix arriving from a service that does not report `service.version`. - An investigation reaching `diagnosed` means the agent found a live cause, so for a verification that maps to "not fixed"; `inconclusive` corroborates the fix. `apply-diagnosis` is now subject-aware so a verification's `severityAssessment` cannot silently re-rank an issue a human already triaged. - Three paths create the same link (propose_fix, the UI, the webhook's body scan), so linking is idempotent. The body scan only matches a dashboard issue URL or a `maple-issue:<uuid>` token, and confirms the issue belongs to the webhook's org first — a PR body is attacker-influenced text. `Fixes #123` is deliberately unsupported: Maple issue ids are UUIDs, so a bare integer can only name the host provider's own tracker. - VCS stays ignorant of issues. The webhook reaches the errors side through a `PullRequestEventSink` port, bound in the composition roots, which also lets the sync-service tests assert forwarding without the error-issue stack. Requires a manual change outside this repo: the GitHub App must add the `Pull request` webhook subscription, or no delivery ever arrives.
`parsePullRequestUrl` and `extractIssueIdsFromText` wrapped `new URL(...)` in `try/catch` to turn a thrown `TypeError` back into a value. Effect has the primitive for this: `Schema.URLFromString`, decoded through `Schema.decodeUnknownOption`, reports the failure as an `Option` instead. Effect's own `SchemaTransformation.urlFromString` is what backs it, so this is the same check the schema layer already performs everywhere else rather than a second, hand-rolled one. Both functions stay synchronous and total, which is what lets the webhook path, the HTTP handler, and the tests keep calling them as plain predicates.
Review of the fix-verification feature turned up one wrong failure type and
a cluster of ways the tick could close an issue it should not have.
- `linkPullRequest` reported a failed upsert/hydrate as
`ErrorIssuePullRequestInvalidError`. The URL had already parsed, so this
told the caller their valid URL was unrecognizable and — because that tag
is an anticipated error — suppressed the span exception, hiding a
persistence anomaly from error dashboards. Both paths now fail with
`ErrorPersistenceError`, which was already in the endpoint's union.
- `Effect.catchCause` catches interrupts too, so an isolate teardown during
the enqueue became `{ enqueued: false }`, which the fallback reads as "no
agent available" and answers with a terminal `verified` verdict. All three
catches now re-raise interrupt-only causes, matching every sibling tick.
- A row whose verdict write failed deterministically aborted the tick before
phase 2 every minute, freezing every other issue in `verifying` with
all-zero counters. Verdict writes are guarded per row and report a new
`failedRows` count.
- The two branches that apply a real verdict counted themselves as `skipped`,
so `verdicts_applied` under-reported exactly the auto-closing paths.
- A per-issue persistence failure aborted the rest of a multi-issue webhook,
which both docstrings promise it does not.
- `String(cause)` in log annotations dumped whole nested causes; use
`summarizeCause` as the sibling enqueue already does.
- One live verification per issue rested on a read-then-insert while the
queue consumer runs with `maxConcurrency: 2`. The partial index is now
UNIQUE (folded into this branch's own migration) and the insert tolerates
the conflict.
- Annotate the four new HTTP handlers and the enqueue's three unannotated
exits, including `no_binding` — the outcome that can auto-close.
UI: the waiting card read "Waiting until in 6h"; link failures all reported
as a bad URL; the attach dialog cleared the pasted URL before the mutation
resolved; the rail cards had no inset; a fix-verification investigation
reached chat keyed by the investigation id rather than the issue's.
Tests: pin the deliberately inverted verdict mapping and the enqueue's four
outcomes, assert PR-event forwarding through the sink (the harness existed
but nothing read it), assert link failures by tag and add cross-tenant
cases. Two existing tests asserted nothing: the machine-owned-state guard
filtered by the same set the implementation uses, and a fixture UUID had a
version nibble `Schema.isUUID()` rejects, so the multi-issue extraction it
claimed was unreachable.
…en iOS spec Both were already red on the parent commit — this branch added a mutating MCP tool and a workflow state without updating the two places that mirror them. `link_pull_request` reached `mutating.ts` and slack-agent's `MUTATING_TOOL_NAMES`, but not the hardcoded list in approval.test.ts. That list exists because the canary next to it skips when the API source is not checked out, so it is the only gate a slack-agent-only checkout has — a mutating tool missing from it ships to Slack without the approval prompt. The iOS OpenAPI spec predates the `verifying` workflow state; regenerated.
Regenerating the OpenAPI spec added `verifying` to the workflow-state enum, which made the three `WorkflowState` switches in Primitives.swift non-exhaustive and failed the build. The spec was stale precisely because this half was never done. Teal, matching `workflow-badge.tsx` and for the reason its comment gives: a merged fix being watched is distinct from `inReview`, where a human is looking, and from `done`, where it is over. Token values are Tailwind's teal-600/400 text and teal-500 fill, the same pair the web badge resolves to. The state-filter picker builds from `allCases`, so it picks this up on its own — correct here, since `verifying` is filterable even though it is machine-owned and never offered as a manual transition.
… Schema `subjectTypeOf` hand-rolled its narrowing — a `typeof` check, an `as` cast to read `.type`, and a bare `string` out — for a value that has a schema. Now `Schema.decodeUnknownOption` over an `InvestigationSubjectDiscriminator`, returning `Option<InvestigationSubjectType>`. The literal set is derived from `InvestigationSubject.members` rather than restated, so a fourth subject type cannot leave it behind, and the guard is `Option.contains` against a narrowed union instead of a magic string compared to `string`. It decodes the discriminator ALONE, deliberately, and the tests pin that. Reading the whole `InvestigationSubject` would answer `None` for a row whose other fields drifted or predate the current shape — and `None` is the dangerous answer here, not the safe one: it means "not a verification", which is what lets a routine post-merge run re-rank a severity a human already set. Excess-key tolerance is what makes a real stored subject decode at all, so that is asserted rather than assumed. Also covers `deterministicInvestigationEventId`, whose output is decoded through a branded id whose UUID check would reject a bad version nibble.
`applyDiagnosisWrites` and `applyInconclusiveWrites` took a raw client and returned a Promise, so every caller had to remember to wrap them and supply the retry, the span and the error mapping. Both now return an Effect and reach `Database` themselves. The drizzle callback inside stays a Promise, and that is the honest end of this: `DatabaseApi.execute` is typed `(db) => Promise<T>` precisely because drizzle's `transaction` is a Promise API with no Effect-native equivalent here — the same shape a dozen sibling services use. Moving the wrapper inside puts the seam at one place per logical call instead of at every call site; it does not pretend the Promise is gone. They fail with the raw `DatabaseError` rather than a domain persistence error. The write straddles two domains — `investigations`, plus the error-issue tables when an issue is linked — and the two callers map differently, so mapping here would force one of them to re-wrap. `InvestigationService` keeps its channel with `Effect.mapError` and provides its captured `Database`; the contention retry is unaffected, living in `makeDbExecute` rather than in the mapping. The fan-out workflow gets `runDbStep`, which backs `Database` with the run's own `PgConnectionScopeApi` — its `run` has exactly `execute`'s signature — so every statement stays on the single connection that run opens and closes in its `finally`, rather than dialing a socket per step. The one call site that bundled a lens-run update with the diagnosis write is now two calls: those were never atomic with each other, since `connection.run` is a span around a logical call and not a transaction.
Across 50 live issues in the internal org, `lease_holder` was null on every single one — `claim_error_issue` had never been called in production. The lease existed to stop two agents fixing the same bug and had never once been held. Claiming was not being skipped out of laziness. `triage -> in_review` is deliberately not a legal edge, but `propose_fix` transitioned straight to `in_review`, so proposing a fix on an untriaged issue — the state agents find issues in — failed with "Illegal transition from 'triage' to 'in_review'". It was the most common MCP error in the org, 18 occurrences in two days, and it failed *after* writing the fix_proposed event and linking the PR. Agents responded by hand-walking `transition_error_issue` instead, which works and leaves the lease empty. So the claim is now taken by the work rather than asked for beforehand: - `propose_fix` acquires the lease and walks to `in_review` via a route the matrix actually permits (`fixProposalRoute`), from any state an issue can be worked from. Everything refusable is refused before anything is written, so a rejected call no longer leaves a half-written proposal. - A transition to `in_progress` acquires the lease too — that is the path the agents actually used. Best-effort: another holder's lease is left alone rather than refusing a state change the caller is entitled to make. - `propose_fix` can now fail with a lease conflict, which is the duplicate-work collision the lease exists to catch, finally caught where agents work. - A `pr_url` that is not a pull request URL is a 400 instead of being swallowed and then reported back as `- PR: <url>`, which told an agent the fix was attached and would be verified when nothing had been linked at all. - An illegal transition now names the moves that would work. - `TERMINAL_WORKFLOW_STATES` -> `CLOSED_WORKFLOW_STATES`: `done` reopens to `regressed`, `verifying` and `triage`, so "no further transition is possible" was false. Both call sites mean "the work is over, drop the lease". - `describeWorkflowTransitions` stops advertising machine-owned targets the transition tool then rejects. Verified two ways: deterministic tests pinning the exact production failure (triage to in_review end to end, the lease actually held, no fix_proposed on a refused call, the lease-conflict path), and a new model-driven eval confirming a real model still reaches for the claiming tools.
… optional
The "Copy as prompt" text is what users paste into their coding agent, so it is
the instruction that actually reaches one. It read:
5. Optionally claim the issue first with `claim_error_issue`, and once you
have a patch, record it with `propose_fix`.
Last of five steps, after "propose a concrete fix", prefixed with "Optionally".
Not one issue in the internal org had ever been claimed.
The claim now leads and is unconditional, and the prompt closes with "Start by
claiming the issue." Also adds a `list_error_issue_events` step so an agent
reads Maple's own AI diagnosis and any earlier attempt before re-fixing a known
bug — guarded on the issue id, since that tool takes one and the legacy
errors-by-type page has none.
`maybeEnqueueTriage` and `enqueueFixVerification` carried byte-identical copies of the binding shape check, the `markFailed` update, the `Exit`-wrapped `create()`, and the outcome span annotations — including two separate declarations of `isFanoutWorkflowBinding`. What differs between them is the subject, the snapshot and the insert, which encode what a run is about. Starting it does not, so that half moves to `startInvestigationFanout`. The verification path keeps its `maple.verification.id` annotation through an `annotations` parameter: the tick reads the start outcome back and can answer `no_binding` with a terminal verdict that auto-closes the issue, so losing that attribute would leave the trace of the close saying nothing about no agent having run.
`error_issues.fingerprint_hash` is shared by three issue kinds: "error" rows
hold the decimal UInt64 from ClickHouse, while "alert" and "integration" rows
reuse the column for a synthetic key (`alert:{ruleId}:{groupKey}`,
`planetscale:{database}:{event}`). `toUInt64('alert:…')` does not skip that
value, it aborts the whole query — so one alert-backed issue in a batch of
twenty 500s the request.
#573 filtered the list in the errors hub, which fixed the surface that was
firing. This is the same guard on the side no other caller can bypass:
`fingerprintHashIn` drops what the warehouse could not hold, which is exact
rather than lenient — a synthetic key cannot have error events by construction,
so nothing a caller could have matched is lost. An all-synthetic list emits a
false literal, because `IN ()` is a ClickHouse syntax error.
Four copies of the workflow-state labels existed and the one in `workflow-badge.tsx` had drifted — "Wontfix" where the ring, the state picker and the bulk bar all said "Won't fix". Both duplicates now import `WORKFLOW_LABEL`; the badge keeps only its tones. A `verified` verdict on a high or critical issue deliberately does not close it — the verdict is posted and a human decides. The card said "The fix holds" while the issue sat open, with nothing to tell you whether Maple was still working or waiting on you. It now says.
…te flow The `maple://instructions` MCP resource covered read-only observability and stopped there — nothing about issues, leases, PR linking or verification. Agents were given the whole write surface and no map of it, which is half of why the flow was walked by hand. Adds an "Error Issue Lifecycle" section: the flow end to end, the six rules that matter (claim, read the timeline first, attach the PR, don't close an issue you linked a PR to, `regressed`/`verifying` are not yours to set, severity drives escalation), and a tool-picking list. Adds `docs/error-issue-lifecycle.md` as the human counterpart — the entity model, who owns which edge and why, the three places AI enters, and why verification asks its question inverted. It also records what happened when claiming was a rule an agent was merely told to follow, so the next tool that starts work on an issue takes the lease itself. The `transition_error_issue` and `link_pull_request` descriptions pick up the same rules, so an agent that never reads the resource still gets them.
Contributor
🍁 Maple PR previewWarning Preview cleanup could not be confirmed. The Alchemy teardown outcome was Final commit |
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
An error issue could carry a fix proposal but never a fix.
propose_fixaccepted apr_urlas free text on an event payload — nothing resolved it, nothing watched it, and nothing ever confirmed the fix worked. Issues sat inin_reviewuntil a human remembered them.Now a pull request is a durable link on an issue. When it merges, the issue enters a new
verifyingstate with a quiet window sized from its severity and its own pre-merge occurrence rate. When the window closes, a verification agent runs and either closes the issue automatically or posts a verdict for a human.The two things that made this cheap
Neither is new — both were already in the repo:
pull_requestwas the one event class the provider ignored.isRegression(error-tick-persistence.ts) already decides whether an occurrence is a real regression by asking whether its build was in the set the issue had been seen from at resolution time. That is exactly the "did the fix work" predicate — a merge just needs the same snapshot taken at merge instead of at close.So the verdict rests on a deterministic signal the agent corroborates, rather than one it has to invent.
Window sizing
verificationWindowMsderives the wait from the time it would have taken to accumulate 20 more occurrences at the pre-merge rate, then clamps it into a severity band (critical[30m, 24h]… low[12h, 14d]). A 10k/day error clears in 30 minutes; a 3/week error waits days. The rate is stored on the row so the UI can say why it is waiting six hours — a window length with no explanation reads as arbitrary, and so does the verdict that follows it.Verdicts
in_progressWorth a reviewer's attention
isRegressionwhich errs toward reopening. Erring the same way here would refute every fix arriving from a service that does not reportservice.version.diagnosed→not_fixedmapping reads backwards until you hold the question the run was asked. The agent is pointed at an error whose counts already look clean and asked to find anything contradicting the fix — so establishing a live cause means the fix did not hold. Documented at the mapping.apply-diagnosisis now subject-aware. A verification investigation links anissueId, so without this itsseverityAssessmentwould silently re-rank an issue a human had already triaged.maple-issue:<uuid>token, and the issue must belong to the webhook's org — a PR body is attacker-influenced text.Fixes #123is unsupported on purpose: Maple issue ids are UUIDs, so a bare integer can only name the host provider's own tracker.PullRequestEventSinkport bound in the composition roots, which also lets the sync-service tests assert forwarding without standing up the error-issue stack.verifyingtouches every surface that enumerates workflow states. All of them derive fromWORKFLOW_STATE_ORDER/WORKFLOW_TRANSITIONS, so the type checker enumerated them exactly — but each consumer is worth a look.Required manual step
The GitHub App must add the
Pull requestwebhook subscription. Until that box is ticked, nopull_requestdelivery arrives and the entire merge path is inert. Not something this PR can do.Testing
Fixes #123negatives).IssueFixVerificationServiceover PGlite: merge opens a window and moves the issue, redelivery does not open a second, unmerged close opens none, a merge on an already-closed issue still verifies, the off-baseline short-circuit, every verdict row above, cross-org auto-link rejection.bun typecheckgreen across all 40 packages;bun run lintclean; affected suites green (apps/apierrors/vcs/routes/runtime/workflows,apps/web,packages/domain,packages/query-engineincl. the SQL catalog gate).Not browser-verified end to end — that needs a seeded local error issue plus a signed webhook replay, which is a follow-up pass rather than something the type checker or tests cover.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.