From 867f763dc23b2e126c8da6aa3384fd5ac6bbb67f Mon Sep 17 00:00:00 2001 From: Courier Date: Mon, 5 Oct 2026 21:55:16 +0000 Subject: [PATCH 1/8] feat(groomer): create bounded child issues idempotently for decomposition plans (#1066) Extend the GroomingPlan child briefs into complete bounded implementation briefs, add a decomposition policy (no children while material design questions remain, at low confidence, or on a close), and apply the plan through a new idempotent children step: one child issue per brief keyed by GroomingChildClaim(childBriefKey = parent + normalized brief), so a retry after partial failure reuses created children and creates only missing ones. Children start as status/backlog with a machine marker and the full brief; the parent rides the umbrella label on the labels step and gets decomposed/followUpUrls + audit through the shared setDecompositionState helper used by the operator decompose route. --- docs/hosted-groomer.md | 17 +- .../migration.sql | 24 ++ prisma/schema.prisma | 23 ++ .../issues/actions/decompose/route.test.ts | 14 +- src/app/api/issues/actions/decompose/route.ts | 48 ++-- src/lib/decomposition.test.ts | 254 +++++++++++++++++ src/lib/decomposition.ts | 174 ++++++++++++ src/lib/groomer/evals/corpus.test.ts | 2 +- src/lib/groomer/evals/drafts.ts | 7 + src/lib/groomer/evals/harness.ts | 9 +- src/lib/groomer/mutation-applier.test.ts | 166 ++++++++++- src/lib/groomer/mutation-applier.ts | 260 +++++++++++++++++- src/lib/groomer/mutation-validator.test.ts | 76 +++++ src/lib/groomer/mutation-validator.ts | 26 ++ src/lib/groomer/plan-schema.test.ts | 26 ++ src/lib/groomer/plan-schema.ts | 7 + src/lib/groomer/plan.test.ts | 82 +++++- src/lib/groomer/plan.ts | 27 ++ src/lib/groomer/prompts/system-prompt.test.ts | 20 ++ src/lib/groomer/prompts/system-prompt.ts | 4 +- src/lib/groomer/run.test.ts | 3 + src/lib/groomer/run.ts | 25 +- src/lib/issue-filters.ts | 3 +- 23 files changed, 1243 insertions(+), 54 deletions(-) create mode 100644 prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql create mode 100644 src/lib/decomposition.test.ts create mode 100644 src/lib/decomposition.ts diff --git a/docs/hosted-groomer.md b/docs/hosted-groomer.md index 067f09ec..7b5b404b 100644 --- a/docs/hosted-groomer.md +++ b/docs/hosted-groomer.md @@ -173,6 +173,18 @@ The content excerpts are checked against is what the run's repository-context fe A close that fails any rule is a validation error and applies nothing. The applier re-checks the whole policy, grounding included, against the run's catalog before closing (see [What is applied, and in which order](#what-is-applied-and-in-which-order)); a close that fails there is withheld and the plan lands as backlog. +### Decomposition + +A plan may also split the issue into bounded children instead of (or alongside) promoting it. This is an independent decision from the close and ready promotions: a plan that decomposes is withheld only when its own decomposition policy fails, never because the close or ready policy did. The policy requires, all at once: + +- no close in the same plan — a decomposed parent is not closed; +- verdict confidence of at least `medium` (a low-confidence split is too speculative to fan out); +- no material uncertainty remaining (the same `verdict.uncertainties[].material` flag the close policy keys on). + +When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. The parent is then decorated as an `umbrella` (a separate label write) and recorded as decomposed — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically. + +A decomposition that fails any rule is withheld: no child is created and the parent is not decorated, and `withheld.decomposition` records why. + ### Compatibility `GroomingRun.validatedOutput` stores the full plan. `mutationPlan` keeps its existing fields and adds `planSchemaVersion`, `evidenceDigest`, `readiness`, `closeRecommendation`, `applicationKey`, `preconditions` and, when a policy withheld something, `withheld`. The run path applies mutations through `toGroomerOutput`, the legacy view that `POST /api/groomer/run` still returns as `output` (with the plan alongside as `plan`). A rejected plan's raw output and `validationErrors` are kept on the run. Runs recorded before the plan contract still render on `/automation/groomer`, marked `legacy`, with no readiness claim. @@ -207,8 +219,9 @@ The diff is computed against the live issue, never Dispatch's cache, and only wh 1. **labels**: priority/type changes and the derived status. For an `already_done` plan the status stays as it was here. 2. **comment**: at most one, with `@` mentions neutralized and a hidden `` marker at its end. Any marker the model wrote into its own text is stripped, and a marker only counts on a comment by an automation author, so nobody else can forge one to suppress or impersonate a groomer comment. 3. **title/body**: one write. A title is rewritten only when the current one is bad (the existing guard). The body is never replaced: enrichment goes into one Dispatch-managed section between `` and `` markers, appended after the human text on first write and replaced in place afterwards. Text outside the section is kept byte for byte. Enrichment still applies only when the human-authored text is sparse, and a body whose markers are unpaired or repeated is left alone. -4. **close**, only for an `already_done` plan that still satisfies the close policy. -5. **status/done**, only once the close has landed, so a failed close leaves the issue open in its previous (groomable) status rather than open with `status/done`, which the selector would skip forever. +4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and the parent is decorated as an `umbrella` and recorded as decomposed with the child URLs as its follow-ups (see [Decomposition](#decomposition)). +5. **close**, only for an `already_done` plan that still satisfies the close policy. +6. **status/done**, only once the close has landed, so a failed close leaves the issue open in its previous (groomable) status rather than open with `status/done`, which the selector would skip forever. Each step is recorded as `applied`, `replayed`, `noop`, `skipped` (comment cooldown), `failed` or `not_attempted` in `appliedMutations.steps`, alongside the existing `labelsUpdated`, `titleUpdated`, `bodyUpdated`, `commentUrl`, `commentSkippedReason`, `commentError`, `issueClosed` and `issueClosedError` fields. A run where a later step failed after earlier ones landed ends with `status: "partial"`, `retryable: true` and an `errorMessage` naming the failed step; the grooming fields and freshness baseline record only what actually landed. A run whose first needed write failed (nothing landed) fails as before. diff --git a/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql b/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql new file mode 100644 index 00000000..94f9b3b6 --- /dev/null +++ b/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql @@ -0,0 +1,24 @@ +-- One row per child brief the hosted groomer intends for a decomposed parent +-- (#1066). The unique childKey (parent issue + normalized child brief) is +-- the claim: creation is idempotent, so a retry after partial failure reuses +-- children whose rows already carry their URLs and creates only the missing +-- ones. +CREATE TABLE "GroomingChildClaim" ( + "id" TEXT NOT NULL, + "childKey" TEXT NOT NULL, + "parentIssueId" TEXT NOT NULL, + "repoFullName" TEXT NOT NULL, + "parentNumber" INTEGER NOT NULL, + "title" TEXT NOT NULL, + "childNumber" INTEGER, + "childUrl" TEXT, + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "updatedAt" TIMESTAMP(3) NOT NULL, + + CONSTRAINT "GroomingChildClaim_pkey" PRIMARY KEY ("id") +); + +ALTER TABLE "GroomingChildClaim" ADD CONSTRAINT "GroomingChildClaim_parentIssueId_fkey" FOREIGN KEY ("parentIssueId") REFERENCES "Issue"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +CREATE UNIQUE INDEX "GroomingChildClaim_childKey_key" ON "GroomingChildClaim"("childKey"); +CREATE INDEX "GroomingChildClaim_parentIssueId_idx" ON "GroomingChildClaim"("parentIssueId"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index d918ba58..53c27560 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -42,6 +42,7 @@ model Issue { leases Lease[] groomingRuns GroomingRun[] groomingApplications GroomingApplication[] + groomingChildClaims GroomingChildClaim[] // Escalated lane outcome tracking decomposed Boolean @default(false) @@ -604,6 +605,28 @@ model GroomingApplication { @@index([groomingRunId]) } +/// One row per child brief the hosted groomer intends for a decomposed parent +/// (#1066). The unique childKey (parent issue + normalized child brief) is +/// the claim: creation is idempotent, so a retry after partial failure reuses +/// children whose rows already carry their URLs and creates only the missing +/// ones. +model GroomingChildClaim { + id String @id @default(cuid()) + childKey String @unique + parentIssueId String + repoFullName String + parentNumber Int + title String @db.Text + childNumber Int? + childUrl String? @db.Text + createdAt DateTime @default(now()) + updatedAt DateTime @updatedAt + + parentIssue Issue @relation(fields: [parentIssueId], references: [id], onDelete: Cascade) + + @@index([parentIssueId]) +} + // --------------------------------------------------------------------------- // Issue sync run tracking (for scheduled sync endpoint) // --------------------------------------------------------------------------- diff --git a/src/app/api/issues/actions/decompose/route.test.ts b/src/app/api/issues/actions/decompose/route.test.ts index 57e52adc..9633ead6 100644 --- a/src/app/api/issues/actions/decompose/route.test.ts +++ b/src/app/api/issues/actions/decompose/route.test.ts @@ -4,6 +4,7 @@ import { makeDispatchEnvMock } from "@/test/route-helpers"; const { mocks } = vi.hoisted(() => ({ mocks: { findFirstIssue: vi.fn().mockResolvedValue(null), + findUniqueIssue: vi.fn().mockResolvedValue(null), updateIssue: vi.fn().mockResolvedValue(undefined), createAuditLog: vi.fn().mockResolvedValue({ id: "log-1" }), }, @@ -13,6 +14,7 @@ vi.mock("@/lib/prisma", () => ({ prisma: { issue: { findFirst: mocks.findFirstIssue, + findUnique: mocks.findUniqueIssue, update: mocks.updateIssue, }, auditLog: { @@ -50,14 +52,16 @@ describe("POST /api/issues/actions/decompose — actor attribution", () => { number: 66, labels: ["priority/p1"], }); - mocks.updateIssue.mockResolvedValue({ + const updated = { id: "issue-1", decomposed: true, decomposedAt: new Date(), decomposedBy: "example-agent", decomposedNote: null, followUpUrls: [], - }); + }; + mocks.updateIssue.mockResolvedValue(updated); + mocks.findUniqueIssue.mockResolvedValue(updated); mocks.createAuditLog.mockResolvedValue({ id: "log-1" }); }); @@ -213,14 +217,16 @@ describe("POST /api/issues/actions/decompose — reactivity", () => { number: 66, labels: ["priority/p1"], }); - mocks.updateIssue.mockResolvedValue({ + const updated = { id: "issue-1", decomposed: false, decomposedAt: null, decomposedBy: null, decomposedNote: null, followUpUrls: [], - }); + }; + mocks.updateIssue.mockResolvedValue(updated); + mocks.findUniqueIssue.mockResolvedValue(updated); mocks.createAuditLog.mockResolvedValue({ id: "log-1" }); }); diff --git a/src/app/api/issues/actions/decompose/route.ts b/src/app/api/issues/actions/decompose/route.ts index 1cb69048..d4008eb5 100644 --- a/src/app/api/issues/actions/decompose/route.ts +++ b/src/app/api/issues/actions/decompose/route.ts @@ -1,5 +1,6 @@ import { NextResponse } from "next/server"; import { errorResponse, handleApiError } from "@/lib/api-errors"; +import { setDecompositionState } from "@/lib/decomposition"; import { prisma } from "@/lib/prisma"; import { authorizeRequest, authErrorResponse } from "@/lib/auth"; import { resolveActor } from "@/lib/resolve-actor"; @@ -65,41 +66,28 @@ export async function POST(request: Request) { return errorResponse(`Issue #${issueNumber} not found in ${repo}`, 404); } - // Update decomposed state - const updated = await prisma.issue.update({ - where: { id: issue.id }, - data: { - decomposed, - decomposedAt: decomposed ? new Date() : null, - decomposedBy: decomposed ? actor : null, - decomposedNote: note ?? null, - followUpUrls: followUpUrls ?? [], - }, + // Persist the decomposition state and its audit entry through the shared + // helper, so the operator route and the hosted groomer write it identically + // (dispatch#1066). + await setDecompositionState(prisma, { + issue: { id: issue.id, labels: issue.labels }, + repoFullName: `${owner}/${name}`, + issueNumber, + actor, + decomposed, + note: note ?? null, + followUpUrls: followUpUrls ?? [], }); - // Log the action in audit trail - await prisma.auditLog.create({ - data: { - actor, - action: decomposed ? "issue_decomposed" : "issue_reactivated", - repoFullName: `${owner}/${name}`, - issueNumber, - issueId: issue.id, - beforeLabels: [...issue.labels], - afterLabels: [...issue.labels], - success: true, - notes: decomposed - ? `Issue marked as decomposed. Note: ${note ?? "none"}. Follow-up URLs: ${(followUpUrls ?? []).join(", ")}` - : `Issue reactivated (decomposed set to false)`, - }, - }); + // Re-read the issue so the response reflects the persisted state. + const updated = await prisma.issue.findUnique({ where: { id: issue.id } }); return NextResponse.json({ success: true, - issueId: updated.id, - decomposed: updated.decomposed, - decomposedAt: updated.decomposedAt, - followUpUrls: updated.followUpUrls, + issueId: updated?.id ?? issue.id, + decomposed: updated?.decomposed ?? decomposed, + decomposedAt: updated?.decomposedAt ?? null, + followUpUrls: updated?.followUpUrls ?? (followUpUrls ?? []), }, { status: 200 }); } catch (error) { return handleApiError("update decomposed state", error); diff --git a/src/lib/decomposition.test.ts b/src/lib/decomposition.test.ts new file mode 100644 index 00000000..5ea0ca69 --- /dev/null +++ b/src/lib/decomposition.test.ts @@ -0,0 +1,254 @@ +import { describe, expect, it } from "vitest"; +import { + CHILD_ISSUE_LABELS, + UMBRELLA_LABEL, + childBodyMarker, + childBriefKey, + renderChildIssueBody, + setDecompositionState, + type ChildIssueTarget, + type DecompositionStateClient, +} from "./decomposition"; +import type { ChildBrief } from "./groomer/plan"; + +const parent: ChildIssueTarget = { + repoFullName: "acme/storefront", + number: 260, + url: "https://github.com/acme/storefront/issues/260", +}; + +const fullChild: ChildBrief = { + title: "Add order search to the admin dashboard", + problem: "Admins cannot search orders from the dashboard.", + designDecision: "Search lives in a dedicated admin sub-route.", + verifiedCurrentBehavior: "Dashboard.tsx renders four unrelated legacy widgets.", + relevantPaths: ["src/admin/Dashboard.tsx", "src/admin/routes.ts"], + inScope: ["the order search query and results list"], + outOfScope: ["moving the dashboard to the new design system"], + dependencies: ["the refund approval queue child"], + acceptanceCriteria: ["an admin can search orders by id and see the match"], + tests: ["src/admin/orders-search.test.ts"], +}; + +const minimal: ChildBrief = { + title: "Fix redirect after reset", + problem: "Login drops the return URL after a password reset.", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [], + outOfScope: [], + dependencies: [], + acceptanceCriteria: ["a reset-then-login test lands on the return URL"], + tests: [], +}; + +describe("labels", () => { + it("uses the umbrella label and starts children in status/backlog", () => { + expect(UMBRELLA_LABEL).toBe("umbrella"); + expect(CHILD_ISSUE_LABELS).toEqual(["status/backlog"]); + }); +}); + +describe("childBriefKey", () => { + it("is stable for the same input", () => { + expect(childBriefKey(parent.repoFullName, parent.number, fullChild)).toBe( + childBriefKey(parent.repoFullName, parent.number, fullChild), + ); + }); + + it("is insensitive to repo case and surrounding whitespace", () => { + expect(childBriefKey(" ACME/Storefront ", 260, fullChild)).toBe(childBriefKey("acme/storefront", 260, fullChild)); + }); + + it("differs for a different parent issue", () => { + expect(childBriefKey(parent.repoFullName, 261, fullChild)).not.toBe(childBriefKey(parent.repoFullName, 260, fullChild)); + }); + + it("keeps the identity of an old-shape brief: missing fields hash as null/empty", () => { + const { + designDecision: _dd, + verifiedCurrentBehavior: _vcb, + relevantPaths: _rp, + inScope: _ins, + outOfScope: _oos, + dependencies: _deps, + tests: _tests, + ...oldShape + } = fullChild; + const oldShapeWithNulls: ChildBrief = { + ...oldShape, + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [], + outOfScope: [], + dependencies: [], + tests: [], + }; + expect(childBriefKey(parent.repoFullName, parent.number, oldShape as unknown as ChildBrief)).toBe( + childBriefKey(parent.repoFullName, parent.number, oldShapeWithNulls), + ); + }); +}); + +describe("childBodyMarker", () => { + it("embeds the child key in a dispatch-groomer HTML comment", () => { + expect(childBodyMarker("abc123")).toBe(""); + }); +}); + +describe("renderChildIssueBody", () => { + it("puts the marker first, the parent link, and a status/backlog footer", () => { + const body = renderChildIssueBody({ brief: minimal, parent, decompositionReason: null, childKey: "k" }); + expect(body.split("\n")[0]).toBe(""); + expect(body).toContain(`Parent: ${parent.url}`); + expect(body).toContain("This child starts as `status/backlog`"); + }); + + it("omits sections whose content is null or empty", () => { + const body = renderChildIssueBody({ brief: minimal, parent, decompositionReason: null, childKey: "k" }); + for (const heading of [ + "## Verified current behavior", + "## Current relevant code paths", + "## Settled design decision", + "## In scope", + "## Out of scope", + "## Dependencies", + "## Tests", + "Decomposition reason:", + ]) { + expect(body).not.toContain(heading); + } + }); + + it("renders a full brief deterministically, with checklist acceptance criteria", () => { + const input = { brief: fullChild, parent, decompositionReason: "The dashboard bundles four independent features.", childKey: "abc" }; + const expected = ` + +Parent: ${parent.url} + +## Problem +Admins cannot search orders from the dashboard. + +## Verified current behavior +Dashboard.tsx renders four unrelated legacy widgets. + +## Current relevant code paths +- src/admin/Dashboard.tsx +- src/admin/routes.ts + +## Settled design decision +Search lives in a dedicated admin sub-route. + +## In scope +- the order search query and results list + +## Out of scope +- moving the dashboard to the new design system + +## Dependencies +- the refund approval queue child + +## Acceptance criteria +- [ ] an admin can search orders by id and see the match + +## Tests +- src/admin/orders-search.test.ts + +--- +Created by the Dispatch hosted groomer when it decomposed ${parent.url}. +Decomposition reason: The dashboard bundles four independent features. +This child starts as \`status/backlog\`: it needs its own evidence-backed grooming pass before it is worker-ready. Do not implement it directly from this brief.`; + expect(renderChildIssueBody(input)).toBe(expected); + expect(renderChildIssueBody(input)).toBe(expected); + }); + + it("neutralizes @-mentions so model text never carries a live mention", () => { + const brief: ChildBrief = { ...minimal, problem: "Login drops the return URL; asked @user for input." }; + const body = renderChildIssueBody({ brief, parent, decompositionReason: null, childKey: "k" }); + expect(body).toContain("asked `@user` for input"); + }); +}); + +describe("setDecompositionState", () => { + function fakeClient() { + const calls = { update: [] as Record[], create: [] as Record[] }; + const client: DecompositionStateClient = { + issue: { + update: (args) => { + calls.update.push(args.data); + return Promise.resolve({}); + }, + }, + auditLog: { + create: (args) => { + calls.create.push(args.data); + return Promise.resolve({}); + }, + }, + }; + return { client, calls }; + } + + it("marks the issue decomposed and records the audit entry", async () => { + const labels = ["status/backlog", "priority/p2"]; + const { client, calls } = fakeClient(); + await setDecompositionState(client, { + issue: { id: "issue-1", labels }, + repoFullName: "acme/storefront", + issueNumber: 260, + actor: "hosted-groomer", + decomposed: true, + note: "split into four children", + followUpUrls: ["https://github.com/acme/storefront/issues/261"], + }); + labels.push("mutated-after"); + + expect(calls.update).toHaveLength(1); + expect(calls.update[0]).toMatchObject({ + decomposed: true, + decomposedBy: "hosted-groomer", + decomposedNote: "split into four children", + followUpUrls: ["https://github.com/acme/storefront/issues/261"], + }); + expect(calls.update[0]!.decomposedAt).toBeInstanceOf(Date); + expect(calls.create).toHaveLength(1); + expect(calls.create[0]).toMatchObject({ + actor: "hosted-groomer", + action: "issue_decomposed", + repoFullName: "acme/storefront", + issueNumber: 260, + issueId: "issue-1", + beforeLabels: ["status/backlog", "priority/p2"], + afterLabels: ["status/backlog", "priority/p2"], + success: true, + }); + expect(calls.create[0]!.notes).toBe( + "Issue marked as decomposed. Note: split into four children. Follow-up URLs: https://github.com/acme/storefront/issues/261", + ); + }); + + it("reactivates with nulls and the reactivation note", async () => { + const { client, calls } = fakeClient(); + await setDecompositionState(client, { + issue: { id: "issue-1", labels: [] }, + repoFullName: "acme/storefront", + issueNumber: 260, + actor: "operator", + decomposed: false, + note: null, + followUpUrls: [], + }); + + expect(calls.update[0]).toMatchObject({ + decomposed: false, + decomposedAt: null, + decomposedBy: null, + decomposedNote: null, + followUpUrls: [], + }); + expect(calls.create[0]).toMatchObject({ action: "issue_reactivated" }); + expect(calls.create[0]!.notes).toBe("Issue reactivated (decomposed set to false)"); + }); +}); diff --git a/src/lib/decomposition.ts b/src/lib/decomposition.ts new file mode 100644 index 00000000..82b46a4b --- /dev/null +++ b/src/lib/decomposition.ts @@ -0,0 +1,174 @@ +/** + * Shared decomposition helpers for the hosted groomer (dispatch#1066). + * + * When a GroomingPlan requires decomposition, the applier creates one bounded + * child issue per ChildBrief. These helpers are the contract both the + * operator route and the groomer build on: a stable child identity (so child + * creation is idempotent across retries), the child issue body, and the + * parent's decomposition-state persistence with its audit entry. + */ + +import { createHash } from "crypto"; +import type { ChildBrief } from "./groomer/plan"; +import { neutralizeMentions } from "./groomer/sanitize"; + +/** Label that marks an issue as an audit/umbrella parent. */ +export const UMBRELLA_LABEL = "umbrella"; + +/** Labels a created child issue carries; it is not worker-ready until groomed. */ +export const CHILD_ISSUE_LABELS: readonly string[] = ["status/backlog"]; + +/** + * The brief in canonical hashing form: exactly the ten ChildBrief fields in + * interface order, strings trimmed, nulls and missing list items kept as + * null/empty. Missing optional fields therefore hash identically to explicit + * nulls/empty arrays, so a child stored before the brief was widened keeps + * its identity (dispatch#1066). + */ +function canonicalChildBrief(brief: ChildBrief): Record { + // A field stored before the brief was widened is `undefined`, not null; + // both canonicalize to null so old briefs keep their identity. + const text = (value: string | null | undefined): string | null => + value === null || value === undefined ? null : String(value).trim(); + const list = (value: readonly string[] | null | undefined): string[] => (value ?? []).map((item) => String(item).trim()); + return { + title: String(brief.title).trim(), + problem: String(brief.problem).trim(), + designDecision: text(brief.designDecision), + verifiedCurrentBehavior: text(brief.verifiedCurrentBehavior), + relevantPaths: list(brief.relevantPaths), + inScope: list(brief.inScope), + outOfScope: list(brief.outOfScope), + dependencies: list(brief.dependencies), + acceptanceCriteria: list(brief.acceptanceCriteria), + tests: list(brief.tests), + }; +} + +/** + * Stable identity for a child: sha256 over the normalized parent + * (repo lowercased/trimmed + issue number) and the canonical child brief. + * The GroomingChildClaim row is keyed by it, which is what makes child + * creation idempotent. + */ +export function childBriefKey(repoFullName: string, parentIssueNumber: number, brief: ChildBrief): string { + const payload = { + v: 1, + repo: repoFullName.trim().toLowerCase(), + parent: parentIssueNumber, + brief: canonicalChildBrief(brief), + }; + return createHash("sha256").update(JSON.stringify(payload)).digest("hex"); +} + +/** Machine-readable marker that identifies a groomer-created child issue. */ +export function childBodyMarker(childKey: string): string { + return ``; +} + +export interface ChildIssueTarget { + repoFullName: string; + number: number; + url: string; +} + +/** + * Deterministic Markdown body for a groomer-created child issue: the child + * marker first, then the bounded brief, then a footer that tells the next + * groomer this child is not yet worker-ready. Same input, byte-identical + * output. Model text never carries live @-mentions, so the result runs + * through neutralizeMentions. + */ +export function renderChildIssueBody(input: { + brief: ChildBrief; + parent: ChildIssueTarget; + decompositionReason: string | null; + childKey: string; +}): string { + const { brief, parent, decompositionReason, childKey } = input; + const lines: string[] = [childBodyMarker(childKey), "", `Parent: ${parent.url}`]; + + const section = (heading: string, bodyLines: string[]): void => { + if (bodyLines.length === 0) return; + lines.push("", `## ${heading}`, ...bodyLines); + }; + const bullets = (items: readonly string[]): string[] => items.map((item) => `- ${item}`); + + section("Problem", [brief.problem]); + section("Verified current behavior", brief.verifiedCurrentBehavior === null ? [] : [brief.verifiedCurrentBehavior]); + section("Current relevant code paths", bullets(brief.relevantPaths)); + section("Settled design decision", brief.designDecision === null ? [] : [brief.designDecision]); + section("In scope", bullets(brief.inScope)); + section("Out of scope", bullets(brief.outOfScope)); + section("Dependencies", bullets(brief.dependencies)); + section("Acceptance criteria", brief.acceptanceCriteria.map((criterion) => `- [ ] ${criterion}`)); + section("Tests", bullets(brief.tests)); + + const footer: string[] = ["", "---", `Created by the Dispatch hosted groomer when it decomposed ${parent.url}.`]; + if (decompositionReason !== null) footer.push(`Decomposition reason: ${decompositionReason}`); + footer.push( + "This child starts as `status/backlog`: it needs its own evidence-backed grooming pass before it is worker-ready. Do not implement it directly from this brief.", + ); + lines.push(...footer); + + // Trim trailing whitespace (per line and at the end) so the body is + // canonical regardless of how the brief strings were produced. + const rendered = lines.join("\n").replace(/[ \t]+$/gm, "").trim(); + return neutralizeMentions(rendered); +} + +/** + * The slice of the Prisma client the shared decomposition persistence needs; + * the real client satisfies it structurally. + */ +export interface DecompositionStateClient { + issue: { update(args: { where: { id: string }; data: Record }): Promise }; + auditLog: { create(args: { data: Record }): Promise }; +} + +/** + * Persist a parent's decomposition state and record the audit entry + * (dispatch#1066). Shared by the operator decompose route and the hosted + * groomer so decomposition persistence and audit stay in one place. + */ +export async function setDecompositionState( + client: DecompositionStateClient, + input: { + issue: { id: string; labels: readonly string[] }; + repoFullName: string; + issueNumber: number; + actor: string; + decomposed: boolean; + note: string | null; + followUpUrls: string[]; + }, +): Promise { + const { issue, repoFullName, issueNumber, actor, decomposed, note, followUpUrls } = input; + + await client.issue.update({ + where: { id: issue.id }, + data: { + decomposed, + decomposedAt: decomposed ? new Date() : null, + decomposedBy: decomposed ? actor : null, + decomposedNote: note ?? null, + followUpUrls, + }, + }); + + await client.auditLog.create({ + data: { + actor, + action: decomposed ? "issue_decomposed" : "issue_reactivated", + repoFullName, + issueNumber, + issueId: issue.id, + beforeLabels: [...issue.labels], + afterLabels: [...issue.labels], + success: true, + notes: decomposed + ? `Issue marked as decomposed. Note: ${note ?? "none"}. Follow-up URLs: ${followUpUrls.join(", ")}` + : "Issue reactivated (decomposed set to false)", + }, + }); +} diff --git a/src/lib/groomer/evals/corpus.test.ts b/src/lib/groomer/evals/corpus.test.ts index 00f5d8ef..48cfef09 100644 --- a/src/lib/groomer/evals/corpus.test.ts +++ b/src/lib/groomer/evals/corpus.test.ts @@ -161,7 +161,7 @@ describe("grooming corpus", () => { expect(first.writes.closes).toBe(1); expect(first.writes.comments).toHaveLength(1); expect(replay.mutationPlan?.applicationKey).toBe(first.mutationPlan?.applicationKey); - expect(replay.writes).toEqual({ labels: [], titleBody: [], comments: [], closes: 0 }); + expect(replay.writes).toEqual({ labels: [], titleBody: [], comments: [], closes: 0, children: [] }); }); }); diff --git a/src/lib/groomer/evals/drafts.ts b/src/lib/groomer/evals/drafts.ts index 4c7e3f55..673f738e 100644 --- a/src/lib/groomer/evals/drafts.ts +++ b/src/lib/groomer/evals/drafts.ts @@ -146,6 +146,13 @@ export function children(titles: string[]): ChildBrief[] { return titles.map((title) => ({ title, problem: `${title}, as its own bounded change.`, + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [title.toLowerCase()], + outOfScope: [], + dependencies: [], acceptanceCriteria: [`${title} works end to end`], + tests: [], })); } diff --git a/src/lib/groomer/evals/harness.ts b/src/lib/groomer/evals/harness.ts index cbda4c44..55cd0af7 100644 --- a/src/lib/groomer/evals/harness.ts +++ b/src/lib/groomer/evals/harness.ts @@ -35,6 +35,7 @@ export interface GitHubWrites { titleBody: Array<{ title?: string; body?: string | null }>; comments: string[]; closes: number; + children: Array<{ number: number; url: string }>; } export interface GroomingOutcome { @@ -123,7 +124,7 @@ export async function runCandidate( candidate: CaseCandidate, options: RunCandidateOptions = {}, ): Promise { - const writes: GitHubWrites = { labels: [], titleBody: [], comments: [], closes: 0 }; + const writes: GitHubWrites = { labels: [], titleBody: [], comments: [], closes: 0, children: [] }; let validation: GroomingPlanValidationResult | null = null; let catalog: EvidenceCatalog | null = null; let context = ""; @@ -241,6 +242,12 @@ export async function runCandidate( closeIssue: async () => { writes.closes++; }, + createIssue: async (repoFullName, input) => { + const number = 1000 + writes.children.length; + const url = `https://github.com/${repoFullName}/issues/${number}`; + writes.children.push({ number, url }); + return { number, html_url: url }; + }, findActiveLeases: async () => [], upsertLease: async () => ({ created: true, lease: { id: "lease-1" } }), releaseLease: async () => ({ id: "lease-1" }), diff --git a/src/lib/groomer/mutation-applier.test.ts b/src/lib/groomer/mutation-applier.test.ts index 613374f1..fea3aeb4 100644 --- a/src/lib/groomer/mutation-applier.test.ts +++ b/src/lib/groomer/mutation-applier.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from "vitest"; +import { childBriefKey } from "@/lib/decomposition"; import type { GroomingEvidenceSnapshot } from "./evidence-snapshot"; import { buildEvidenceCatalog } from "./plan-evidence"; import { collectPinnedReadContent } from "./close-grounding"; @@ -21,6 +22,7 @@ import { type ApplierGitHub, type ApplyInput, type ApplySteps, + type ChildClaimRecord, type GroomingMutationDiff, } from "./mutation-applier"; @@ -127,6 +129,46 @@ function planFor(d: GroomingPlanDraft, snap = snapshot()): GroomingPlan { return result.plan!; } +/** A plan that splits the issue into two bounded children (dispatch#1066). */ +function decomposeDraft(): GroomingPlanDraft { + // A ready `implementation` plan may not decompose, so the split lands as a + // non-ready (backlog) verdict. + return { + ...draft({ actionability: "backlog", lane: { id: "backlog", confidence: "high", reason: "bounded" } }), + implementationBrief: null, + decomposition: { + required: true, + reason: "splits into two bounded children", + childBriefs: [ + { + title: "Child issue A", + problem: "Child A problem", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: ["A"], + outOfScope: [], + dependencies: [], + acceptanceCriteria: ["A works"], + tests: [], + }, + { + title: "Child issue B", + problem: "Child B problem", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: ["B"], + outOfScope: [], + dependencies: [], + acceptanceCriteria: ["B works"], + tests: [], + }, + ], + }, + }; +} + function live(snap = snapshot()): LiveIssueState { return { title: snap.issue.title, body: snap.issue.body, labels: snap.issue.labels, state: snap.issue.state }; } @@ -201,6 +243,17 @@ describe("computeMutationDiff", () => { expect(diff.labelsAfter.filter((l) => l.startsWith("status/"))).toEqual(["status/done"]); }); + it("keeps a non-status label in the labels step on the done (close) branch", () => { + // The umbrella is a non-status label; `umbrella` itself is not a model-usable + // label, so a representative allowed non-status label (type/bug) proves the + // done branch preserves non-status labels — the same derivation that keeps + // the umbrella once the decomposition adds it to labelsAfter. + const diff = diffFor(alreadyDone({ labelsToAdd: ["type/bug"] })); + expect(diff.close).toBe(true); + expect(diff.labelsAfter).toContain("type/bug"); + expect(diff.labelsStep).toContain("type/bug"); + }); + it("neutralizes @-mentions in a rewritten title and in the managed body section", () => { const snap = snapshot({ issue: { ...snapshot().issue, title: "P0" } }); const diff = diffFor(draft({}, { proposedTitle: "Ask @alice about the redirect", proposedBody: "cc @bob" }), snap); @@ -288,11 +341,16 @@ describe("computeApplicationKey", () => { // ─── applyGroomingMutations ────────────────────────────────────────────────── -function memoryStore(initial?: ApplicationRecord): ApplicationStore & { rows: Map } { +function memoryStore(initial?: ApplicationRecord): ApplicationStore & { + rows: Map; + children: Map; +} { const rows = new Map(); + const children = new Map(); if (initial) rows.set(initial.applicationKey, initial); return { rows, + children, find: async (key) => rows.get(key) ?? null, claim: async (input) => { const existing = rows.get(input.applicationKey) ?? null; @@ -320,6 +378,21 @@ function memoryStore(initial?: ApplicationRecord): ApplicationStore & { rows: Ma row.updatedAt = new Date(); return true; }, + claimChild: async (input) => { + const existing = children.get(input.childKey) ?? null; + if (!existing) { + children.set(input.childKey, { childKey: input.childKey, childNumber: null, childUrl: null }); + } + return { existing: existing ? { ...existing } : null }; + }, + saveChild: async (childKey, data) => { + const row = children.get(childKey)!; + row.childNumber = data.childNumber; + row.childUrl = data.childUrl; + }, + // The shared helper's persistence is exercised by the route and + // integration tests; the in-memory fake records nothing. + setDecompositionState: async () => {}, }; } @@ -339,6 +412,10 @@ function fakeGitHub(overrides: Partial = {}) { closeIssue: vi.fn(async () => { calls.push("close"); }), + createIssue: vi.fn(async (_r, input) => { + calls.push(`child:${input.title}`); + return { number: 43, url: "https://github.com/org/repo/issues/43" }; + }), fetchRecentComments: vi.fn(async () => []), ...overrides, }; @@ -350,6 +427,7 @@ function applyInput(diff: GroomingMutationDiff, overrides: Partial = repoFullName: "org/repo", issueNumber: 42, issueId: "issue-42", + parentUrl: "https://github.com/org/repo/issues/42", groomingRunId: "run-1", applicationKey: KEY, diff, @@ -604,6 +682,85 @@ describe("applyGroomingMutations", () => { }); }); +describe("applyGroomingMutations → decomposition", () => { + it("creates each child issue, rides the umbrella on the labels step, and records the child links", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + expect(diff.children).not.toBeNull(); + // The umbrella decoration rides the labels step, not a second write. + expect(diff.labelsAfter).toContain("umbrella"); + expect(diff.labelsStep).toContain("umbrella"); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + expect(github.createIssue).toHaveBeenCalledTimes(2); + expect(store.children.size).toBe(2); + // The single labels write carries the umbrella; the children step makes none. + expect(github.updateLabels).toHaveBeenCalledTimes(1); + expect(github.updateLabels).toHaveBeenCalledWith("org/repo", 42, expect.arrayContaining(["umbrella"])); + // The children step carries the created child LINKS (key/number/url), not counts. + const step = result.steps.children!; + expect(step.status).toBe("applied"); + expect(step.children?.created).toHaveLength(2); + expect(step.children?.reused).toHaveLength(0); + for (const link of step.children!.created) { + expect(link.key).toBeTypeOf("string"); + expect(link.number).toBeTypeOf("number"); + expect(link.url).toBeTypeOf("string"); + } + // ApplyResult.labels (freshness baseline / audit) includes the umbrella. + expect(result.labels).toContain("umbrella"); + // ApplyResult.children carries the created links in brief order. + expect(result.children).toHaveLength(2); + }); + + it("reuses a child an earlier attempt already created, creating only the missing one", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const briefs = diff.children!.briefs; + // Simulate the first child having been created by an earlier attempt. + const firstKey = childBriefKey("org/repo", 42, briefs[0]); + store.children.set(firstKey, { childKey: firstKey, childNumber: 99, childUrl: "https://github.com/org/repo/issues/99" }); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(github.createIssue).toHaveBeenCalledTimes(1); + const step = result.steps.children!; + expect(step.children?.created).toHaveLength(1); + expect(step.children?.reused).toEqual([{ key: firstKey, number: 99, url: "https://github.com/org/repo/issues/99" }]); + // ApplyResult.children is in brief order: the reused first child, then the created one. + expect(result.children.map((l) => l.number)).toEqual([99, 43]); + }); + + it("surfaces the created child links again on a replay, without re-creating them", async () => { + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const first = fakeGitHub(); + const applied = await applyGroomingMutations(applyInput(diff), first.github, store); + expect(applied.outcome).toBe("applied"); + expect(applied.children).toHaveLength(2); + // A replay of the same applied key re-surfaces the links and writes nothing. + const second = fakeGitHub(); + const replayed = await applyGroomingMutations(applyInput(diff), second.github, store); + expect(replayed.outcome).toBe("replayed"); + expect(second.github.createIssue).not.toHaveBeenCalled(); + expect(replayed.children).toEqual(applied.children); + }); + + it("creates no children when the decomposition is withheld by policy", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + // A low-confidence split is withheld; no children land. + const base = decomposeDraft(); + const diff = diffFor({ ...base, verdict: { ...base.verdict, confidence: "low" } }); + expect(diff.children).toBeNull(); + expect(diff.withheld.decomposition).toBeDefined(); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(github.createIssue).not.toHaveBeenCalled(); + expect(store.children.size).toBe(0); + expect(result.steps.children).toMatchObject({ status: "noop" }); + }); +}); + function addCommentBody(github: ApplierGitHub): string { return (github.addComment as ReturnType).mock.calls[0][2] as string; } @@ -631,6 +788,13 @@ describe("makePrismaApplicationStore", () => { updateMany: vi.fn(async () => ({ count: 1 })), }, groomingRun: { findFirst: vi.fn(async () => null) }, + groomingChildClaim: { + findUnique: vi.fn(async (): Promise => null), + create: vi.fn(async ({ data }: { data: Record }) => data), + update: vi.fn(async () => ({})), + }, + issue: { update: vi.fn(async () => ({})) }, + auditLog: { create: vi.fn(async () => ({})) }, }; } diff --git a/src/lib/groomer/mutation-applier.ts b/src/lib/groomer/mutation-applier.ts index 9036689e..153d4b1d 100644 --- a/src/lib/groomer/mutation-applier.ts +++ b/src/lib/groomer/mutation-applier.ts @@ -18,13 +18,28 @@ import { createHash } from "crypto"; import type { StatusLabel } from "@/types"; +import { + childBriefKey, + CHILD_ISSUE_LABELS, + renderChildIssueBody, + setDecompositionState, + UMBRELLA_LABEL, + type ChildIssueTarget, + type DecompositionStateClient, +} from "@/lib/decomposition"; import { getBacklogLane } from "@/lib/lane-config"; import { isAutomationAuthor } from "./context"; import { neutralizeMentions } from "./sanitize"; -import { inFlightStatus, toGroomerOutput, type GroomingPlan } from "./plan"; +import { inFlightStatus, toGroomerOutput, type ChildBrief, type GroomingPlan } from "./plan"; import type { EvidenceCatalog } from "./plan-evidence"; import type { GroomerOutput } from "./schema"; -import { evaluateClosePolicy, evaluateReadyPolicy, type LiveComment, type LiveIssueState } from "./mutation-validator"; +import { + evaluateClosePolicy, + evaluateDecompositionPolicy, + evaluateReadyPolicy, + type LiveComment, + type LiveIssueState, +} from "./mutation-validator"; export const APPLICATION_KEY_VERSION = 1; export const MAX_GITHUB_COMMENT_CHARS = 4096; @@ -163,11 +178,21 @@ export function groomerCommentKey(comment: Pick) // ─── Diff ───────────────────────────────────────────────────────────────────── +/** + * The plan's decomposition intent, when the plan splits the issue into + * bounded children (dispatch#1066). `null` when the plan does not decompose + * (or the decomposition was withheld at apply time). + */ +export interface DecompositionIntent { + briefs: ChildBrief[]; + reason: string | null; +} + export interface GroomingMutationDiff { /** The effective legacy view the diff was computed from (after any withholding). */ output: GroomerOutput; - /** Why an intended close or ready promotion was withheld at apply time. */ - withheld: { close?: string[]; ready?: string[] }; + /** Why an intended close, ready promotion or decomposition was withheld at apply time. */ + withheld: { close?: string[]; ready?: string[]; decomposition?: string[] }; labelsBefore: string[]; /** Final label set. */ labelsAfter: string[]; @@ -183,6 +208,8 @@ export interface GroomingMutationDiff { /** Why proposed body enrichment was not applied, when it was not. */ bodySkippedReason: string | null; close: boolean; + /** The plan's decomposition intent; null when the plan does not decompose. */ + children: DecompositionIntent | null; } export interface MutationDiffInput { @@ -254,6 +281,23 @@ export function computeMutationDiff(input: MutationDiffInput): GroomingMutationD } } + // The decomposition is a separate, independent decision from the close and + // ready promotions above: a plan that splits the issue into children is + // withheld only when its own decomposition policy fails (a close in the + // same plan, low confidence, or a material uncertainty), never because the + // close or ready policy did. + let children: DecompositionIntent | null = + plan.decomposition.required && plan.decomposition.childBriefs.length > 0 + ? { briefs: plan.decomposition.childBriefs, reason: plan.decomposition.reason } + : null; + if (children) { + const reasons = evaluateDecompositionPolicy(plan); + if (reasons.length > 0) { + withheld.decomposition = reasons; + children = null; + } + } + const output = toGroomerOutput(effective, live.labels); const done = effective.verdict.actionability === "already_done"; const labelsBefore = [...live.labels]; @@ -264,6 +308,13 @@ export function computeMutationDiff(input: MutationDiffInput): GroomingMutationD effective.mutations.status, ); + // A policy-passing decomposition decorates the parent as an umbrella. That + // label rides the labels step (not a second write in the children step) so + // the application key, ApplyResult.labels and the freshness baseline all + // include it — otherwise the next snapshot would read the groomer's own + // umbrella as an external change. + if (children !== null && !labelsAfter.includes(UMBRELLA_LABEL)) labelsAfter.push(UMBRELLA_LABEL); + // status/done lands only after the close succeeds, so a failed close // leaves the issue open with its previous status (still groomable), // never open with status/done (which the selector skips forever). An issue @@ -313,6 +364,7 @@ export function computeMutationDiff(input: MutationDiffInput): GroomingMutationD body, bodySkippedReason, close: done && live.state === "open" && inFlightStatus(live.labels) === null, + children, }; } @@ -341,6 +393,12 @@ export function computeApplicationKey(input: { title: diff.title, body: diff.body, close: diff.close, + // The decomposition is part of the intent: its children are the stable + // childBriefKeys, so a change to the split (or its withholding) changes + // the application key. + children: diff.children + ? diff.children.briefs.map((brief) => childBriefKey(input.repoFullName, input.issueNumber, brief)) + : null, }, }; return createHash("sha256").update(JSON.stringify(canonical)).digest("hex"); @@ -348,7 +406,7 @@ export function computeApplicationKey(input: { // ─── Application ────────────────────────────────────────────────────────────── -export const APPLY_STEPS = ["labels", "comment", "content", "close", "done_label"] as const; +export const APPLY_STEPS = ["labels", "comment", "content", "children", "close", "done_label"] as const; export type ApplyStep = (typeof APPLY_STEPS)[number]; /** @@ -362,12 +420,27 @@ export type ApplyStep = (typeof APPLY_STEPS)[number]; */ export type ApplyStepStatus = "applied" | "replayed" | "noop" | "skipped" | "failed" | "not_attempted"; +/** A created-or-reused child issue, auditable from the run and the parent. */ +export interface ChildIssueLink { + key: string; + number: number; + url: string; +} + +/** The child issues a decomposition step created and reused this attempt. */ +export interface AppliedChildIssues { + created: ChildIssueLink[]; + reused: ChildIssueLink[]; +} + export interface ApplyStepResult { status: ApplyStepStatus; detail?: string; error?: string; commentUrl?: string | null; at?: string; + /** Children step: the child issues created and reused this attempt. */ + children?: AppliedChildIssues; } export type ApplySteps = Partial>; @@ -399,6 +472,20 @@ export interface ApplicationRecord { updatedAt?: Date | string | null; } +/** + * A GroomingChildClaim row, as the applier reads it: the stable child key and + * the created child's number/URL once the creation has been recorded. A claim + * whose childNumber is null means the creation write has not been recorded, so + * re-creating it covers a crashed attempt — though a create whose response was + * lost after GitHub accepted it can still duplicate, which the child body + * marker exists to surface for manual discovery. + */ +export interface ChildClaimRecord { + childKey: string; + childNumber: number | null; + childUrl: string | null; +} + export interface ApplicationStore { /** The record for a key, if one exists. Read-only (dry runs use it). */ find(applicationKey: string): Promise; @@ -419,6 +506,33 @@ export interface ApplicationStore { resume(applicationKey: string, seen: ApplicationRecord): Promise; /** Whether a hosted-groomer comment was recorded on this issue since `since`. */ hasRecentComment(issueId: string, since: Date): Promise; + /** + * Claim a child key, atomically (dispatch#1066). `existing` is the prior + * claim when the child was already created, so a retry reuses it rather than + * creating a second child. + */ + claimChild(input: { + childKey: string; + parentIssueId: string; + repoFullName: string; + parentNumber: number; + title: string; + }): Promise<{ existing: ChildClaimRecord | null }>; + /** Record the created child's number and URL on its claim. */ + saveChild(childKey: string, data: { childNumber: number; childUrl: string }): Promise; + /** + * Persist a parent's decomposition state and its audit entry (dispatch#1066), + * sharing the operator route's persistence path. + */ + setDecompositionState(input: { + issue: { id: string; labels: readonly string[] }; + repoFullName: string; + issueNumber: number; + actor: string; + decomposed: boolean; + note: string | null; + followUpUrls: string[]; + }): Promise; } export interface ApplierGitHub { @@ -428,12 +542,16 @@ export interface ApplierGitHub { closeIssue(repoFullName: string, issueNumber: number): Promise; /** Newest first; used to find a comment that landed before its write reported success. */ fetchRecentComments(repoFullName: string, issueNumber: number, max: number): Promise; + /** Open a new issue (a decomposition child); returns its number and URL. */ + createIssue(repoFullName: string, input: { title: string; body: string; labels?: string[] }): Promise<{ number: number; url: string }>; } export interface ApplyInput { repoFullName: string; issueNumber: number; issueId: string; + /** The parent issue's URL, so a child body can link back to it. */ + parentUrl: string; groomingRunId: string; applicationKey: string; diff: GroomingMutationDiff; @@ -452,6 +570,8 @@ export interface ApplyResult { commentUrl: string | null; /** Labels on GitHub after this attempt, as far as the recorded steps show. */ labels: string[]; + /** Child issues created or reused this attempt, in brief order. */ + children: ChildIssueLink[]; title: string | null; body: string | null; closed: boolean; @@ -507,7 +627,16 @@ export async function applyGroomingMutations( }); const prior = readSteps(existing?.steps); const claimedByRunId = existing && existing.groomingRunId !== input.groomingRunId ? existing.groomingRunId : null; - const nothing = { claimedByRunId, commentUrl: null, labels: diff.labelsBefore, title: null, body: null, closed: false, failure: null }; + const nothing = { + claimedByRunId, + commentUrl: null, + labels: diff.labelsBefore, + children: [] as ChildIssueLink[], + title: null, + body: null, + closed: false, + failure: null, + }; if (existing && existing.status !== "applied") { const updatedAt = existing.updatedAt ? new Date(existing.updatedAt).getTime() : NaN; @@ -521,6 +650,10 @@ export async function applyGroomingMutations( if (existing?.status === "applied") { const commentUrl = prior.comment?.commentUrl ?? null; + // A replay surfaces the children an earlier attempt created or reused. + const children = prior.children?.children + ? [...prior.children.children.created, ...prior.children.children.reused] + : []; return { outcome: "replayed", steps: Object.fromEntries( @@ -531,6 +664,7 @@ export async function applyGroomingMutations( ) as ApplySteps, ...nothing, commentUrl, + children, }; } @@ -652,12 +786,79 @@ export async function applyGroomingMutations( return { detail: Object.keys(fields).join("+") }; }, diff.bodySkippedReason ?? "title and body unchanged"); - // 4. Close, the highest-impact write, only after everything above landed. + // 4. Children (dispatch#1066): one bounded child issue per brief. Creation + // is idempotent through the GroomingChildClaim keyed by the childBriefKey + // (a retry reuses a created child and creates only the missing ones). The + // umbrella label rides the labels step above, so this step only creates + // the children and records the parent's decomposition state with their + // URLs as the follow-ups. It lands before the close, so a close is never + // applied on top of a decomposition that failed to land. + const children = diff.children; + let appliedChildren: ChildIssueLink[] = []; + await run( + "children", + children !== null, + async () => { + if (children === null) return; + const parentTarget: ChildIssueTarget = { repoFullName, number: issueNumber, url: input.parentUrl }; + const created: ChildIssueLink[] = []; + const reused: ChildIssueLink[] = []; + const links: ChildIssueLink[] = []; + for (const brief of children.briefs) { + const childKey = childBriefKey(repoFullName, issueNumber, brief); + const { existing } = await store.claimChild({ + childKey, + parentIssueId: input.issueId, + repoFullName, + parentNumber: issueNumber, + title: brief.title, + }); + if (existing && existing.childNumber !== null && existing.childUrl !== null) { + // An earlier attempt of this application already created this child. + const link: ChildIssueLink = { key: childKey, number: existing.childNumber, url: existing.childUrl }; + reused.push(link); + links.push(link); + } else { + const body = renderChildIssueBody({ + brief, + parent: parentTarget, + decompositionReason: children.reason, + childKey, + }); + const issue = await github.createIssue(repoFullName, { + title: brief.title, + body, + labels: [...CHILD_ISSUE_LABELS], + }); + await store.saveChild(childKey, { childNumber: issue.number, childUrl: issue.url }); + const link: ChildIssueLink = { key: childKey, number: issue.number, url: issue.url }; + created.push(link); + links.push(link); + } + } + appliedChildren = links; + // The umbrella label is already on GitHub from the labels step; record + // the decomposition state with the final label set and the child URLs. + await store.setDecompositionState({ + issue: { id: input.issueId, labels: diff.labelsAfter }, + repoFullName, + issueNumber, + actor: "hosted-groomer", + decomposed: true, + note: children.reason, + followUpUrls: links.map((child) => child.url), + }); + return { children: { created, reused }, detail: `${created.length} created, ${reused.length} reused` }; + }, + "no decomposition in the plan", + ); + + // 5. Close, the highest-impact write, only after everything above landed. await run("close", diff.close, async () => { await github.closeIssue(repoFullName, issueNumber); }, "no close in the plan"); - // 5. status/done, only once the issue is actually closed. + // 6. status/done, only once the issue is actually closed. const closed = landed(steps.close); await run( "done_label", @@ -681,6 +882,7 @@ export async function applyGroomingMutations( claimedByRunId, commentUrl, labels, + children: appliedChildren, title: landed(steps.content) && diff.title !== null ? diff.title : null, body: landed(steps.content) && diff.body !== null ? diff.body : null, closed, @@ -697,9 +899,20 @@ interface GroomingApplicationDelegateLike { updateMany(args: { where: Record; data: Record }): Promise<{ count: number }>; } -export interface ApplicationStoreClient { +/** + * The slice of the Prisma client the application store needs. Extends + * `DecompositionStateClient` so the store can also persist a parent's + * decomposition state through the shared helper (dispatch#1066). + */ +export interface ApplicationStoreClient extends DecompositionStateClient { groomingApplication: GroomingApplicationDelegateLike; groomingRun: { findFirst(args: unknown): Promise }; + /** GroomingChildClaim rows keying a decomposition's children (dispatch#1066). */ + groomingChildClaim: { + findUnique(args: { where: { childKey: string } }): Promise; + create(args: { data: Record }): Promise; + update(args: { where: { childKey: string }; data: Record }): Promise; + }; } function isUniqueViolation(err: unknown): boolean { @@ -760,5 +973,34 @@ export function makePrismaApplicationStore(client: ApplicationStoreClient): Appl }); return recent !== null && recent !== undefined; }, + async claimChild(input) { + // The unique childKey makes the claim atomic: a concurrent creator for + // the same child loses with P2002 and reads the winner's row, so two + // attempts never create the same child twice. + const child = client.groomingChildClaim; + const existing = await child.findUnique({ where: { childKey: input.childKey } }); + if (existing) return { existing }; + try { + await child.create({ + data: { + childKey: input.childKey, + parentIssueId: input.parentIssueId, + repoFullName: input.repoFullName, + parentNumber: input.parentNumber, + title: input.title, + }, + }); + return { existing: null }; + } catch (err) { + if (!isUniqueViolation(err)) throw err; + return { existing: await child.findUnique({ where: { childKey: input.childKey } }) }; + } + }, + async saveChild(childKey, data) { + await client.groomingChildClaim.update({ where: { childKey }, data }); + }, + async setDecompositionState(input) { + await setDecompositionState(client, input); + }, }; } diff --git a/src/lib/groomer/mutation-validator.test.ts b/src/lib/groomer/mutation-validator.test.ts index 3de3d3a2..edc55a88 100644 --- a/src/lib/groomer/mutation-validator.test.ts +++ b/src/lib/groomer/mutation-validator.test.ts @@ -5,6 +5,7 @@ import { collectPinnedReadContent } from "./close-grounding"; import { validateGroomingPlan, type GroomingPlan, type GroomingPlanDraft } from "./plan"; import { evaluateClosePolicy, + evaluateDecompositionPolicy, evaluateReadyPolicy, validateApplyPreconditions, type LiveComment, @@ -113,6 +114,35 @@ function alreadyDoneDraft(evidenceRefs: string[] = ["repo:src/auth/login.ts"]): }; } +/** A plan that splits the issue into one bounded child (dispatch#1066). */ +function decomposeDraft(): GroomingPlanDraft { + return { + ...readyDraft(), + // A ready `implementation` plan may not decompose, so the split lands as + // a non-ready (backlog) verdict. + verdict: { ...readyDraft().verdict, actionability: "backlog", lane: { id: "backlog", confidence: "high", reason: "bounded" } }, + implementationBrief: null, + decomposition: { + required: true, + reason: "splits into two bounded children", + childBriefs: [ + { + title: "Child issue A", + problem: "Child A problem", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: ["A"], + outOfScope: [], + dependencies: [], + acceptanceCriteria: ["A works"], + tests: [], + }, + ], + }, + }; +} + function planFor(draft: GroomingPlanDraft, snap = snapshot()): GroomingPlan { const result = validateGroomingPlan(draft, { catalog: catalogFor(snap) }); if (!result.valid) throw new Error(result.errors!.join("; ")); @@ -503,3 +533,49 @@ describe("evaluateReadyPolicy", () => { expect(evaluateReadyPolicy(forged, catalogFor(snapshot()))).toContain("the plan's derived readiness is not ready"); }); }); + +describe("evaluateDecompositionPolicy", () => { + const plan = planFor(decomposeDraft()); + + it("allows a decisive decomposition with no close and no material uncertainty", () => { + expect(evaluateDecompositionPolicy(plan)).toEqual([]); + }); + + it("refuses a decomposition the plan also recommends closing for", () => { + const forged: GroomingPlan = { + ...plan, + mutations: { + ...plan.mutations, + close: { reason: "already_done", rationale: "done", evidenceRefs: ["repo:src/auth/login.ts"] }, + }, + }; + expect(evaluateDecompositionPolicy(forged)).toContain( + "the plan recommends closing the issue; a decomposed parent is not closed", + ); + }); + + it("refuses a decomposition whose confidence is low", () => { + const forged: GroomingPlan = { ...plan, verdict: { ...plan.verdict, confidence: "low" } }; + expect(evaluateDecompositionPolicy(forged)).toContain( + "verdict confidence is low; decomposition requires at least medium confidence", + ); + }); + + it("refuses a decomposition with a material uncertainty", () => { + const forged: GroomingPlan = { + ...plan, + verdict: { ...plan.verdict, uncertainties: [{ kind: "scope", question: "Which child owns the migration?", material: true }] }, + }; + expect(evaluateDecompositionPolicy(forged)).toContain( + "material uncertainty remains (verdict.uncertainties[0]): Which child owns the migration?", + ); + }); + + it("ignores a non-material uncertainty", () => { + const forged: GroomingPlan = { + ...plan, + verdict: { ...plan.verdict, uncertainties: [{ kind: "scope", question: "Nice to know", material: false }] }, + }; + expect(evaluateDecompositionPolicy(forged)).toEqual([]); + }); +}); diff --git a/src/lib/groomer/mutation-validator.ts b/src/lib/groomer/mutation-validator.ts index cfed0a75..a7406555 100644 --- a/src/lib/groomer/mutation-validator.ts +++ b/src/lib/groomer/mutation-validator.ts @@ -484,3 +484,29 @@ export function evaluateReadyPolicy(plan: GroomingPlan, catalog: EvidenceCatalog reasons.push(...evaluateReadiness(plan, catalog)); return reasons; } + +/** + * Decomposition re-checked at apply time (dispatch#1066). Returns every + * reason the plan's children may not be created; empty means they may. + * + * A decomposition splits one issue into bounded children, so it is only + * applied when the parent's analysis is decisive enough to trust the split: + * - the plan does not also recommend closing the issue (a closed parent has + * no children to carry its work); + * - the verdict confidence is not low; + * - no material uncertainty of any kind remains. + * The apply preconditions separately guarantee the issue is still open. + */ +export function evaluateDecompositionPolicy(plan: GroomingPlan): string[] { + const reasons: string[] = []; + if (plan.mutations.close) { + reasons.push("the plan recommends closing the issue; a decomposed parent is not closed"); + } + if (plan.verdict.confidence === "low") { + reasons.push("verdict confidence is low; decomposition requires at least medium confidence"); + } + plan.verdict.uncertainties.forEach((u, i) => { + if (u.material) reasons.push(`material uncertainty remains (verdict.uncertainties[${i}]): ${u.question}`); + }); + return reasons; +} diff --git a/src/lib/groomer/plan-schema.test.ts b/src/lib/groomer/plan-schema.test.ts index c82bff3e..891e26dd 100644 --- a/src/lib/groomer/plan-schema.test.ts +++ b/src/lib/groomer/plan-schema.test.ts @@ -83,6 +83,32 @@ describe("buildGroomingPlanResponseSchema", () => { expect(criteria.items.properties.excerpt).toMatchObject({ type: "string", minLength: 24, maxLength: 300 }); }); + it("describes the full child brief shape (dispatch#1066)", () => { + const childBriefs = schema.properties.decomposition.properties.childBriefs; + expect(childBriefs.maxItems).toBe(8); + expect(childBriefs.items.required).toEqual([ + "title", + "problem", + "designDecision", + "verifiedCurrentBehavior", + "relevantPaths", + "inScope", + "outOfScope", + "dependencies", + "acceptanceCriteria", + "tests", + ]); + const props = childBriefs.items.properties; + expect(props.title).toMatchObject({ type: "string", minLength: 10, maxLength: 200 }); + expect(props.problem).toMatchObject({ type: "string", maxLength: 1000 }); + expect(props.designDecision).toMatchObject({ anyOf: [{ type: "null" }, { type: "string", maxLength: 1000 }] }); + expect(props.verifiedCurrentBehavior).toMatchObject({ anyOf: [{ type: "null" }, { type: "string", maxLength: 1000 }] }); + for (const key of ["relevantPaths", "inScope", "outOfScope", "dependencies", "tests"]) { + expect(props[key], key).toMatchObject({ type: "array", maxItems: 12, items: { type: "string", maxLength: 300 } }); + } + expect(props.acceptanceCriteria).toMatchObject({ type: "array", maxItems: 8, items: { type: "string", maxLength: 300 } }); + }); + it("forces arrays empty when the catalog has no ids of the needed kind", () => { const bare = buildGroomingPlanResponseSchema(buildEvidenceCatalog({ ...snapshot, sources: [] })) as Node; expect(bare.properties.relatedWork.maxItems).toBe(0); diff --git a/src/lib/groomer/plan-schema.ts b/src/lib/groomer/plan-schema.ts index cf8dde57..6a88fe57 100644 --- a/src/lib/groomer/plan-schema.ts +++ b/src/lib/groomer/plan-schema.ts @@ -140,7 +140,14 @@ export function buildGroomingPlanResponseSchema(catalog?: EvidenceCatalog): Sche obj({ title: str(L.titleMax, L.titleMin), problem: str(L.text), + designDecision: nullable(str(L.text)), + verifiedCurrentBehavior: nullable(str(L.text)), + relevantPaths: list(str(L.shortText), L.listItems), + inScope: list(str(L.shortText), L.listItems), + outOfScope: list(str(L.shortText), L.listItems), + dependencies: list(str(L.shortText), L.listItems), acceptanceCriteria: list(str(L.shortText), L.childCriteria), + tests: list(str(L.shortText), L.listItems), }), L.childBriefs, ), diff --git a/src/lib/groomer/plan.test.ts b/src/lib/groomer/plan.test.ts index 6756dcf1..05da1f8e 100644 --- a/src/lib/groomer/plan.test.ts +++ b/src/lib/groomer/plan.test.ts @@ -325,7 +325,26 @@ describe("GroomingPlan readiness invariant", () => { it("rejects implementation-ready work that needs decomposition", () => { expectInvalid( - draft({ decomposition: { required: true, reason: "two changes", childBriefs: [{ title: "Fix redirect after reset", problem: "p", acceptanceCriteria: [] }] } }), + draft({ + decomposition: { + required: true, + reason: "two changes", + childBriefs: [ + { + title: "Fix redirect after reset", + problem: "p", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [], + outOfScope: [], + dependencies: [], + acceptanceCriteria: [], + tests: [], + }, + ], + }, + }), "readiness: decomposition.required is true", ); }); @@ -533,6 +552,67 @@ describe("GroomingPlan validation failures", () => { expectInvalid(notReady("backlog", { decomposition: { required: true, reason: "umbrella", childBriefs: [] } }), "decomposition.childBriefs"); }); + describe("child briefs (dispatch#1066)", () => { + const fullChild = { + title: "Add order search to the admin dashboard", + problem: "Admins cannot search orders from the dashboard.", + designDecision: "Search lives in a dedicated admin sub-route, not a dashboard widget.", + verifiedCurrentBehavior: "Dashboard.tsx renders four unrelated legacy widgets; routes.ts has no admin sub-routes.", + relevantPaths: ["src/admin/Dashboard.tsx", "src/admin/routes.ts"], + inScope: ["the order search query and results list"], + outOfScope: ["moving the dashboard to the new design system"], + dependencies: ["the refund approval queue child"], + acceptanceCriteria: ["an admin can search orders by id and see the match"], + tests: ["src/admin/orders-search.test.ts"], + }; + + it("round-trips every field of a full child brief", () => { + const plan = validPlan( + notReady("backlog", { decomposition: { required: true, reason: "umbrella", childBriefs: [fullChild] } }), + ); + expect(plan.decomposition.childBriefs).toEqual([fullChild]); + }); + + it("parses an old three-field brief with the new fields defaulted to null/empty", () => { + const old = { + ...notReady("backlog"), + decomposition: { + required: true, + reason: "umbrella", + childBriefs: [{ title: "Fix redirect after reset", problem: "p", acceptanceCriteria: ["a reset-then-login test passes"] }], + }, + } as unknown as Record; + const plan = validPlan(old); + expect(plan.decomposition.childBriefs).toEqual([ + { + title: "Fix redirect after reset", + problem: "p", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [], + outOfScope: [], + dependencies: [], + acceptanceCriteria: ["a reset-then-login test passes"], + tests: [], + }, + ]); + }); + + it("bounds child brief fields with the field path", () => { + const overList = { ...fullChild, relevantPaths: Array.from({ length: PLAN_LIMITS.listItems + 1 }, (_, i) => `path/${i}.ts`) }; + expectInvalid( + notReady("backlog", { decomposition: { required: true, reason: "umbrella", childBriefs: [overList] } }), + `decomposition.childBriefs[0].relevantPaths: must have at most ${PLAN_LIMITS.listItems} items`, + ); + const overText = { ...fullChild, designDecision: "x".repeat(PLAN_LIMITS.text + 1) }; + expectInvalid( + notReady("backlog", { decomposition: { required: true, reason: "umbrella", childBriefs: [overText] } }), + `decomposition.childBriefs[0].designDecision: must be at most ${PLAN_LIMITS.text} characters`, + ); + }); + }); + describe("close decisions", () => { const done = (patch: DraftPatch = {}) => draft({ diff --git a/src/lib/groomer/plan.ts b/src/lib/groomer/plan.ts index 545a38c0..c9c9368e 100644 --- a/src/lib/groomer/plan.ts +++ b/src/lib/groomer/plan.ts @@ -170,10 +170,30 @@ export interface GroomingMutationIntent { close: CloseRecommendation | null; } +/** + * One bounded child brief (dispatch#1066). Rich enough that a fresh worker + * with no other context can implement the child: problem/motivation, the + * current behavior the parent's analysis verified, the relevant code paths, + * the settled design decision, explicit scope, dependencies, deterministic + * acceptance criteria and tests. The child still gets its own evidence-backed + * grooming pass before worker admission; the brief orients, it does not + * certify. + */ export interface ChildBrief { title: string; problem: string; + /** The settled design decision this child implements; null when there is no decision left. */ + designDecision: string | null; + /** Current behavior verified by the parent's analysis; null when not verified. */ + verifiedCurrentBehavior: string | null; + /** Current relevant code paths the child touches or reads. */ + relevantPaths: string[]; + inScope: string[]; + outOfScope: string[]; + /** Descriptive only: dependencies between siblings or on external work. */ + dependencies: string[]; acceptanceCriteria: string[]; + tests: string[]; } export interface GroomingDecomposition { @@ -448,7 +468,14 @@ function parseDraft(data: Obj, r: Reader): GroomingPlanDraft { return { title: r.text(c.title, `${path}.title`, L.titleMax, L.titleMin), problem: r.text(c.problem, `${path}.problem`, L.text), + designDecision: r.optionalText(c.designDecision, `${path}.designDecision`, L.text), + verifiedCurrentBehavior: r.optionalText(c.verifiedCurrentBehavior, `${path}.verifiedCurrentBehavior`, L.text), + relevantPaths: r.textList(c.relevantPaths, `${path}.relevantPaths`, L.listItems, L.shortText), + inScope: r.textList(c.inScope, `${path}.inScope`, L.listItems, L.shortText), + outOfScope: r.textList(c.outOfScope, `${path}.outOfScope`, L.listItems, L.shortText), + dependencies: r.textList(c.dependencies, `${path}.dependencies`, L.listItems, L.shortText), acceptanceCriteria: r.textList(c.acceptanceCriteria, `${path}.acceptanceCriteria`, L.childCriteria, L.shortText), + tests: r.textList(c.tests, `${path}.tests`, L.listItems, L.shortText), }; }), }; diff --git a/src/lib/groomer/prompts/system-prompt.test.ts b/src/lib/groomer/prompts/system-prompt.test.ts index f981a86f..5e5a46e3 100644 --- a/src/lib/groomer/prompts/system-prompt.test.ts +++ b/src/lib/groomer/prompts/system-prompt.test.ts @@ -136,6 +136,26 @@ describe("buildGroomerSystemPrompt", () => { expect(prompt).toContain("decomposition.required is false"); }); + it("describes the full child brief shape and when children may be emitted (dispatch#1066)", () => { + const prompt = buildGroomerSystemPrompt(baseParams); + for (const field of [ + "designDecision", + "verifiedCurrentBehavior", + "relevantPaths", + "inScope", + "outOfScope", + "dependencies", + "acceptanceCriteria", + "tests", + ]) { + expect(prompt, field).toContain(field); + } + expect(prompt).toContain("complete bounded implementation brief"); + expect(prompt).toContain("never emit child briefs while a material design question remains unresolved"); + expect(prompt).toContain("that work goes to the escalation lane instead"); + expect(prompt).toContain("start as `status/backlog`"); + }); + it("lets unknowns be recorded instead of guessed", () => { const prompt = buildGroomerSystemPrompt(baseParams); expect(prompt).toContain("Unknown is an answer."); diff --git a/src/lib/groomer/prompts/system-prompt.ts b/src/lib/groomer/prompts/system-prompt.ts index b6e8ec04..8de84e80 100644 --- a/src/lib/groomer/prompts/system-prompt.ts +++ b/src/lib/groomer/prompts/system-prompt.ts @@ -65,7 +65,7 @@ Return ONLY valid JSON: a grooming plan with this shape (the response schema set "decomposition": { "required": false, "reason": null, "childBriefs": [] }, "relatedWork": [] } -Use null for implementationBrief when you cannot write one honestly (for example needs_info or design work). "close" is null or { "reason": "already_done|duplicate|superseded", "rationale": "...", "evidenceRefs": [...], "criteria": [{ "criterion": "...", "evidenceRef": "repo:path/you/read.ts", "excerpt": "..." }] } (criteria is [] unless the reason is already_done). relatedWork entries are { "ref": "github:issue:owner/repo#12", "relation": "duplicate_of|superseded_by|related", "note": "..." }: the ref is always a "github:" id from the evidence list. childBriefs entries are { "title": "...", "problem": "...", "acceptanceCriteria": ["..."] }. +Use null for implementationBrief when you cannot write one honestly (for example needs_info or design work). "close" is null or { "reason": "already_done|duplicate|superseded", "rationale": "...", "evidenceRefs": [...], "criteria": [{ "criterion": "...", "evidenceRef": "repo:path/you/read.ts", "excerpt": "..." }] } (criteria is [] unless the reason is already_done). relatedWork entries are { "ref": "github:issue:owner/repo#12", "relation": "duplicate_of|superseded_by|related", "note": "..." }: the ref is always a "github:" id from the evidence list. childBriefs entries are { "title": "...", "problem": "what the child must fix, in repo terms", "designDecision": "the settled design decision the child implements" or null, "verifiedCurrentBehavior": "what the parent's analysis verified the code does today" or null, "relevantPaths": ["repo:path/the/child/touches.ts"], "inScope": ["what the child may do"], "outOfScope": ["what the child must not do"], "dependencies": ["sibling or external work this child depends on"], "acceptanceCriteria": ["deterministic, observable result"], "tests": ["test to add or update"] }. Evidence rules: - The user message ends with "Evidence you can cite": the only valid evidence ids for this run. Cite ids exactly as listed wherever the plan asks for evidenceRefs or a ref. Never invent an id; an unknown id rejects the whole plan. @@ -78,7 +78,7 @@ Readiness rules (Dispatch rejects a "ready" plan that breaks any of these): - verdict.evidenceRefs cites at least one "repo:" id read at the pinned head SHA, and confidence is not low. - No material uncertainty remains. - For implementation work: implementationBrief is present, verifiedCurrentBehavior cites "repo:" evidence, at least one relevant path or file to create is named, every relevantPaths entry with change "modify" is a "repo:" id read at the pinned head SHA (a path only surfaced by search may have moved), inScope is not empty, and every acceptance criterion is deterministic: its verification is automated_test, command or code_inspection, never subjective. -- decomposition.required is false. When an issue bundles several independently shippable changes, set decomposition.required with one childBrief per change; that issue is not implementation-ready. +- decomposition.required is false. When an issue bundles several independently shippable changes, set decomposition.required with one childBrief per change; that issue is not implementation-ready. Every childBrief must be a complete bounded implementation brief (problem, verified current behavior, relevant paths, the settled design decision, in/out of scope, dependencies, acceptance criteria, tests) written for a worker with no other context; never emit child briefs while a material design question remains unresolved — that work goes to the escalation lane instead. Created children start as \`status/backlog\` and get their own grooming pass. - mutations.close is null. If any rule fails, the issue is not ready: pick the actionability that says why and record what is missing. diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index c2195f07..1502fa73 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -26,6 +26,7 @@ const { mocks } = vi.hoisted(() => ({ addIssueComment: vi.fn(), updateIssueTitleAndBody: vi.fn(), closeIssue: vi.fn(), + createIssue: vi.fn(), findActiveLeasesForIssue: vi.fn(), upsertLease: vi.fn(), releaseLease: vi.fn(), @@ -43,6 +44,7 @@ const { mocks } = vi.hoisted(() => ({ automationRepo: { findUnique: vi.fn() }, groomingRun: { create: vi.fn(), update: vi.fn(), findFirst: vi.fn(), findMany: vi.fn() }, groomingApplication: { findUnique: vi.fn(), create: vi.fn(), update: vi.fn(), updateMany: vi.fn() }, + groomingChildClaim: { findUnique: vi.fn(), create: vi.fn(), update: vi.fn() }, issue: { update: vi.fn(), findMany: vi.fn() }, issueLane: { create: vi.fn() }, agentRun: { create: vi.fn() }, @@ -73,6 +75,7 @@ vi.mock("@/lib/github", () => ({ addIssueComment: mocks.addIssueComment, updateIssueTitleAndBody: mocks.updateIssueTitleAndBody, closeIssue: mocks.closeIssue, + createIssue: mocks.createIssue, addIssueLabel: mocks.addIssueLabel, removeIssueLabel: mocks.removeIssueLabel, })); diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index d434772f..29d1b3dc 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -1,5 +1,5 @@ import { prisma } from "@/lib/prisma"; -import { addIssueComment, closeIssue, updateIssueLabels, updateIssueTitleAndBody } from "@/lib/github"; +import { addIssueComment, closeIssue, createIssue, updateIssueLabels, updateIssueTitleAndBody } from "@/lib/github"; import { findActiveLeasesForIssue, releaseLease, upsertLease } from "@/lib/lease"; import { acquireGroomerLock, heartbeatGroomerLock, HEARTBEAT_MS, releaseGroomerLock } from "./groomer-lock"; import { selectGroomingCandidate } from "./selector"; @@ -107,6 +107,8 @@ export interface GroomerDeps { addComment: typeof addIssueComment; updateTitleAndBody: typeof updateIssueTitleAndBody; closeIssue: typeof closeIssue; + /** Open a decomposition child issue (dispatch#1066). */ + createIssue: typeof createIssue; findActiveLeases: typeof findActiveLeasesForIssue; upsertLease: typeof upsertLease; releaseLease: typeof releaseLease; @@ -138,6 +140,7 @@ const defaultDeps: GroomerDeps = { addComment: addIssueComment, updateTitleAndBody: updateIssueTitleAndBody, closeIssue, + createIssue, findActiveLeases: findActiveLeasesForIssue, upsertLease, releaseLease, @@ -581,7 +584,9 @@ async function executeGroomerRun( } for (const [what, reasons] of Object.entries(diff.withheld)) { - contextWarnings.push(`apply: withheld ${what === "close" ? "the already_done close" : "the ready promotion"}: ${reasons!.join("; ")}`); + const withheldName = + what === "close" ? "the already_done close" : what === "ready" ? "the ready promotion" : "the decomposition"; + contextWarnings.push(`apply: withheld ${withheldName}: ${reasons!.join("; ")}`); } // Post-condition invariant (dispatch#941): exactly one status/* label. @@ -597,6 +602,7 @@ async function executeGroomerRun( notReadyReason: notReadyReason ?? null, willComment: diff.comment !== null, willCloseIssue: diff.close, + willCreateChildren: diff.children !== null, titleRewritten: diff.title !== null, originalTitle: diff.title !== null ? analyzed.title : undefined, proposedTitle: diff.title ?? undefined, @@ -624,6 +630,7 @@ async function executeGroomerRun( inFlightStatus: inFlight, willComment: false, willCloseIssue: false, + willCreateChildren: false, titleRewritten: false, bodyEnriched: false, }); @@ -834,6 +841,11 @@ async function executeGroomerRun( addComment: deps.addComment, updateTitleAndBody: deps.updateTitleAndBody, closeIssue: deps.closeIssue, + // The GitHub client returns `html_url`; the applier wants `url`. + createIssue: async (repoFullName, input) => { + const created = await deps.createIssue(repoFullName, input); + return { number: created.number, url: created.html_url }; + }, fetchRecentComments: (_repo, _number, max) => reader.fetchRecentComments(max), }; const applied = await applyGroomingMutations( @@ -841,6 +853,7 @@ async function executeGroomerRun( repoFullName: candidate.repoFullName, issueNumber: candidate.number, issueId: candidate.id, + parentUrl: candidate.url, groomingRunId: groomingRun.id, applicationKey, diff, @@ -1137,6 +1150,14 @@ function describeApplication(applied: ApplyResult, withheld: Record 0) out.childrenCreated = childrenStep.children.created; + if (childrenStep.children.reused.length > 0) out.childrenReused = childrenStep.children.reused; + } + if (childrenStep.status === "failed") out.childrenError = childrenStep.error; + } const comment = steps.comment; if (comment && comment.status !== "noop") { if (applied.commentUrl) out.commentUrl = applied.commentUrl; diff --git a/src/lib/issue-filters.ts b/src/lib/issue-filters.ts index 7020ba75..7743904e 100644 --- a/src/lib/issue-filters.ts +++ b/src/lib/issue-filters.ts @@ -1,4 +1,5 @@ import { AGENT_PREFIX, isAgentLabel, isOwnerLabel, OWNER_PREFIX, STATUS_LABELS } from "@/types"; +import { UMBRELLA_LABEL } from "@/lib/decomposition"; /** * Single source of truth for Renovate issue detection. Both the in-memory @@ -141,7 +142,7 @@ export function buildUmbrellaIssueExclusionWhere() { return { NOT: { OR: [ - { labels: { has: "umbrella" } }, + { labels: { has: UMBRELLA_LABEL } }, { title: { startsWith: "Weekly tech debt audit:", mode: "insensitive" } }, ], }, From ab55d5797a211778b46c16268b230ac4d5889bb2 Mon Sep 17 00:00:00 2001 From: Courier Date: Mon, 5 Oct 2026 23:09:02 +0000 Subject: [PATCH 2/8] test(groomer): run-level and eval coverage for decomposition child creation (#1066) Five run-level tests pin the acceptance criteria: one child per brief with the marker, backlink and backlog label plus the parent decomposition audit; exact-retry replay; a failed create converging on retry (reuse + create missing); withheld splits creating nothing. The eval corpus #1066 pending stub becomes a real check: children created once, zero on replay. --- .../evals/cases/broad-needs-decomposition.ts | 2 +- src/lib/groomer/evals/corpus.test.ts | 20 +++ src/lib/groomer/evals/harness.ts | 23 +++ src/lib/groomer/evals/types.ts | 6 + src/lib/groomer/run.test.ts | 156 ++++++++++++++++++ 5 files changed, 206 insertions(+), 1 deletion(-) diff --git a/src/lib/groomer/evals/cases/broad-needs-decomposition.ts b/src/lib/groomer/evals/cases/broad-needs-decomposition.ts index bebee43e..1ef65904 100644 --- a/src/lib/groomer/evals/cases/broad-needs-decomposition.ts +++ b/src/lib/groomer/evals/cases/broad-needs-decomposition.ts @@ -74,5 +74,5 @@ export const broadNeedsDecomposition: GroomingCase = { }, }, ], - pending: [{ name: "accepted child briefs are created once, idempotently", on: "#1066" }], + expectsChildCreation: true, }; diff --git a/src/lib/groomer/evals/corpus.test.ts b/src/lib/groomer/evals/corpus.test.ts index 48cfef09..d3c22d10 100644 --- a/src/lib/groomer/evals/corpus.test.ts +++ b/src/lib/groomer/evals/corpus.test.ts @@ -194,6 +194,26 @@ describe("grooming corpus", () => { }); } + if (c.expectsChildCreation) { + it("creates accepted child briefs once, idempotently (dispatch#1066)", async () => { + const candidate = c.candidates.find( + (x) => + x.expect.accepted === true && + (x.output as GroomingPlanDraft)?.decomposition?.required === true && + (((x.output as GroomingPlanDraft)?.decomposition?.childBriefs.length ?? 0) > 0), + ); + expect(candidate, `[${c.id}] no accepted decomposing candidate to check child creation`).toBeTruthy(); + const briefCount = (candidate!.output as GroomingPlanDraft).decomposition.childBriefs.length; + const applications = new Map(); + const childClaims = new Map(); + const first = await runCandidate(c, candidate!, { applications, childClaims, runId: "run-1" }); + const second = await runCandidate(c, candidate!, { applications, childClaims, runId: "run-2" }); + expect(first.writes.children, `[${c.id}] first run should create one child per brief`).toHaveLength(briefCount); + expect(second.writes.children, `[${c.id}] replay should create no new child`).toHaveLength(0); + expect(childClaims.size, `[${c.id}] one child claim per brief`).toBe(briefCount); + }); + } + for (const pending of c.pending ?? []) { it.todo(`${pending.name} [pending ${pending.on}]`); } diff --git a/src/lib/groomer/evals/harness.ts b/src/lib/groomer/evals/harness.ts index 55cd0af7..63725eb6 100644 --- a/src/lib/groomer/evals/harness.ts +++ b/src/lib/groomer/evals/harness.ts @@ -113,8 +113,12 @@ function openTrackedIssues(c: GroomingCase, args: unknown) { /** GroomingApplication rows (#1063); share one between runs to exercise replay. */ export type ApplicationRows = Map & { applicationKey: string }>; +/** GroomingChildClaim rows (#1066); share one between runs to exercise child-creation idempotency. */ +export type ChildClaimRows = Map & { childKey: string }>; + export interface RunCandidateOptions { applications?: ApplicationRows; + childClaims?: ChildClaimRows; /** Distinguishes GroomingRun ids when one case is run more than once. */ runId?: string; } @@ -168,6 +172,7 @@ export async function runCandidate( }; const applications: ApplicationRows = options.applications ?? new Map(); + const childClaims: ChildClaimRows = options.childClaims ?? new Map(); const runId = options.runId ?? `run-${c.id}`; const prisma = { automationRepo: { findUnique: async () => ({ id: "repo-1", fullName: c.repoFullName, enabled: true }) }, @@ -210,6 +215,24 @@ export async function runCandidate( return { count: 1 }; }, }, + // Child-issue creation claims (#1066), in memory for this run. + groomingChildClaim: { + findUnique: async ({ where }: { where: { childKey: string } }) => childClaims.get(where.childKey) ?? null, + create: async ({ data }: { data: Record }) => { + const key = String(data.childKey); + // The unique childKey, as Postgres enforces it. + if (childClaims.has(key)) throw Object.assign(new Error("Unique constraint failed on childKey"), { code: "P2002" }); + // A created row has no childNumber/childUrl until the creation is recorded. + const row = { ...structuredClone(data), childKey: key, childNumber: null, childUrl: null }; + childClaims.set(key, row); + return row; + }, + update: async ({ where, data }: { where: { childKey: string }; data: Record }) => { + const row = childClaims.get(where.childKey)!; + Object.assign(row, structuredClone(data)); + return row; + }, + }, }; const deps: GroomerDeps = { diff --git a/src/lib/groomer/evals/types.ts b/src/lib/groomer/evals/types.ts index f655fc7f..44cf84ae 100644 --- a/src/lib/groomer/evals/types.ts +++ b/src/lib/groomer/evals/types.ts @@ -175,4 +175,10 @@ export interface GroomingCase { candidates: CaseCandidate[]; freshness?: FreshnessProbe[]; pending?: PendingBehavior[]; + /** + * Set when an accepted candidate decomposes the issue into bounded children. + * The runner then asserts the child-creation idempotency (dispatch#1066): + * the first run creates one child per brief, an exact replay creates none. + */ + expectsChildCreation?: boolean; } diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index 1502fa73..efbdb11c 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -40,6 +40,7 @@ const { mocks } = vi.hoisted(() => ({ releaseGroomerLock: vi.fn(), compareCommits: vi.fn(), applications: new Map>(), + childClaims: new Map>(), prisma: { automationRepo: { findUnique: vi.fn() }, groomingRun: { create: vi.fn(), update: vi.fn(), findFirst: vi.fn(), findMany: vi.fn() }, @@ -357,6 +358,31 @@ describe("runHostedGroomer", () => { return row; }, ); + // In-memory GroomingChildClaim with the unique childKey claim (dispatch#1066). + // A created row has no childNumber/childUrl until the creation is recorded, + // matching the nullable columns the reuse check reads. + mocks.childClaims.clear(); + mocks.prisma.groomingChildClaim.findUnique.mockImplementation( + async ({ where }: { where: { childKey: string } }) => mocks.childClaims.get(where.childKey) ?? null, + ); + mocks.prisma.groomingChildClaim.create.mockImplementation(async ({ data }: { data: Record }) => { + if (mocks.childClaims.has(data.childKey)) throw Object.assign(new Error("Unique constraint"), { code: "P2002" }); + const row = { ...data, childNumber: null, childUrl: null }; + mocks.childClaims.set(data.childKey, row); + return row; + }); + mocks.prisma.groomingChildClaim.update.mockImplementation( + async ({ where, data }: { where: { childKey: string }; data: Record }) => { + const row = mocks.childClaims.get(where.childKey)!; + Object.assign(row, JSON.parse(JSON.stringify(data))); + return row; + }, + ); + // Each created child gets its own number and URL, as GitHub would. + mocks.createIssue.mockImplementation(async (repoFullName: string) => { + const number = mocks.createIssue.mock.calls.length + 1001; + return { number, html_url: `https://github.com/${repoFullName}/issues/${number}` }; + }); }); it("returns null when no grooming candidate available", async () => { @@ -2604,6 +2630,136 @@ Investigate session handling in auth module.`; }); }); + describe("decomposition child creation (dispatch#1066)", () => { + const childBrief = (n: number) => ({ + title: `Bounded child ${n}`, + problem: `Child ${n} problem, as its own bounded change.`, + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [`child ${n}`], + outOfScope: [], + dependencies: [], + acceptanceCriteria: [`Child ${n} works end to end`], + tests: [], + }); + + /** A non-ready (backlog) verdict that splits the issue into `count` bounded children. */ + const decomposingDraft = (count: number) => + notReadyDraft("backlog", { + decomposition: { + required: true, + reason: "the issue spans several independent areas", + childBriefs: Array.from({ length: count }, (_, i) => childBrief(i + 1)), + }, + }); + + it("creates one bounded child per brief, decorates the parent as an umbrella, and records the decomposition", async () => { + mocks.callGroomerLLM.mockResolvedValue(decomposingDraft(2)); + const result = await runHostedGroomer(); + + expect(result!.appliedMutations).toMatchObject({ outcome: "applied" }); + expect(mocks.createIssue).toHaveBeenCalledTimes(2); + // Each child lands as a backlog issue (not worker-ready). + expect(mocks.createIssue).toHaveBeenCalledWith( + "org/repo", + expect.objectContaining({ labels: ["status/backlog"] }), + ); + // The umbrella rides the single labels write; the children step makes none. + expect(mocks.updateIssueLabels).toHaveBeenCalledTimes(1); + expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, expect.arrayContaining(["umbrella"])); + // The parent's decomposition state is persisted with the child URLs as follow-ups. + expect(mocks.prisma.issue.update).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ decomposed: true, followUpUrls: expect.any(Array) }), + }), + ); + expect(mocks.prisma.auditLog.create).toHaveBeenCalledWith( + expect.objectContaining({ data: expect.objectContaining({ action: "issue_decomposed" }) }), + ); + // The run records the created child links, in brief order. + expect(result!.appliedMutations!.childrenCreated).toHaveLength(2); + }); + + it("an exact retry is a replay: it creates no new child and re-surfaces the created links", async () => { + mocks.callGroomerLLM.mockResolvedValue(decomposingDraft(2)); + + const first = await runHostedGroomer(); + const second = await runHostedGroomer(); + + expect(first!.appliedMutations).toMatchObject({ outcome: "applied" }); + expect(second!.appliedMutations).toMatchObject({ outcome: "replayed" }); + // Only the first attempt created the children; the replay re-surfaces them. + expect(mocks.createIssue).toHaveBeenCalledTimes(2); + expect(first!.appliedMutations!.childrenCreated).toHaveLength(2); + expect(second!.appliedMutations!.childrenCreated).toHaveLength(2); + // Same application key, so the replay is attributable to the first run. + expect(first!.mutationPlan!.applicationKey).toBe(second!.mutationPlan!.applicationKey); + }); + + it("a child-creation failure is a partial, retryable run; the retry reuses the landed child and creates only the missing ones", async () => { + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + mocks.callGroomerLLM.mockResolvedValue(decomposingDraft(3)); + let createCalls = 0; + mocks.createIssue.mockImplementation(async () => { + createCalls += 1; + if (createCalls === 2) throw new Error("GitHub API error creating issue: 502"); + const number = createCalls + 1000; + return { number, html_url: `https://github.com/org/repo/issues/${number}` }; + }); + + const first = await runHostedGroomer(); + expect(first!.appliedMutations).toMatchObject({ outcome: "partial" }); + expect(first!.appliedMutations!.childrenError).toMatch(/502/); + + // The retry: the failing creation now succeeds. + mocks.createIssue.mockImplementation(async () => { + createCalls += 1; + const number = createCalls + 1000; + return { number, html_url: `https://github.com/org/repo/issues/${number}` }; + }); + const second = await runHostedGroomer(); + errSpy.mockRestore(); + + expect(second!.appliedMutations).toMatchObject({ outcome: "applied" }); + // The child that landed before the failure is reused; only the missing ones are created. + expect(second!.appliedMutations!.childrenCreated).toHaveLength(2); + expect(second!.appliedMutations!.childrenReused).toHaveLength(1); + // One claim per distinct child, across both attempts (idempotent child identity). + expect(mocks.prisma.groomingChildClaim.create).toHaveBeenCalledTimes(3); + }); + + it("creates no children when the split is withheld for low confidence", async () => { + mocks.callGroomerLLM.mockResolvedValue( + notReadyDraft("backlog", { + verdict: { confidence: "low" }, + decomposition: { required: true, reason: "split it", childBriefs: [childBrief(1)] }, + }), + ); + const result = await runHostedGroomer(); + + expect(result!.appliedMutations).toMatchObject({ outcome: "applied" }); + expect(result!.appliedMutations!.withheld).toMatchObject({ decomposition: expect.any(Array) }); + expect(mocks.createIssue).not.toHaveBeenCalled(); + expect(mocks.prisma.groomingChildClaim.create).not.toHaveBeenCalled(); + }); + + it("creates no children when a material uncertainty remains", async () => { + mocks.callGroomerLLM.mockResolvedValue( + notReadyDraft("backlog", { + verdict: { uncertainties: [{ kind: "scope", question: "Which child owns the migration?", material: true }] }, + decomposition: { required: true, reason: "split it", childBriefs: [childBrief(1)] }, + }), + ); + const result = await runHostedGroomer(); + + expect(result!.appliedMutations).toMatchObject({ outcome: "applied" }); + expect(result!.appliedMutations!.withheld).toMatchObject({ decomposition: expect.any(Array) }); + expect(mocks.createIssue).not.toHaveBeenCalled(); + expect(mocks.prisma.groomingChildClaim.create).not.toHaveBeenCalled(); + }); + }); + it("strips a status the groomer does not own, so the derived status is the only one", async () => { mocks.selectGroomingCandidate.mockResolvedValue({ ...mockCandidate, labels: ["priority/p0", "status/needs-review"] }); mocks.callGroomerLLM.mockResolvedValue(notReadyDraft("backlog")); From 703dffc07ec627e89a6710b723b6e64d66f84637 Mon Sep 17 00:00:00 2001 From: Courier Date: Tue, 6 Oct 2026 00:21:12 +0000 Subject: [PATCH 3/8] fix(groomer): land the umbrella only after children exist; harden child claims (#1066) Review round: - the selector excludes umbrella-labeled issues on every path, including a targeted re-groom, so the umbrella must NOT ride the labels step: a decomposition that failed halfway would strand its parent forever. The children step now adds it additively only after every child exists or is reused, and ApplyResult.labels folds it in so the freshness baseline and audit record the real label set. - a fresh null child claim (younger than ACTIVE_CLAIM_MS) is held by another in-flight attempt: creating on top of it could duplicate, so the step fails and a later attempt converges. - a failed children write carries the partial created/reused links on the failed step, so the run record shows children that landed even when a later create threw. - child keys in the application-key intent are sorted: brief order is not identity. - accurate docs: umbrella ordering + createIssue consumers. --- docs/hosted-groomer.md | 4 +- src/lib/github-issues.ts | 5 +- src/lib/groomer/evals/harness.ts | 3 + src/lib/groomer/mutation-applier.test.ts | 228 +++++++++++++++++++---- src/lib/groomer/mutation-applier.ts | 162 ++++++++++------ src/lib/groomer/run.test.ts | 17 +- src/lib/groomer/run.ts | 6 +- 7 files changed, 331 insertions(+), 94 deletions(-) diff --git a/docs/hosted-groomer.md b/docs/hosted-groomer.md index 7b5b404b..7c3fed91 100644 --- a/docs/hosted-groomer.md +++ b/docs/hosted-groomer.md @@ -181,7 +181,7 @@ A plan may also split the issue into bounded children instead of (or alongside) - verdict confidence of at least `medium` (a low-confidence split is too speculative to fan out); - no material uncertainty remaining (the same `verdict.uncertainties[].material` flag the close policy keys on). -When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. The parent is then decorated as an `umbrella` (a separate label write) and recorded as decomposed — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically. +When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. The umbrella label is added by the children step **only after every child exists or is reused** (an additive `addLabel`, not part of the labels write), and the parent is then recorded as decomposed — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically. The ordering matters: the umbrella label removes the issue from groomer selection (the selector excludes `umbrella`-labeled issues on every path, including a targeted re-groom), so it must not land before a decomposition that could fail halfway. A partially applied decomposition therefore stays re-selectable and converges on retry. A decomposition that fails any rule is withheld: no child is created and the parent is not decorated, and `withheld.decomposition` records why. @@ -219,7 +219,7 @@ The diff is computed against the live issue, never Dispatch's cache, and only wh 1. **labels**: priority/type changes and the derived status. For an `already_done` plan the status stays as it was here. 2. **comment**: at most one, with `@` mentions neutralized and a hidden `` marker at its end. Any marker the model wrote into its own text is stripped, and a marker only counts on a comment by an automation author, so nobody else can forge one to suppress or impersonate a groomer comment. 3. **title/body**: one write. A title is rewritten only when the current one is bad (the existing guard). The body is never replaced: enrichment goes into one Dispatch-managed section between `` and `` markers, appended after the human text on first write and replaced in place afterwards. Text outside the section is kept byte for byte. Enrichment still applies only when the human-authored text is sparse, and a body whose markers are unpaired or repeated is left alone. -4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and the parent is decorated as an `umbrella` and recorded as decomposed with the child URLs as its follow-ups (see [Decomposition](#decomposition)). + 4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and only once every child exists or is reused is the parent decorated as an `umbrella` and recorded as decomposed with the child URLs as its follow-ups (see [Decomposition](#decomposition)). 5. **close**, only for an `already_done` plan that still satisfies the close policy. 6. **status/done**, only once the close has landed, so a failed close leaves the issue open in its previous (groomable) status rather than open with `status/done`, which the selector would skip forever. diff --git a/src/lib/github-issues.ts b/src/lib/github-issues.ts index 6ade37cf..f8e051c7 100644 --- a/src/lib/github-issues.ts +++ b/src/lib/github-issues.ts @@ -335,8 +335,9 @@ export async function closeIssue( } /** - * Open a new issue. Used by the CI-failure ingester; every other issue in - * Dispatch arrives from GitHub rather than being created by it. + * Open a new issue. Used by the CI-failure ingester and the hosted groomer's + * decomposition children (dispatch#1066); every other issue in Dispatch arrives + * from GitHub rather than being created by it. */ export async function createIssue( repoFullName: string, diff --git a/src/lib/groomer/evals/harness.ts b/src/lib/groomer/evals/harness.ts index 63725eb6..3029f245 100644 --- a/src/lib/groomer/evals/harness.ts +++ b/src/lib/groomer/evals/harness.ts @@ -271,6 +271,9 @@ export async function runCandidate( writes.children.push({ number, url }); return { number, html_url: url }; }, + // The children step's umbrella: a separate additive write, not part of the + // labels-step writes.labels. + addLabel: async () => {}, findActiveLeases: async () => [], upsertLease: async () => ({ created: true, lease: { id: "lease-1" } }), releaseLease: async () => ({ id: "lease-1" }), diff --git a/src/lib/groomer/mutation-applier.test.ts b/src/lib/groomer/mutation-applier.test.ts index fea3aeb4..3f6ddecc 100644 --- a/src/lib/groomer/mutation-applier.test.ts +++ b/src/lib/groomer/mutation-applier.test.ts @@ -6,6 +6,7 @@ import { collectPinnedReadContent } from "./close-grounding"; import { validateGroomingPlan, type GroomingPlan, type GroomingPlanDraft } from "./plan"; import type { LiveComment, LiveIssueState } from "./mutation-validator"; import { + ACTIVE_CLAIM_MS, MANAGED_BODY_END, MANAGED_BODY_START, applyGroomingMutations, @@ -129,42 +130,32 @@ function planFor(d: GroomingPlanDraft, snap = snapshot()): GroomingPlan { return result.plan!; } -/** A plan that splits the issue into two bounded children (dispatch#1066). */ -function decomposeDraft(): GroomingPlanDraft { +/** A plan that splits the issue into `count` bounded children (dispatch#1066). */ +function decomposeDraft(count = 2): GroomingPlanDraft { // A ready `implementation` plan may not decompose, so the split lands as a // non-ready (backlog) verdict. + const letter = (n: number) => String.fromCharCode(64 + n); return { ...draft({ actionability: "backlog", lane: { id: "backlog", confidence: "high", reason: "bounded" } }), implementationBrief: null, decomposition: { required: true, - reason: "splits into two bounded children", - childBriefs: [ - { - title: "Child issue A", - problem: "Child A problem", + reason: `splits into ${count} bounded children`, + childBriefs: Array.from({ length: count }, (_, i) => { + const l = letter(i + 1); + return { + title: `Child issue ${l}`, + problem: `Child ${l} problem`, designDecision: null, verifiedCurrentBehavior: null, relevantPaths: [], - inScope: ["A"], + inScope: [l], outOfScope: [], dependencies: [], - acceptanceCriteria: ["A works"], + acceptanceCriteria: [`Child ${l} works`], tests: [], - }, - { - title: "Child issue B", - problem: "Child B problem", - designDecision: null, - verifiedCurrentBehavior: null, - relevantPaths: [], - inScope: ["B"], - outOfScope: [], - dependencies: [], - acceptanceCriteria: ["B works"], - tests: [], - }, - ], + }; + }), }, }; } @@ -344,13 +335,16 @@ describe("computeApplicationKey", () => { function memoryStore(initial?: ApplicationRecord): ApplicationStore & { rows: Map; children: Map; + decompositionStates: Array<{ labels: readonly string[]; followUpUrls: string[] }>; } { const rows = new Map(); const children = new Map(); + const decompositionStates: Array<{ labels: readonly string[]; followUpUrls: string[] }> = []; if (initial) rows.set(initial.applicationKey, initial); return { rows, children, + decompositionStates, find: async (key) => rows.get(key) ?? null, claim: async (input) => { const existing = rows.get(input.applicationKey) ?? null; @@ -391,8 +385,10 @@ function memoryStore(initial?: ApplicationRecord): ApplicationStore & { row.childUrl = data.childUrl; }, // The shared helper's persistence is exercised by the route and - // integration tests; the in-memory fake records nothing. - setDecompositionState: async () => {}, + // integration tests; the in-memory fake records the call only. + setDecompositionState: async (input) => { + decompositionStates.push({ labels: input.issue.labels, followUpUrls: input.followUpUrls }); + }, }; } @@ -402,6 +398,9 @@ function fakeGitHub(overrides: Partial = {}) { updateLabels: vi.fn(async (_r, _n, labels: string[]) => { calls.push(`labels:${labels.filter((l) => l.startsWith("status/")).join(",")}`); }), + addLabel: vi.fn(async (_r, _n, label: string) => { + calls.push(`addLabel:${label}`); + }), addComment: vi.fn(async () => { calls.push("comment"); return { url: "https://github.com/org/repo/issues/42#issuecomment-1" }; @@ -683,21 +682,31 @@ describe("applyGroomingMutations", () => { }); describe("applyGroomingMutations → decomposition", () => { - it("creates each child issue, rides the umbrella on the labels step, and records the child links", async () => { - const { github } = fakeGitHub(); + it("creates each child issue, adds the umbrella via addLabel after all children land, and records the child links", async () => { + const { github, calls } = fakeGitHub(); const store = memoryStore(); const diff = diffFor(decomposeDraft()); expect(diff.children).not.toBeNull(); - // The umbrella decoration rides the labels step, not a second write. - expect(diff.labelsAfter).toContain("umbrella"); - expect(diff.labelsStep).toContain("umbrella"); + // The umbrella is NOT part of the labels step: it is an additive write the + // children step makes via addLabel, so it never rides labelsStep/labelsAfter. + expect(diff.labelsAfter).not.toContain("umbrella"); + expect(diff.labelsStep).not.toContain("umbrella"); const result = await applyGroomingMutations(applyInput(diff), github, store); expect(result.outcome).toBe("applied"); expect(github.createIssue).toHaveBeenCalledTimes(2); expect(store.children.size).toBe(2); - // The single labels write carries the umbrella; the children step makes none. + // The single labels write carries no umbrella; the children step makes it. expect(github.updateLabels).toHaveBeenCalledTimes(1); - expect(github.updateLabels).toHaveBeenCalledWith("org/repo", 42, expect.arrayContaining(["umbrella"])); + expect(github.updateLabels).toHaveBeenCalledWith("org/repo", 42, expect.not.arrayContaining(["umbrella"])); + // addLabel is called exactly once, with the umbrella, and only after every + // child was created or reused. + expect(github.addLabel).toHaveBeenCalledTimes(1); + expect(github.addLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); + const addLabelIdx = calls.indexOf("addLabel:umbrella"); + const childAIdx = calls.indexOf("child:Child issue A"); + const childBIdx = calls.indexOf("child:Child issue B"); + expect(addLabelIdx).toBeGreaterThan(childAIdx); + expect(addLabelIdx).toBeGreaterThan(childBIdx); // The children step carries the created child LINKS (key/number/url), not counts. const step = result.steps.children!; expect(step.status).toBe("applied"); @@ -708,6 +717,11 @@ describe("applyGroomingMutations → decomposition", () => { expect(link.number).toBeTypeOf("number"); expect(link.url).toBeTypeOf("string"); } + // The parent's decomposition state is recorded with the final label set + // (labelsAfter + umbrella) and the child URLs as the follow-ups. + expect(store.decompositionStates).toHaveLength(1); + expect(store.decompositionStates[0].labels).toContain("umbrella"); + expect(store.decompositionStates[0].followUpUrls).toHaveLength(2); // ApplyResult.labels (freshness baseline / audit) includes the umbrella. expect(result.labels).toContain("umbrella"); // ApplyResult.children carries the created links in brief order. @@ -756,8 +770,158 @@ describe("applyGroomingMutations → decomposition", () => { expect(diff.withheld.decomposition).toBeDefined(); const result = await applyGroomingMutations(applyInput(diff), github, store); expect(github.createIssue).not.toHaveBeenCalled(); + expect(github.addLabel).not.toHaveBeenCalled(); + expect(store.children.size).toBe(0); + expect(store.decompositionStates).toHaveLength(0); + expect(result.steps.children).toMatchObject({ status: "noop" }); + expect(result.labels).not.toContain("umbrella"); + }); + + it("adds no umbrella when the decomposition is withheld for material uncertainty", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + // A split with a material uncertainty is withheld; no children and no umbrella. + const base = decomposeDraft(); + const diff = diffFor({ + ...base, + verdict: { ...base.verdict, uncertainties: [{ kind: "scope", question: "Which child owns the migration?", material: true }] }, + }); + expect(diff.children).toBeNull(); + expect(diff.withheld.decomposition).toBeDefined(); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(github.createIssue).not.toHaveBeenCalled(); + expect(github.addLabel).not.toHaveBeenCalled(); expect(store.children.size).toBe(0); + expect(store.decompositionStates).toHaveLength(0); expect(result.steps.children).toMatchObject({ status: "noop" }); + expect(result.labels).not.toContain("umbrella"); + }); + + it("does not create a child whose null claim is held by a fresh in-flight attempt", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const briefs = diff.children!.briefs; + const firstKey = childBriefKey("org/repo", 42, briefs[0]); + // A claim with no recorded child, written moments ago: another attempt is + // in flight on it. Do not create a duplicate on top of it. + store.children.set(firstKey, { childKey: firstKey, childNumber: null, childUrl: null, updatedAt: new Date() }); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("partial"); // the labels step landed, children failed + expect(result.failure).toMatchObject({ step: "children", error: expect.stringContaining(`child claim ${firstKey} is held by another in-flight attempt`) }); + expect(result.steps.children).toMatchObject({ status: "failed" }); + // No create for the held child, and no umbrella on a failed decomposition. + expect(github.createIssue).not.toHaveBeenCalled(); + expect(github.addLabel).not.toHaveBeenCalled(); + expect(result.labels).not.toContain("umbrella"); + }); + + it("creates a child whose null claim has aged out (no in-flight attempt)", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const briefs = diff.children!.briefs; + const firstKey = childBriefKey("org/repo", 42, briefs[0]); + // A claim with no recorded child, written more than 2×ACTIVE_CLAIM_MS ago: + // the attempt that took it is long gone, so create proceeds. + store.children.set(firstKey, { + childKey: firstKey, + childNumber: null, + childUrl: null, + updatedAt: new Date(Date.now() - 2 * ACTIVE_CLAIM_MS), + }); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + expect(github.createIssue).toHaveBeenCalledTimes(2); + // The stale claim is overwritten with the newly created child. + expect(store.children.get(firstKey)).toMatchObject({ childNumber: 43, childUrl: "https://github.com/org/repo/issues/43" }); + expect(github.addLabel).toHaveBeenCalledTimes(1); + }); + + it("carries the partial children on a failed create, and converges on a retry of the same key", async () => { + const first = fakeGitHub({ + createIssue: vi.fn(async (_r: string, input: { title: string }) => { + if (input.title === "Child issue B") throw new Error("GitHub API error creating issue: 502"); + return { number: 43, url: "https://github.com/org/repo/issues/43" }; + }), + }); + const store = memoryStore(); + const diff = diffFor(decomposeDraft(3)); + const attempt = await applyGroomingMutations(applyInput(diff), first.github, store); + expect(attempt.outcome).toBe("partial"); // the labels step landed + const step = attempt.steps.children!; + expect(step.status).toBe("failed"); + expect(step.error).toContain("502"); + // The failed step record carries the child that landed before the failure. + expect(step.children?.created).toHaveLength(1); + expect(step.children?.created[0].number).toBe(43); + expect(attempt.failure).toMatchObject({ step: "children" }); + // A partial decomposition never lands the umbrella. + expect(first.github.addLabel).not.toHaveBeenCalled(); + + // A retry under the same application key: child A is reused, only the + // missing children are created, and the umbrella lands at that point. + const retry = fakeGitHub(); + const converged = await applyGroomingMutations(applyInput(diff), retry.github, store); + expect(converged.outcome).toBe("applied"); + expect(retry.github.createIssue).toHaveBeenCalledTimes(2); // B and C, not A + expect(retry.github.createIssue).not.toHaveBeenCalledWith("org/repo", expect.objectContaining({ title: "Child issue A" })); + const retriedStep = converged.steps.children!; + expect(retriedStep.children?.created).toHaveLength(2); + expect(retriedStep.children?.reused).toHaveLength(1); + expect(retry.github.addLabel).toHaveBeenCalledTimes(1); + expect(retry.github.addLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); + }); + + it("reuses every child under a different application key, creating no duplicates", async () => { + const first = fakeGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const applied = await applyGroomingMutations(applyInput(diff), first.github, store); + expect(applied.outcome).toBe("applied"); + expect(first.github.createIssue).toHaveBeenCalledTimes(2); + + // A different application (a re-plan under a new key) that plans the same + // children: the child claims from the first run are reused, no duplicates. + const otherKey = "b".repeat(64); + const second = fakeGitHub(); + const reused = await applyGroomingMutations(applyInput(diff, { applicationKey: otherKey, groomingRunId: "run-2" }), second.github, store); + expect(reused.outcome).toBe("applied"); + expect(second.github.createIssue).not.toHaveBeenCalled(); + expect(reused.steps.children?.children?.reused).toHaveLength(2); + expect(reused.steps.children?.children?.created).toHaveLength(0); + expect(second.github.addLabel).toHaveBeenCalledTimes(1); + }); + + it("computes an identical application key for the same children in a different brief order", () => { + const base = decomposeDraft(); + const keyA = computeApplicationKey({ + repoFullName: "org/repo", + issueNumber: 42, + plan: planFor(base), + diff: diffFor(base), + }); + // The same set of children, reordered: the application key is stable. + const reordered: GroomingPlanDraft = { ...base, decomposition: { ...base.decomposition, childBriefs: [...base.decomposition.childBriefs].reverse() } }; + const keyB = computeApplicationKey({ + repoFullName: "org/repo", + issueNumber: 42, + plan: planFor(reordered), + diff: diffFor(reordered), + }); + expect(keyB).toBe(keyA); + // A changed brief (a new title) is a different application. + const changed: GroomingPlanDraft = { + ...base, + decomposition: { ...base.decomposition, childBriefs: base.decomposition.childBriefs.map((b, i) => (i === 0 ? { ...b, title: "A renamed child" } : b)) }, + }; + const keyC = computeApplicationKey({ + repoFullName: "org/repo", + issueNumber: 42, + plan: planFor(changed), + diff: diffFor(changed), + }); + expect(keyC).not.toBe(keyA); }); }); diff --git a/src/lib/groomer/mutation-applier.ts b/src/lib/groomer/mutation-applier.ts index 153d4b1d..0856ee56 100644 --- a/src/lib/groomer/mutation-applier.ts +++ b/src/lib/groomer/mutation-applier.ts @@ -308,13 +308,15 @@ export function computeMutationDiff(input: MutationDiffInput): GroomingMutationD effective.mutations.status, ); - // A policy-passing decomposition decorates the parent as an umbrella. That - // label rides the labels step (not a second write in the children step) so - // the application key, ApplyResult.labels and the freshness baseline all - // include it — otherwise the next snapshot would read the groomer's own - // umbrella as an external change. - if (children !== null && !labelsAfter.includes(UMBRELLA_LABEL)) labelsAfter.push(UMBRELLA_LABEL); - + // The umbrella label is deliberately NOT part of labelsAfter (and so not of + // the labels step): it is added by the children step, and only after every + // child exists or is reused. The groomer selector excludes umbrella-labeled + // issues from selection on every path — including a targeted re-groom by + // issueNumber (which bypasses only the grooming-state exclusion, not the + // umbrella one) — so landing the umbrella first would make a decomposition + // that fails halfway un-reselectable, breaking "a partial create converges on + // retry". The label we write is still folded into ApplyResult.labels below so + // the freshness baseline and audit record it. // status/done lands only after the close succeeds, so a failed close // leaves the issue open with its previous status (still groomable), // never open with status/done (which the selector skips forever). An issue @@ -395,9 +397,10 @@ export function computeApplicationKey(input: { close: diff.close, // The decomposition is part of the intent: its children are the stable // childBriefKeys, so a change to the split (or its withholding) changes - // the application key. + // the application key. Sorted, because the brief order is not identity — + // the same set of children in a different order is the same application. children: diff.children - ? diff.children.briefs.map((brief) => childBriefKey(input.repoFullName, input.issueNumber, brief)) + ? diff.children.briefs.map((brief) => childBriefKey(input.repoFullName, input.issueNumber, brief)).sort() : null, }, }; @@ -406,6 +409,18 @@ export function computeApplicationKey(input: { // ─── Application ────────────────────────────────────────────────────────────── +/** + * A step write that failed partway, carrying the partial result that did + * complete (e.g. the children created before a later child's create threw) so + * the failed step record — and a retry — can see and reuse it. + */ +export class PartialStepError extends Error { + constructor(message: string, readonly partial: Partial) { + super(message); + this.name = "PartialStepError"; + } +} + export const APPLY_STEPS = ["labels", "comment", "content", "children", "close", "done_label"] as const; export type ApplyStep = (typeof APPLY_STEPS)[number]; @@ -484,6 +499,8 @@ export interface ChildClaimRecord { childKey: string; childNumber: number | null; childUrl: string | null; + /** When the claim row was last written; the prisma store returns it. */ + updatedAt?: Date | string | null; } export interface ApplicationStore { @@ -544,6 +561,8 @@ export interface ApplierGitHub { fetchRecentComments(repoFullName: string, issueNumber: number, max: number): Promise; /** Open a new issue (a decomposition child); returns its number and URL. */ createIssue(repoFullName: string, input: { title: string; body: string; labels?: string[] }): Promise<{ number: number; url: string }>; + /** Add a single label to the issue (the children step's umbrella). */ + addLabel(repoFullName: string, issueNumber: number, label: string): Promise; } export interface ApplyInput { @@ -705,7 +724,15 @@ export async function applyGroomingMutations( steps[step] = { status: "applied", ...(result ?? {}), at: now().toISOString() }; } catch (err) { const error = errorMessage(err); - steps[step] = { status: "failed", error, at: now().toISOString() }; + // A PartialStepError carries the partial result that completed (e.g. the + // children created before a later create threw), so the failed step + // record — and a retry — can see and reuse it. + steps[step] = { + status: "failed", + error, + ...(err instanceof PartialStepError ? err.partial : {}), + at: now().toISOString(), + }; failure = { step, error }; console.error(`[groomer] ${repoFullName}#${issueNumber}: ${step} failed; later steps not attempted:`, err); } @@ -788,11 +815,14 @@ export async function applyGroomingMutations( // 4. Children (dispatch#1066): one bounded child issue per brief. Creation // is idempotent through the GroomingChildClaim keyed by the childBriefKey - // (a retry reuses a created child and creates only the missing ones). The - // umbrella label rides the labels step above, so this step only creates - // the children and records the parent's decomposition state with their - // URLs as the follow-ups. It lands before the close, so a close is never - // applied on top of a decomposition that failed to land. + // (a retry reuses a created child and creates only the missing ones). Only + // after every child exists or is reused does this step add the umbrella + // label (an additive write) and record the parent's decomposition state + // with the child URLs as the follow-ups. The umbrella lands last so a + // decomposition that fails mid-create leaves the parent still + // re-selectable (the selector excludes umbrella issues) and converges on + // retry. It lands before the close, so a close is never applied on top of + // a decomposition that failed to land. const children = diff.children; let appliedChildren: ChildIssueLink[] = []; await run( @@ -804,51 +834,68 @@ export async function applyGroomingMutations( const created: ChildIssueLink[] = []; const reused: ChildIssueLink[] = []; const links: ChildIssueLink[] = []; - for (const brief of children.briefs) { - const childKey = childBriefKey(repoFullName, issueNumber, brief); - const { existing } = await store.claimChild({ - childKey, - parentIssueId: input.issueId, - repoFullName, - parentNumber: issueNumber, - title: brief.title, - }); - if (existing && existing.childNumber !== null && existing.childUrl !== null) { - // An earlier attempt of this application already created this child. - const link: ChildIssueLink = { key: childKey, number: existing.childNumber, url: existing.childUrl }; - reused.push(link); - links.push(link); - } else { - const body = renderChildIssueBody({ - brief, - parent: parentTarget, - decompositionReason: children.reason, + try { + for (const brief of children.briefs) { + const childKey = childBriefKey(repoFullName, issueNumber, brief); + const { existing } = await store.claimChild({ childKey, - }); - const issue = await github.createIssue(repoFullName, { + parentIssueId: input.issueId, + repoFullName, + parentNumber: issueNumber, title: brief.title, - body, - labels: [...CHILD_ISSUE_LABELS], }); - await store.saveChild(childKey, { childNumber: issue.number, childUrl: issue.url }); - const link: ChildIssueLink = { key: childKey, number: issue.number, url: issue.url }; - created.push(link); - links.push(link); + if (existing && existing.childNumber !== null && existing.childUrl !== null) { + // An earlier attempt already created this child. + const link: ChildIssueLink = { key: childKey, number: existing.childNumber, url: existing.childUrl }; + reused.push(link); + links.push(link); + } else { + // A claim with no recorded child and a fresh updatedAt is held by + // another in-flight attempt (a different application key claiming + // the same child); do not create a duplicate on top of it. + const heldAt = existing?.updatedAt ? new Date(existing.updatedAt).getTime() : NaN; + if (existing && existing.childNumber === null && Number.isFinite(heldAt) && now().getTime() - heldAt < ACTIVE_CLAIM_MS) { + throw new Error(`child claim ${childKey} is held by another in-flight attempt`); + } + const body = renderChildIssueBody({ + brief, + parent: parentTarget, + decompositionReason: children.reason, + childKey, + }); + const issue = await github.createIssue(repoFullName, { + title: brief.title, + body, + labels: [...CHILD_ISSUE_LABELS], + }); + await store.saveChild(childKey, { childNumber: issue.number, childUrl: issue.url }); + const link: ChildIssueLink = { key: childKey, number: issue.number, url: issue.url }; + created.push(link); + links.push(link); + } } + appliedChildren = links; + // Add the umbrella only now that every child exists or is reused; a + // failed create above throws before this line, so a partial + // decomposition never lands the umbrella. + await github.addLabel(repoFullName, issueNumber, UMBRELLA_LABEL); + // Record the decomposition state with the final label set (which + // includes the umbrella just written) and the child URLs. + await store.setDecompositionState({ + issue: { id: input.issueId, labels: [...new Set([...diff.labelsAfter, UMBRELLA_LABEL])] }, + repoFullName, + issueNumber, + actor: "hosted-groomer", + decomposed: true, + note: children.reason, + followUpUrls: links.map((child) => child.url), + }); + return { children: { created, reused }, detail: `${created.length} created, ${reused.length} reused` }; + } catch (err) { + // Carry what completed so far so the failed step record (and a retry) + // can see the children that did land. + throw new PartialStepError(errorMessage(err), { children: { created, reused } }); } - appliedChildren = links; - // The umbrella label is already on GitHub from the labels step; record - // the decomposition state with the final label set and the child URLs. - await store.setDecompositionState({ - issue: { id: input.issueId, labels: diff.labelsAfter }, - repoFullName, - issueNumber, - actor: "hosted-groomer", - decomposed: true, - note: children.reason, - followUpUrls: links.map((child) => child.url), - }); - return { children: { created, reused }, detail: `${created.length} created, ${reused.length} reused` }; }, "no decomposition in the plan", ); @@ -871,6 +918,11 @@ export async function applyGroomingMutations( if (landed(steps.labels)) labels = diff.labelsStep; if (landed(steps.done_label)) labels = diff.labelsAfter; + // The children step writes the umbrella additively (it is not part of + // diff.labelsAfter), so fold in the label we actually wrote. A union with the + // current set (not a reset to diff.labelsAfter) keeps whatever the labels / + // done-label step landed while adding the umbrella the children step wrote. + if (landed(steps.children)) labels = [...new Set([...labels, UMBRELLA_LABEL])]; const wrote = APPLY_STEPS.some((step) => steps[step]?.status === "applied" || steps[step]?.status === "replayed"); const outcome: ApplyOutcome = failure ? (wrote ? "partial" : "failed") : wrote ? "applied" : "noop"; diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index efbdb11c..bc999740 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -291,6 +291,7 @@ describe("runHostedGroomer", () => { mocks.getHostedGroomerConfig.mockReturnValue(mockConfig); mocks.callGroomerLLM.mockResolvedValue(mockOutput); mocks.updateIssueLabels.mockResolvedValue(undefined); + mocks.addIssueLabel.mockResolvedValue(undefined); mocks.updateIssueTitleAndBody.mockResolvedValue(undefined); mocks.addIssueComment.mockResolvedValue({ url: null }); mocks.closeIssue.mockResolvedValue(undefined); @@ -2665,9 +2666,11 @@ Investigate session handling in auth module.`; "org/repo", expect.objectContaining({ labels: ["status/backlog"] }), ); - // The umbrella rides the single labels write; the children step makes none. + // The umbrella is an additive write by the children step, not part of the labels write. expect(mocks.updateIssueLabels).toHaveBeenCalledTimes(1); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, expect.arrayContaining(["umbrella"])); + expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, expect.not.arrayContaining(["umbrella"])); + expect(mocks.addIssueLabel).toHaveBeenCalledTimes(1); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); // The parent's decomposition state is persisted with the child URLs as follow-ups. expect(mocks.prisma.issue.update).toHaveBeenCalledWith( expect.objectContaining({ @@ -2693,6 +2696,9 @@ Investigate session handling in auth module.`; expect(mocks.createIssue).toHaveBeenCalledTimes(2); expect(first!.appliedMutations!.childrenCreated).toHaveLength(2); expect(second!.appliedMutations!.childrenCreated).toHaveLength(2); + // The umbrella is added once, on the first attempt; the replay adds none. + expect(mocks.addIssueLabel).toHaveBeenCalledTimes(1); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); // Same application key, so the replay is attributable to the first run. expect(first!.mutationPlan!.applicationKey).toBe(second!.mutationPlan!.applicationKey); }); @@ -2711,6 +2717,8 @@ Investigate session handling in auth module.`; const first = await runHostedGroomer(); expect(first!.appliedMutations).toMatchObject({ outcome: "partial" }); expect(first!.appliedMutations!.childrenError).toMatch(/502/); + // A partial decomposition never lands the umbrella. + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); // The retry: the failing creation now succeeds. mocks.createIssue.mockImplementation(async () => { @@ -2725,6 +2733,9 @@ Investigate session handling in auth module.`; // The child that landed before the failure is reused; only the missing ones are created. expect(second!.appliedMutations!.childrenCreated).toHaveLength(2); expect(second!.appliedMutations!.childrenReused).toHaveLength(1); + // The umbrella lands once the decomposition fully converges. + expect(mocks.addIssueLabel).toHaveBeenCalledTimes(1); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); // One claim per distinct child, across both attempts (idempotent child identity). expect(mocks.prisma.groomingChildClaim.create).toHaveBeenCalledTimes(3); }); @@ -2741,6 +2752,7 @@ Investigate session handling in auth module.`; expect(result!.appliedMutations).toMatchObject({ outcome: "applied" }); expect(result!.appliedMutations!.withheld).toMatchObject({ decomposition: expect.any(Array) }); expect(mocks.createIssue).not.toHaveBeenCalled(); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); expect(mocks.prisma.groomingChildClaim.create).not.toHaveBeenCalled(); }); @@ -2756,6 +2768,7 @@ Investigate session handling in auth module.`; expect(result!.appliedMutations).toMatchObject({ outcome: "applied" }); expect(result!.appliedMutations!.withheld).toMatchObject({ decomposition: expect.any(Array) }); expect(mocks.createIssue).not.toHaveBeenCalled(); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); expect(mocks.prisma.groomingChildClaim.create).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/groomer/run.ts b/src/lib/groomer/run.ts index 29d1b3dc..26549b65 100644 --- a/src/lib/groomer/run.ts +++ b/src/lib/groomer/run.ts @@ -1,5 +1,5 @@ import { prisma } from "@/lib/prisma"; -import { addIssueComment, closeIssue, createIssue, updateIssueLabels, updateIssueTitleAndBody } from "@/lib/github"; +import { addIssueComment, addIssueLabel, closeIssue, createIssue, updateIssueLabels, updateIssueTitleAndBody } from "@/lib/github"; import { findActiveLeasesForIssue, releaseLease, upsertLease } from "@/lib/lease"; import { acquireGroomerLock, heartbeatGroomerLock, HEARTBEAT_MS, releaseGroomerLock } from "./groomer-lock"; import { selectGroomingCandidate } from "./selector"; @@ -109,6 +109,8 @@ export interface GroomerDeps { closeIssue: typeof closeIssue; /** Open a decomposition child issue (dispatch#1066). */ createIssue: typeof createIssue; + /** Add a single label (the children step's umbrella, dispatch#1066). */ + addLabel: typeof addIssueLabel; findActiveLeases: typeof findActiveLeasesForIssue; upsertLease: typeof upsertLease; releaseLease: typeof releaseLease; @@ -141,6 +143,7 @@ const defaultDeps: GroomerDeps = { updateTitleAndBody: updateIssueTitleAndBody, closeIssue, createIssue, + addLabel: addIssueLabel, findActiveLeases: findActiveLeasesForIssue, upsertLease, releaseLease, @@ -841,6 +844,7 @@ async function executeGroomerRun( addComment: deps.addComment, updateTitleAndBody: deps.updateTitleAndBody, closeIssue: deps.closeIssue, + addLabel: deps.addLabel, // The GitHub client returns `html_url`; the applier wants `url`. createIssue: async (repoFullName, input) => { const created = await deps.createIssue(repoFullName, input); From 0d139d5bc0241484f4516b6c92734e6ee7e227e2 Mon Sep 17 00:00:00 2001 From: Courier Date: Tue, 6 Oct 2026 00:54:22 +0000 Subject: [PATCH 4/8] fix(groomer): exempt same-key retries from the fresh child-claim guard; order the umbrella last (#1066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2: - GroomingChildClaim records the claiming applicationKey. A fresh null claim under a DIFFERENT key is another in-flight attempt (skip, retry later); under the SAME key it is my own abandoned create — the GroomingApplication resume CAS already excludes a concurrent same-key attempt — so the retry creates the missing child immediately. The in-memory fakes now stamp updatedAt on claim create like @updatedAt, so the convergence tests exercise the real store's semantics. - the children step now ends with the umbrella label: children first, the parent's decomposition state second, the umbrella last. Any failure before the umbrella keeps the parent re-selectable (every selector path excludes umbrella-labeled issues), so a partially applied decomposition always has an automatic retry path. --- docs/hosted-groomer.md | 4 +- .../migration.sql | 5 +- prisma/schema.prisma | 5 ++ src/lib/groomer/evals/harness.ts | 5 +- src/lib/groomer/mutation-applier.test.ts | 87 +++++++++++++++++-- src/lib/groomer/mutation-applier.ts | 71 +++++++++++---- src/lib/groomer/run.test.ts | 6 +- 7 files changed, 148 insertions(+), 35 deletions(-) diff --git a/docs/hosted-groomer.md b/docs/hosted-groomer.md index 7c3fed91..a7d671b5 100644 --- a/docs/hosted-groomer.md +++ b/docs/hosted-groomer.md @@ -181,7 +181,7 @@ A plan may also split the issue into bounded children instead of (or alongside) - verdict confidence of at least `medium` (a low-confidence split is too speculative to fan out); - no material uncertainty remaining (the same `verdict.uncertainties[].material` flag the close policy keys on). -When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. The umbrella label is added by the children step **only after every child exists or is reused** (an additive `addLabel`, not part of the labels write), and the parent is then recorded as decomposed — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically. The ordering matters: the umbrella label removes the issue from groomer selection (the selector excludes `umbrella`-labeled issues on every path, including a targeted re-groom), so it must not land before a decomposition that could fail halfway. A partially applied decomposition therefore stays re-selectable and converges on retry. +When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. Once every child exists or is reused, the children step records the parent's decomposition state — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically, and **only then** adds the umbrella label, as the step's final write (an additive `addLabel`, not part of the labels write). The ordering matters: the umbrella label removes the issue from groomer selection on every path (the selector excludes `umbrella`-labeled issues on every path, including a targeted re-groom), so it must be the step's last write; any earlier failure (a child create, the state write) keeps the parent re-selectable, so a partially applied decomposition converges on retry. A decomposition that fails any rule is withheld: no child is created and the parent is not decorated, and `withheld.decomposition` records why. @@ -219,7 +219,7 @@ The diff is computed against the live issue, never Dispatch's cache, and only wh 1. **labels**: priority/type changes and the derived status. For an `already_done` plan the status stays as it was here. 2. **comment**: at most one, with `@` mentions neutralized and a hidden `` marker at its end. Any marker the model wrote into its own text is stripped, and a marker only counts on a comment by an automation author, so nobody else can forge one to suppress or impersonate a groomer comment. 3. **title/body**: one write. A title is rewritten only when the current one is bad (the existing guard). The body is never replaced: enrichment goes into one Dispatch-managed section between `` and `` markers, appended after the human text on first write and replaced in place afterwards. Text outside the section is kept byte for byte. Enrichment still applies only when the human-authored text is sparse, and a body whose markers are unpaired or repeated is left alone. - 4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and only once every child exists or is reused is the parent decorated as an `umbrella` and recorded as decomposed with the child URLs as its follow-ups (see [Decomposition](#decomposition)). + 4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and once every child exists or is reused the parent is recorded as decomposed with the child URLs as its follow-ups, and only then decorated as an `umbrella` — the step's final write (see [Decomposition](#decomposition)). 5. **close**, only for an `already_done` plan that still satisfies the close policy. 6. **status/done**, only once the close has landed, so a failed close leaves the issue open in its previous (groomable) status rather than open with `status/done`, which the selector would skip forever. diff --git a/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql b/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql index 94f9b3b6..76642768 100644 --- a/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql +++ b/prisma/migrations/20261007000000_add_grooming_child_claim/migration.sql @@ -2,7 +2,9 @@ -- (#1066). The unique childKey (parent issue + normalized child brief) is -- the claim: creation is idempotent, so a retry after partial failure reuses -- children whose rows already carry their URLs and creates only the missing --- ones. +-- ones. applicationKey records the claiming application: same-key retries are +-- serialized by the GroomingApplication resume CAS, so a fresh null claim +-- under the retry's own key is an abandoned create, not a concurrent holder. CREATE TABLE "GroomingChildClaim" ( "id" TEXT NOT NULL, "childKey" TEXT NOT NULL, @@ -12,6 +14,7 @@ CREATE TABLE "GroomingChildClaim" ( "title" TEXT NOT NULL, "childNumber" INTEGER, "childUrl" TEXT, + "applicationKey" TEXT, "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, "updatedAt" TIMESTAMP(3) NOT NULL, diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 53c27560..272f7a96 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -619,6 +619,11 @@ model GroomingChildClaim { title String @db.Text childNumber Int? childUrl String? @db.Text + /// The application that claimed this child. Same-key retries are serialized + /// by the GroomingApplication resume CAS, so a null claim under my key is an + /// abandoned create, not a concurrent holder. No FK: the application row + /// may be deleted later. + applicationKey String? @db.Text createdAt DateTime @default(now()) updatedAt DateTime @updatedAt diff --git a/src/lib/groomer/evals/harness.ts b/src/lib/groomer/evals/harness.ts index 3029f245..ab3a2b39 100644 --- a/src/lib/groomer/evals/harness.ts +++ b/src/lib/groomer/evals/harness.ts @@ -222,8 +222,9 @@ export async function runCandidate( const key = String(data.childKey); // The unique childKey, as Postgres enforces it. if (childClaims.has(key)) throw Object.assign(new Error("Unique constraint failed on childKey"), { code: "P2002" }); - // A created row has no childNumber/childUrl until the creation is recorded. - const row = { ...structuredClone(data), childKey: key, childNumber: null, childUrl: null }; + // A created row has no childNumber/childUrl until the creation is + // recorded; updatedAt is stamped at create, mirroring @updatedAt. + const row = { ...structuredClone(data), childKey: key, childNumber: null, childUrl: null, updatedAt: new Date() }; childClaims.set(key, row); return row; }, diff --git a/src/lib/groomer/mutation-applier.test.ts b/src/lib/groomer/mutation-applier.test.ts index 3f6ddecc..8ba7dbcf 100644 --- a/src/lib/groomer/mutation-applier.test.ts +++ b/src/lib/groomer/mutation-applier.test.ts @@ -375,7 +375,15 @@ function memoryStore(initial?: ApplicationRecord): ApplicationStore & { claimChild: async (input) => { const existing = children.get(input.childKey) ?? null; if (!existing) { - children.set(input.childKey, { childKey: input.childKey, childNumber: null, childUrl: null }); + // Mirrors @updatedAt: the row is stamped at create, so a claim left by + // a crashed attempt is "fresh" until it ages out. + children.set(input.childKey, { + childKey: input.childKey, + childNumber: null, + childUrl: null, + applicationKey: input.applicationKey, + updatedAt: new Date(), + }); } return { existing: existing ? { ...existing } : null }; }, @@ -728,6 +736,25 @@ describe("applyGroomingMutations → decomposition", () => { expect(result.children).toHaveLength(2); }); + it("records the decomposition state before adding the umbrella label (step ordering)", async () => { + const { github, calls } = fakeGitHub(); + const store = memoryStore(); + const recordState = store.setDecompositionState; + store.setDecompositionState = async (input) => { + calls.push("decompositionState"); + return recordState(input); + }; + const diff = diffFor(decomposeDraft()); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + // The umbrella, which removes the parent from every selection path, must + // be the children step's final write: the state lands before it. + const stateIdx = calls.indexOf("decompositionState"); + const addLabelIdx = calls.indexOf("addLabel:umbrella"); + expect(stateIdx).toBeGreaterThan(-1); + expect(addLabelIdx).toBeGreaterThan(stateIdx); + }); + it("reuses a child an earlier attempt already created, creating only the missing one", async () => { const { github } = fakeGitHub(); const store = memoryStore(); @@ -735,7 +762,12 @@ describe("applyGroomingMutations → decomposition", () => { const briefs = diff.children!.briefs; // Simulate the first child having been created by an earlier attempt. const firstKey = childBriefKey("org/repo", 42, briefs[0]); - store.children.set(firstKey, { childKey: firstKey, childNumber: 99, childUrl: "https://github.com/org/repo/issues/99" }); + store.children.set(firstKey, { + childKey: firstKey, + childNumber: 99, + childUrl: "https://github.com/org/repo/issues/99", + applicationKey: KEY, + }); const result = await applyGroomingMutations(applyInput(diff), github, store); expect(github.createIssue).toHaveBeenCalledTimes(1); const step = result.steps.children!; @@ -803,9 +835,16 @@ describe("applyGroomingMutations → decomposition", () => { const diff = diffFor(decomposeDraft()); const briefs = diff.children!.briefs; const firstKey = childBriefKey("org/repo", 42, briefs[0]); - // A claim with no recorded child, written moments ago: another attempt is - // in flight on it. Do not create a duplicate on top of it. - store.children.set(firstKey, { childKey: firstKey, childNumber: null, childUrl: null, updatedAt: new Date() }); + // A claim with no recorded child under a DIFFERENT application key, + // written moments ago: another attempt is in flight on it. Do not create a + // duplicate on top of it. + store.children.set(firstKey, { + childKey: firstKey, + childNumber: null, + childUrl: null, + applicationKey: "b".repeat(64), + updatedAt: new Date(), + }); const result = await applyGroomingMutations(applyInput(diff), github, store); expect(result.outcome).toBe("partial"); // the labels step landed, children failed expect(result.failure).toMatchObject({ step: "children", error: expect.stringContaining(`child claim ${firstKey} is held by another in-flight attempt`) }); @@ -816,18 +855,45 @@ describe("applyGroomingMutations → decomposition", () => { expect(result.labels).not.toContain("umbrella"); }); + it("creates a child whose fresh null claim is its own (same application key)", async () => { + const { github } = fakeGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const briefs = diff.children!.briefs; + const firstKey = childBriefKey("org/repo", 42, briefs[0]); + // A claim with no recorded child under THIS application's own key, written + // moments ago: the GroomingApplication resume CAS already excludes a + // concurrent same-key attempt, so this is my own abandoned create from a + // crashed attempt — create on top of it (the immediate same-key retry). + store.children.set(firstKey, { + childKey: firstKey, + childNumber: null, + childUrl: null, + applicationKey: KEY, + updatedAt: new Date(), + }); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + expect(github.createIssue).toHaveBeenCalledTimes(2); + // The abandoned claim is overwritten with the newly created child. + expect(store.children.get(firstKey)).toMatchObject({ childNumber: 43, childUrl: "https://github.com/org/repo/issues/43" }); + expect(github.addLabel).toHaveBeenCalledTimes(1); + }); + it("creates a child whose null claim has aged out (no in-flight attempt)", async () => { const { github } = fakeGitHub(); const store = memoryStore(); const diff = diffFor(decomposeDraft()); const briefs = diff.children!.briefs; const firstKey = childBriefKey("org/repo", 42, briefs[0]); - // A claim with no recorded child, written more than 2×ACTIVE_CLAIM_MS ago: - // the attempt that took it is long gone, so create proceeds. + // A claim with no recorded child and no applicationKey (a row shape no + // deployed env has), written more than 2×ACTIVE_CLAIM_MS ago: the attempt + // that took it is long gone, so create proceeds. store.children.set(firstKey, { childKey: firstKey, childNumber: null, childUrl: null, + applicationKey: null, updatedAt: new Date(Date.now() - 2 * ACTIVE_CLAIM_MS), }); const result = await applyGroomingMutations(applyInput(diff), github, store); @@ -859,8 +925,11 @@ describe("applyGroomingMutations → decomposition", () => { // A partial decomposition never lands the umbrella. expect(first.github.addLabel).not.toHaveBeenCalled(); - // A retry under the same application key: child A is reused, only the - // missing children are created, and the umbrella lands at that point. + // A retry under the same application key (no hand-seeded rows): child A + // is reused; child B's fresh null claim carries THIS key, which the + // GroomingApplication resume CAS proves is not a concurrent holder, so the + // held-claim guard does not fire and only the missing children are created, + // and the umbrella lands at that point. const retry = fakeGitHub(); const converged = await applyGroomingMutations(applyInput(diff), retry.github, store); expect(converged.outcome).toBe("applied"); diff --git a/src/lib/groomer/mutation-applier.ts b/src/lib/groomer/mutation-applier.ts index 0856ee56..d5fd3fb9 100644 --- a/src/lib/groomer/mutation-applier.ts +++ b/src/lib/groomer/mutation-applier.ts @@ -499,6 +499,14 @@ export interface ChildClaimRecord { childKey: string; childNumber: number | null; childUrl: string | null; + /** + * The application that claimed this child (dispatch#1066); null for rows + * written before the column existed (none in any deployed env). A same-key + * retry is provably exclusive — the GroomingApplication claim/resume CAS + * lets exactly one attempt per application key proceed — so a null claim + * under MY key is my own abandoned create, not a concurrent holder. + */ + applicationKey: string | null; /** When the claim row was last written; the prisma store returns it. */ updatedAt?: Date | string | null; } @@ -526,7 +534,9 @@ export interface ApplicationStore { /** * Claim a child key, atomically (dispatch#1066). `existing` is the prior * claim when the child was already created, so a retry reuses it rather than - * creating a second child. + * creating a second child. `applicationKey` is recorded on a new claim so a + * same-key retry can tell its own abandoned create apart from a fresh + * claim held by a DIFFERENT application. */ claimChild(input: { childKey: string; @@ -534,6 +544,7 @@ export interface ApplicationStore { repoFullName: string; parentNumber: number; title: string; + applicationKey: string; }): Promise<{ existing: ChildClaimRecord | null }>; /** Record the created child's number and URL on its claim. */ saveChild(childKey: string, data: { childNumber: number; childUrl: string }): Promise; @@ -816,13 +827,14 @@ export async function applyGroomingMutations( // 4. Children (dispatch#1066): one bounded child issue per brief. Creation // is idempotent through the GroomingChildClaim keyed by the childBriefKey // (a retry reuses a created child and creates only the missing ones). Only - // after every child exists or is reused does this step add the umbrella - // label (an additive write) and record the parent's decomposition state - // with the child URLs as the follow-ups. The umbrella lands last so a - // decomposition that fails mid-create leaves the parent still - // re-selectable (the selector excludes umbrella issues) and converges on - // retry. It lands before the close, so a close is never applied on top of - // a decomposition that failed to land. + // after every child exists or is reused does this step record the parent's + // decomposition state with the child URLs as the follow-ups, and only then + // add the umbrella label (an additive write) last. The umbrella lands last + // — after the state write — so a decomposition that fails mid-create, or + // whose state write fails, leaves the parent still re-selectable (the + // selector excludes umbrella issues) and converges on retry. It lands + // before the close, so a close is never applied on top of a decomposition + // that failed to land. const children = diff.children; let appliedChildren: ChildIssueLink[] = []; await run( @@ -843,6 +855,7 @@ export async function applyGroomingMutations( repoFullName, parentNumber: issueNumber, title: brief.title, + applicationKey: input.applicationKey, }); if (existing && existing.childNumber !== null && existing.childUrl !== null) { // An earlier attempt already created this child. @@ -850,11 +863,23 @@ export async function applyGroomingMutations( reused.push(link); links.push(link); } else { - // A claim with no recorded child and a fresh updatedAt is held by - // another in-flight attempt (a different application key claiming - // the same child); do not create a duplicate on top of it. + // A null claim under a DIFFERENT application key with a fresh + // updatedAt is held by another in-flight attempt claiming the same + // child; do not create a duplicate on top of it. A null claim under + // THIS key is my own abandoned create: the GroomingApplication + // resume CAS already excludes a concurrent same-key attempt, so + // creating on top of it is how an immediate same-key retry + // converges. A row with no applicationKey (none exists in any + // deployed env; the migration ships with the feature) is not mine, + // so a fresh one is a foreign holder. const heldAt = existing?.updatedAt ? new Date(existing.updatedAt).getTime() : NaN; - if (existing && existing.childNumber === null && Number.isFinite(heldAt) && now().getTime() - heldAt < ACTIVE_CLAIM_MS) { + if ( + existing && + existing.childNumber === null && + existing.applicationKey !== input.applicationKey && + Number.isFinite(heldAt) && + now().getTime() - heldAt < ACTIVE_CLAIM_MS + ) { throw new Error(`child claim ${childKey} is held by another in-flight attempt`); } const body = renderChildIssueBody({ @@ -875,12 +900,11 @@ export async function applyGroomingMutations( } } appliedChildren = links; - // Add the umbrella only now that every child exists or is reused; a - // failed create above throws before this line, so a partial - // decomposition never lands the umbrella. - await github.addLabel(repoFullName, issueNumber, UMBRELLA_LABEL); - // Record the decomposition state with the final label set (which - // includes the umbrella just written) and the child URLs. + // Record the decomposition state with the final label set (labelsAfter + // plus the umbrella this step lands next) and the child URLs, BEFORE + // the umbrella: a failure here — like a failed child create above — + // must leave the parent still re-selectable, so the umbrella, which + // removes it from every selection path, is the step's final write. await store.setDecompositionState({ issue: { id: input.issueId, labels: [...new Set([...diff.labelsAfter, UMBRELLA_LABEL])] }, repoFullName, @@ -890,6 +914,11 @@ export async function applyGroomingMutations( note: children.reason, followUpUrls: links.map((child) => child.url), }); + // The umbrella lands last, now that every child exists or is reused and + // the decomposition state is recorded; a failed create or a failed + // state write throws before this line, so a partial decomposition never + // lands the umbrella. + await github.addLabel(repoFullName, issueNumber, UMBRELLA_LABEL); return { children: { created, reused }, detail: `${created.length} created, ${reused.length} reused` }; } catch (err) { // Carry what completed so far so the failed step record (and a retry) @@ -1028,7 +1057,10 @@ export function makePrismaApplicationStore(client: ApplicationStoreClient): Appl async claimChild(input) { // The unique childKey makes the claim atomic: a concurrent creator for // the same child loses with P2002 and reads the winner's row, so two - // attempts never create the same child twice. + // attempts never create the same child twice. The create records + // applicationKey (and findUnique selects it back), so a same-key retry + // can tell its own abandoned create apart from a foreign in-flight + // holder. const child = client.groomingChildClaim; const existing = await child.findUnique({ where: { childKey: input.childKey } }); if (existing) return { existing }; @@ -1040,6 +1072,7 @@ export function makePrismaApplicationStore(client: ApplicationStoreClient): Appl repoFullName: input.repoFullName, parentNumber: input.parentNumber, title: input.title, + applicationKey: input.applicationKey, }, }); return { existing: null }; diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index bc999740..a80e5ecb 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -361,14 +361,16 @@ describe("runHostedGroomer", () => { ); // In-memory GroomingChildClaim with the unique childKey claim (dispatch#1066). // A created row has no childNumber/childUrl until the creation is recorded, - // matching the nullable columns the reuse check reads. + // matching the nullable columns the reuse check reads; updatedAt is stamped + // at create, mirroring @updatedAt, and applicationKey is carried from the + // claim input. mocks.childClaims.clear(); mocks.prisma.groomingChildClaim.findUnique.mockImplementation( async ({ where }: { where: { childKey: string } }) => mocks.childClaims.get(where.childKey) ?? null, ); mocks.prisma.groomingChildClaim.create.mockImplementation(async ({ data }: { data: Record }) => { if (mocks.childClaims.has(data.childKey)) throw Object.assign(new Error("Unique constraint"), { code: "P2002" }); - const row = { ...data, childNumber: null, childUrl: null }; + const row = { ...data, childNumber: null, childUrl: null, updatedAt: new Date() }; mocks.childClaims.set(data.childKey, row); return row; }); From beb6a9b02e2f639748f132f47371e02e8084503a Mon Sep 17 00:00:00 2001 From: Courier Date: Tue, 6 Oct 2026 01:12:44 +0000 Subject: [PATCH 5/8] fix(groomer): the decomposition audit records labels at state-write time (#1186 review) The children step's setDecompositionState entry no longer lists the umbrella label: the umbrella is the step's final write, so claiming it in the audit before addLabel lands overstates the parent's labels if that add fails. The umbrella's own success or failure stays visible on the children step record and the run's groom audit. --- src/lib/groomer/mutation-applier.test.ts | 8 +++++--- src/lib/groomer/mutation-applier.ts | 17 +++++++++++------ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/lib/groomer/mutation-applier.test.ts b/src/lib/groomer/mutation-applier.test.ts index 8ba7dbcf..96b146cf 100644 --- a/src/lib/groomer/mutation-applier.test.ts +++ b/src/lib/groomer/mutation-applier.test.ts @@ -725,10 +725,12 @@ describe("applyGroomingMutations → decomposition", () => { expect(link.number).toBeTypeOf("number"); expect(link.url).toBeTypeOf("string"); } - // The parent's decomposition state is recorded with the final label set - // (labelsAfter + umbrella) and the child URLs as the follow-ups. + // The parent's decomposition state is recorded with the label set at + // state-write time (labelsAfter — the umbrella add lands afterwards, so it + // is not claimed yet) and the child URLs as the follow-ups. expect(store.decompositionStates).toHaveLength(1); - expect(store.decompositionStates[0].labels).toContain("umbrella"); + expect(store.decompositionStates[0].labels).toEqual(diff.labelsAfter); + expect(store.decompositionStates[0].labels).not.toContain("umbrella"); expect(store.decompositionStates[0].followUpUrls).toHaveLength(2); // ApplyResult.labels (freshness baseline / audit) includes the umbrella. expect(result.labels).toContain("umbrella"); diff --git a/src/lib/groomer/mutation-applier.ts b/src/lib/groomer/mutation-applier.ts index d5fd3fb9..a6435f76 100644 --- a/src/lib/groomer/mutation-applier.ts +++ b/src/lib/groomer/mutation-applier.ts @@ -900,13 +900,18 @@ export async function applyGroomingMutations( } } appliedChildren = links; - // Record the decomposition state with the final label set (labelsAfter - // plus the umbrella this step lands next) and the child URLs, BEFORE - // the umbrella: a failure here — like a failed child create above — - // must leave the parent still re-selectable, so the umbrella, which - // removes it from every selection path, is the step's final write. + // Record the decomposition state with the parent's label set at the + // moment of the state write (labelsAfter — the umbrella genuinely is + // not on the issue yet) and the child URLs, BEFORE the umbrella add: + // a failure here — like a failed child create above — must leave the + // parent still re-selectable, so the umbrella, which removes it from + // every selection path, is the step's final write. The audit entry + // records labels at state-write time; the umbrella add lands + // afterwards and its own success/failure is visible on the children + // step record and the run's groom audit, so the entry never claims a + // label that has not landed. await store.setDecompositionState({ - issue: { id: input.issueId, labels: [...new Set([...diff.labelsAfter, UMBRELLA_LABEL])] }, + issue: { id: input.issueId, labels: diff.labelsAfter }, repoFullName, issueNumber, actor: "hosted-groomer", From 9d82bbf2bbf95a330b07bd6b88e5e42a4bda9a3d Mon Sep 17 00:00:00 2001 From: Courier Date: Tue, 6 Oct 2026 18:01:49 +0000 Subject: [PATCH 6/8] fix(groomer): surface replayed child links on resume; test the child-claim race; soften claim comments (#1186 review) --- docs/hosted-groomer.md | 6 +-- prisma/schema.prisma | 9 ++-- src/lib/groomer/mutation-applier.test.ts | 61 ++++++++++++++++++++++++ src/lib/groomer/mutation-applier.ts | 24 ++++++---- 4 files changed, 85 insertions(+), 15 deletions(-) diff --git a/docs/hosted-groomer.md b/docs/hosted-groomer.md index a7d671b5..1ef93b58 100644 --- a/docs/hosted-groomer.md +++ b/docs/hosted-groomer.md @@ -181,13 +181,13 @@ A plan may also split the issue into bounded children instead of (or alongside) - verdict confidence of at least `medium` (a low-confidence split is too speculative to fan out); - no material uncertainty remaining (the same `verdict.uncertainties[].material` flag the close policy keys on). -When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. Once every child exists or is reused, the children step records the parent's decomposition state — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically, and **only then** adds the umbrella label, as the step's final write (an additive `addLabel`, not part of the labels write). The ordering matters: the umbrella label removes the issue from groomer selection on every path (the selector excludes `umbrella`-labeled issues on every path, including a targeted re-groom), so it must be the step's last write; any earlier failure (a child create, the state write) keeps the parent re-selectable, so a partially applied decomposition converges on retry. +When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. Once every child exists or is reused, the children step records the parent's decomposition state — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically, and **only then** adds the umbrella label, as the step's final write (an additive `addLabel`, not part of the labels write). The ordering matters: the umbrella label removes the issue from groomer selection on every path (the selector excludes `umbrella`-labeled issues on every path, including a targeted re-groom), so it must be the step's last write; any earlier failure (a child create, the state write) keeps the parent re-selectable, so a partially applied decomposition converges on retry. Two convergence behaviors of the child-claim hold are accepted: a fresh null claim under a different application key is held as another in-flight attempt's claim, so if an LLM re-plan produces a new key shortly after a failed attempt, that child's convergence waits out the active-claim window (about 10 minutes) rather than failing forever; and if the final umbrella label add fails after the decomposition state was recorded, a later retry replays the created children but re-records the decomposition state, which appends another `issue_decomposed` audit entry — accepted as audit noise on an already-rare failure path. A decomposition that fails any rule is withheld: no child is created and the parent is not decorated, and `withheld.decomposition` records why. ### Compatibility -`GroomingRun.validatedOutput` stores the full plan. `mutationPlan` keeps its existing fields and adds `planSchemaVersion`, `evidenceDigest`, `readiness`, `closeRecommendation`, `applicationKey`, `preconditions` and, when a policy withheld something, `withheld`. The run path applies mutations through `toGroomerOutput`, the legacy view that `POST /api/groomer/run` still returns as `output` (with the plan alongside as `plan`). A rejected plan's raw output and `validationErrors` are kept on the run. Runs recorded before the plan contract still render on `/automation/groomer`, marked `legacy`, with no readiness claim. +`GroomingRun.validatedOutput` stores the full plan. `mutationPlan` keeps its existing fields and adds `planSchemaVersion`, `evidenceDigest`, `readiness`, `closeRecommendation`, `applicationKey`, `preconditions` and `willCreateChildren` (true when the plan's decomposition children will be created; forced false for in-flight plans, including their dry-run previews, which apply nothing) and, when a policy withheld something, `withheld`. The run path applies mutations through `toGroomerOutput`, the legacy view that `POST /api/groomer/run` still returns as `output` (with the plan alongside as `plan`). A rejected plan's raw output and `validationErrors` are kept on the run. Runs recorded before the plan contract still render on `/automation/groomer`, marked `legacy`, with no readiness claim. ## Applying a plan @@ -219,7 +219,7 @@ The diff is computed against the live issue, never Dispatch's cache, and only wh 1. **labels**: priority/type changes and the derived status. For an `already_done` plan the status stays as it was here. 2. **comment**: at most one, with `@` mentions neutralized and a hidden `` marker at its end. Any marker the model wrote into its own text is stripped, and a marker only counts on a comment by an automation author, so nobody else can forge one to suppress or impersonate a groomer comment. 3. **title/body**: one write. A title is rewritten only when the current one is bad (the existing guard). The body is never replaced: enrichment goes into one Dispatch-managed section between `` and `` markers, appended after the human text on first write and replaced in place afterwards. Text outside the section is kept byte for byte. Enrichment still applies only when the human-authored text is sparse, and a body whose markers are unpaired or repeated is left alone. - 4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and once every child exists or is reused the parent is recorded as decomposed with the child URLs as its follow-ups, and only then decorated as an `umbrella` — the step's final write (see [Decomposition](#decomposition)). +4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and once every child exists or is reused the parent is recorded as decomposed with the child URLs as its follow-ups, and only then decorated as an `umbrella` — the step's final write (see [Decomposition](#decomposition)). 5. **close**, only for an `already_done` plan that still satisfies the close policy. 6. **status/done**, only once the close has landed, so a failed close leaves the issue open in its previous (groomable) status rather than open with `status/done`, which the selector would skip forever. diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 272f7a96..b1ee22db 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -619,10 +619,11 @@ model GroomingChildClaim { title String @db.Text childNumber Int? childUrl String? @db.Text - /// The application that claimed this child. Same-key retries are serialized - /// by the GroomingApplication resume CAS, so a null claim under my key is an - /// abandoned create, not a concurrent holder. No FK: the application row - /// may be deleted later. + /// The application that claimed this child. Same-key attempts are not + /// expected to run concurrently (the GroomingApplication resume CAS + /// serializes fresh claims, and the groomer is confined to a single + /// replica), so a null claim under my key is my own abandoned create. + /// No FK: the application row may be deleted later. applicationKey String? @db.Text createdAt DateTime @default(now()) updatedAt DateTime @updatedAt diff --git a/src/lib/groomer/mutation-applier.test.ts b/src/lib/groomer/mutation-applier.test.ts index 96b146cf..e55261f6 100644 --- a/src/lib/groomer/mutation-applier.test.ts +++ b/src/lib/groomer/mutation-applier.test.ts @@ -944,6 +944,28 @@ describe("applyGroomingMutations → decomposition", () => { expect(retry.github.addLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); }); + it("surfaces the child links again when a resumed partial application replays its landed children step", async () => { + const created = [{ key: "k-a", number: 43, url: "https://github.com/org/repo/issues/43" }]; + const reused = [{ key: "k-b", number: 44, url: "https://github.com/org/repo/issues/44" }]; + // The children step landed in the prior attempt (with its links recorded) + // but the application never finished: the resume replays the step instead + // of re-creating the children, and the result still carries the links. + const store = memoryStore({ + applicationKey: KEY, + groomingRunId: "run-0", + status: "partial", + steps: { children: { status: "applied", children: { created, reused } } }, + attempts: 1, + updatedAt: new Date(Date.now() - 11 * 60 * 1000), + }); + const { github } = fakeGitHub(); + const result = await applyGroomingMutations(applyInput(diffFor(decomposeDraft())), github, store); + expect(result.outcome).not.toBe("busy"); + expect(result.steps.children).toMatchObject({ status: "replayed" }); + expect(github.createIssue).not.toHaveBeenCalled(); + expect(result.children).toEqual([...created, ...reused]); + }); + it("reuses every child under a different application key, creating no duplicates", async () => { const first = fakeGitHub(); const store = memoryStore(); @@ -1083,6 +1105,28 @@ describe("makePrismaApplicationStore", () => { expect(claimed.existing).toEqual(winner); }); + it("reads the winner when it loses a concurrent child claim (P2002)", async () => { + const c = client(); + const winner: ChildClaimRecord = { + childKey: "child-key", + childNumber: 43, + childUrl: "https://github.com/org/repo/issues/43", + applicationKey: KEY, + }; + c.groomingChildClaim.findUnique.mockResolvedValueOnce(null).mockResolvedValueOnce(winner); + c.groomingChildClaim.create.mockRejectedValueOnce(Object.assign(new Error("Unique constraint failed"), { code: "P2002" })); + const store = makePrismaApplicationStore(c); + const claimed = await store.claimChild({ + childKey: "child-key", + parentIssueId: "issue-42", + repoFullName: "org/repo", + parentNumber: 42, + title: "Child issue A", + applicationKey: KEY, + }); + expect(claimed.existing).toEqual(winner); + }); + it("propagates other database errors, so nothing is applied unclaimed", async () => { const c = client(); c.groomingApplication.create.mockRejectedValueOnce(new Error("connection refused")); @@ -1091,4 +1135,21 @@ describe("makePrismaApplicationStore", () => { store.claim({ applicationKey: KEY, issueId: "i", groomingRunId: "r1", repoFullName: "org/repo", issueNumber: 42 }), ).rejects.toThrow("connection refused"); }); + + it("propagates other database errors from a child-claim create, so nothing is claimed unrecorded", async () => { + const c = client(); + c.groomingChildClaim.findUnique.mockResolvedValueOnce(null); + c.groomingChildClaim.create.mockRejectedValueOnce(new Error("connection refused")); + const store = makePrismaApplicationStore(c); + await expect( + store.claimChild({ + childKey: "child-key", + parentIssueId: "issue-42", + repoFullName: "org/repo", + parentNumber: 42, + title: "Child issue A", + applicationKey: KEY, + }), + ).rejects.toThrow("connection refused"); + }); }); diff --git a/src/lib/groomer/mutation-applier.ts b/src/lib/groomer/mutation-applier.ts index a6435f76..8f5973ab 100644 --- a/src/lib/groomer/mutation-applier.ts +++ b/src/lib/groomer/mutation-applier.ts @@ -501,10 +501,11 @@ export interface ChildClaimRecord { childUrl: string | null; /** * The application that claimed this child (dispatch#1066); null for rows - * written before the column existed (none in any deployed env). A same-key - * retry is provably exclusive — the GroomingApplication claim/resume CAS - * lets exactly one attempt per application key proceed — so a null claim - * under MY key is my own abandoned create, not a concurrent holder. + * written before the column existed (none in any deployed env). The resume + * CAS (which serializes attempts while the prior application claim is fresh) + * plus the single-replica confinement of the groomer make concurrent + * same-key application attempts not expected, so a null claim under MY key + * is my own abandoned create, not a concurrent holder. */ applicationKey: string | null; /** When the claim row was last written; the prisma store returns it. */ @@ -702,6 +703,7 @@ export async function applyGroomingMutations( let failure: ApplyResult["failure"] = null; let commentUrl: string | null = null; let labels = diff.labelsBefore; + let appliedChildren: ChildIssueLink[] = []; const persist = async (status: string) => { try { @@ -720,6 +722,11 @@ export async function applyGroomingMutations( if (landed(prior[step])) { steps[step] = { ...prior[step]!, status: "replayed" }; if (step === "comment") commentUrl = prior.comment?.commentUrl ?? null; + // A replayed children step surfaces its links in the result, the same + // as the full-replay path. + if (step === "children" && prior.children?.children) { + appliedChildren = [...prior.children.children.created, ...prior.children.children.reused]; + } return; } if (failure) { @@ -836,7 +843,6 @@ export async function applyGroomingMutations( // before the close, so a close is never applied on top of a decomposition // that failed to land. const children = diff.children; - let appliedChildren: ChildIssueLink[] = []; await run( "children", children !== null, @@ -867,9 +873,11 @@ export async function applyGroomingMutations( // updatedAt is held by another in-flight attempt claiming the same // child; do not create a duplicate on top of it. A null claim under // THIS key is my own abandoned create: the GroomingApplication - // resume CAS already excludes a concurrent same-key attempt, so - // creating on top of it is how an immediate same-key retry - // converges. A row with no applicationKey (none exists in any + // resume CAS (which serializes attempts only while the prior + // application claim is fresh) plus the single-replica confinement + // of the groomer make concurrent same-key application attempts not + // expected, so creating on top of it is how an immediate same-key + // retry converges. A row with no applicationKey (none exists in any // deployed env; the migration ships with the feature) is not mine, // so a fresh one is a foreign holder. const heldAt = existing?.updatedAt ? new Date(existing.updatedAt).getTime() : NaN; From fe43d8f3ba6ce29f9edb1f2aadef09fa0714854b Mon Sep 17 00:00:00 2001 From: Courier Date: Tue, 6 Oct 2026 18:15:38 +0000 Subject: [PATCH 7/8] fix(deps): audit-fix transitive advisories (proxy-addr 2.0.8, sharp 0.35.5, source-map-js 1.2.2) --- package-lock.json | 252 +++++++++++++++++++++++----------------------- 1 file changed, 128 insertions(+), 124 deletions(-) diff --git a/package-lock.json b/package-lock.json index 59fd7985..afb58109 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1380,9 +1380,9 @@ } }, "node_modules/@img/sharp-darwin-arm64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-darwin-arm64/-/sharp-darwin-arm64-0.35.4.tgz", - "integrity": "sha512-Uhfl4V4lhP2nbUVF9+hyH1+luj86f1gUFeo8ALYxFoULoU+G87D43BfeMP8XHsk9boxAnCY/bf2EHwhA7MuGsA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-darwin-arm64/-/sharp-darwin-arm64-0.35.5.tgz", + "integrity": "sha512-QRUlFQ0WxvdWyqqG/WtI3iupfD5rBzmCHXSdPsY91sAtVtTo7Q4cb6zOccZ3gqEqkr0f1As1ehLqmEpDsRf+lg==", "cpu": [ "arm64" ], @@ -1398,13 +1398,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-darwin-arm64": "1.3.3" + "@img/sharp-libvips-darwin-arm64": "1.3.4" } }, "node_modules/@img/sharp-darwin-x64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-darwin-x64/-/sharp-darwin-x64-0.35.4.tgz", - "integrity": "sha512-hWniXY3bG5qKpkKrAwPe4y+VTPmf086YQAnkxWh7uA1YrlRouWGa0M0Mxj3ZjnXFkv7/TD1bTy9lGUK26vRvWw==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-darwin-x64/-/sharp-darwin-x64-0.35.5.tgz", + "integrity": "sha512-+BR255RhDlpygUpOc/Jdt1nT6DQ3XG/ERo5wbcdOf5Q320dKtPCKPLR1LJs9VGXRaMa8l1uUa0tkCNOXiAxZUw==", "cpu": [ "x64" ], @@ -1420,20 +1420,20 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-darwin-x64": "1.3.3" + "@img/sharp-libvips-darwin-x64": "1.3.4" } }, "node_modules/@img/sharp-freebsd-wasm32": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-freebsd-wasm32/-/sharp-freebsd-wasm32-0.35.4.tgz", - "integrity": "sha512-lIsKw/BU+kjB4eZjxrYrZmwOJYi3Ajrv66iAlBmUPyKc3HpnloevB1g3wxGD9P/5BbQ1brBGl65VRRrCvQDEqA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-freebsd-wasm32/-/sharp-freebsd-wasm32-0.35.5.tgz", + "integrity": "sha512-Y/z91nEZ4uIBX5X3nfTovjU9lHNKFYbL2lpHCLVNmXQK03VIZvXBBt0KxbPGp2SdGSF+2mQU4e+hQaWOt86iAw==", "license": "Apache-2.0", "optional": true, "os": [ "freebsd" ], "dependencies": { - "@img/sharp-wasm32": "0.35.4" + "@img/sharp-wasm32": "0.35.5" }, "engines": { "node": ">=20.9.0" @@ -1443,9 +1443,9 @@ } }, "node_modules/@img/sharp-libvips-darwin-arm64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-darwin-arm64/-/sharp-libvips-darwin-arm64-1.3.3.tgz", - "integrity": "sha512-suTBPTDGrI9WodccaDdwZItTSaBYASlBk1NSfElSHrUfzu3szG6lvIF58+WiFvnfzuK8ZBFS5zE00PxqxnRiPg==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-darwin-arm64/-/sharp-libvips-darwin-arm64-1.3.4.tgz", + "integrity": "sha512-5R89nBYiRdUlSWJxPhO+GVtaXzXSxKnRu/xqMn3KTA3L9EB9Oy/P+Nn2f2vlhPuUdy/Zusb2DarbyTpGCfEDuw==", "cpu": [ "arm64" ], @@ -1459,9 +1459,9 @@ } }, "node_modules/@img/sharp-libvips-darwin-x64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-darwin-x64/-/sharp-libvips-darwin-x64-1.3.3.tgz", - "integrity": "sha512-FVJZ5mITMobmXIz/hPDTw0EintTW5H3WfrxwLqEqjiIihlu+hVRyGrFQ60xl0Lxn7Bt3zdpevPaQi0HEzqz9fw==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-darwin-x64/-/sharp-libvips-darwin-x64-1.3.4.tgz", + "integrity": "sha512-iR2OKH80yi0U+dUplyh3/xdpFvps6YkCwsXenIJxqxR1v9o+xtKTGbS9H7cps+2Vxjc8B1j96p75NmTGjIhtpQ==", "cpu": [ "x64" ], @@ -1475,9 +1475,9 @@ } }, "node_modules/@img/sharp-libvips-linux-arm": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-arm/-/sharp-libvips-linux-arm-1.3.3.tgz", - "integrity": "sha512-3rbU4vqXXc3hY/OiXdl52xZvT0F1yEngWfvqudtPJg/KkyiaQw2DRsFrNzpmLvfavbwOq3qXn36GP8obHRULQA==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-arm/-/sharp-libvips-linux-arm-1.3.4.tgz", + "integrity": "sha512-LmRtTsOHuvM2+wlO2Db37dx5MiZhB0FvSunciw48YjdOkZz9KAiRbm8ujeMOA1INqmei5NapFxYEK1D1ZSidmw==", "cpu": [ "arm" ], @@ -1494,9 +1494,9 @@ } }, "node_modules/@img/sharp-libvips-linux-arm64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-arm64/-/sharp-libvips-linux-arm64-1.3.3.tgz", - "integrity": "sha512-0DaL0A6Xu6sQSQFwe4iVCrKWU2cCTItnRsYsCdxAMm9NF6twAA9BKnoqy4hqz4+azQ0JHuA26qiUKsf1XJ/v5A==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-arm64/-/sharp-libvips-linux-arm64-1.3.4.tgz", + "integrity": "sha512-Y3dgX/6lE2QhQb+Gxy0WZxfg9MEm/JBjamZpS2IklP7xIQoKN4hzAm7KcMVGtaVDt3neE9OKBC7vAfonA/Lr1A==", "cpu": [ "arm64" ], @@ -1513,9 +1513,9 @@ } }, "node_modules/@img/sharp-libvips-linux-ppc64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-ppc64/-/sharp-libvips-linux-ppc64-1.3.3.tgz", - "integrity": "sha512-cdn1OvUBwsXhbC0zSzJnNzf5MZ/mTrobawDvNXBTxe8VtqKAm0sRuEY2Evzovb/w9JMk4TvRxqt1mekSuJz64w==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-ppc64/-/sharp-libvips-linux-ppc64-1.3.4.tgz", + "integrity": "sha512-Le6boB8Tai0Nis+gIxIpKx68UDVVIqdR8Tin5Yf1z2LJJQLDJvCDRqRu+jC2qCoD+eIomonmOwB4smBRxfVpYQ==", "cpu": [ "ppc64" ], @@ -1532,9 +1532,9 @@ } }, "node_modules/@img/sharp-libvips-linux-riscv64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-riscv64/-/sharp-libvips-linux-riscv64-1.3.3.tgz", - "integrity": "sha512-HjPVx7yKz+0lqdhDlTw1tt90wamBoxhiXpvl1XZpJLiHH4RCJ5yDTqH+VlYPv2fwFs89JFw4c1IexYOcQUi4IQ==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-riscv64/-/sharp-libvips-linux-riscv64-1.3.4.tgz", + "integrity": "sha512-aHkkIEHPRdQEegJN20MLmGtxYD9R2wQr3Cwpddnu5+YKMt6Uzax7S9h5gpZTo8wyrGuZSlfQ63OevL5mTyOC7Q==", "cpu": [ "riscv64" ], @@ -1551,9 +1551,9 @@ } }, "node_modules/@img/sharp-libvips-linux-s390x": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-s390x/-/sharp-libvips-linux-s390x-1.3.3.tgz", - "integrity": "sha512-neWLh+3yCNThxnfy3c4BbVBeGgt9aftno+XbT56iK28RgeDs3UOFWviLWlUu0bArYVYJaFDK+RRohbicUNCm8Q==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-s390x/-/sharp-libvips-linux-s390x-1.3.4.tgz", + "integrity": "sha512-ra/mB6MikESDUO7Yg+Mi95bFBb9GsObURuhnOv3OqknjGe9sZrG8tCe9q0xSIGrtLgvgw0gKnFWcK4blSgQOuQ==", "cpu": [ "s390x" ], @@ -1570,9 +1570,9 @@ } }, "node_modules/@img/sharp-libvips-linux-x64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-x64/-/sharp-libvips-linux-x64-1.3.3.tgz", - "integrity": "sha512-4vKmvAst9nrowcqquKFAyZJUDolUaIp8uRiN0mWFguJ1IplC9/pitXtlnnlU4aa/eJw3J7i67V+pwUL+wZGdsA==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linux-x64/-/sharp-libvips-linux-x64-1.3.4.tgz", + "integrity": "sha512-GJ//SSXbnwSDes02umB3nDJLFcQzw8a18V8fyhqr6tV515tOEMdImjjxj1AoafMRz56F3PHgftnj1QEKSU1zkw==", "cpu": [ "x64" ], @@ -1589,9 +1589,9 @@ } }, "node_modules/@img/sharp-libvips-linuxmusl-arm64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linuxmusl-arm64/-/sharp-libvips-linuxmusl-arm64-1.3.3.tgz", - "integrity": "sha512-Y9kQaLMuNoB0bPYOOdcZMaseNrFpPodIWWMrx+CZyydf2xn68j9WYc6sWWRrDwNkzCQjKYfc68L7jKjGlHMibw==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linuxmusl-arm64/-/sharp-libvips-linuxmusl-arm64-1.3.4.tgz", + "integrity": "sha512-hvulFwtjUcagsis6BBxHwGFwWoNZjgYmULGVrZcyfNbjA8hKILbRxGg15/7w5HDyXHXUos/j6baAWqnCyQ2DWA==", "cpu": [ "arm64" ], @@ -1608,9 +1608,9 @@ } }, "node_modules/@img/sharp-libvips-linuxmusl-x64": { - "version": "1.3.3", - "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linuxmusl-x64/-/sharp-libvips-linuxmusl-x64-1.3.3.tgz", - "integrity": "sha512-fj8Mv0HHfD1Rr+4I68+3agJynxDWtBFgicTbSOb9Bke6pIwzGcJ+RX/yHjmiEGFMCavY/dxvem7MyNaJF+wDiw==", + "version": "1.3.4", + "resolved": "https://registry.npmjs.org/@img/sharp-libvips-linuxmusl-x64/-/sharp-libvips-linuxmusl-x64-1.3.4.tgz", + "integrity": "sha512-6zXKeE/p39I1AmA3cJG35eyBGNqNddLnUXjhwBnsGjFPWqf5VKkDBEqaEkPDoTEtkxwi2vv8Tcr2mDyP4So7Fg==", "cpu": [ "x64" ], @@ -1627,9 +1627,9 @@ } }, "node_modules/@img/sharp-linux-arm": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linux-arm/-/sharp-linux-arm-0.35.4.tgz", - "integrity": "sha512-7OAS8gI0EReKGVN2HssHlM6umJgxF5VI3xN0p9FA91p/YO+ou5hiNghLdZ5BEHztwaaK5+bLKRf8x/o2L2nk9A==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linux-arm/-/sharp-linux-arm-0.35.5.tgz", + "integrity": "sha512-LEaXK2WdXVK5ykcw0buWyPMsmLLL2vpHLD6yrNSW+JGEL3BZPA4tpKN6iaMc4AxTTAoaX/sU1rOL51lcIz48ZQ==", "cpu": [ "arm" ], @@ -1648,13 +1648,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linux-arm": "1.3.3" + "@img/sharp-libvips-linux-arm": "1.3.4" } }, "node_modules/@img/sharp-linux-arm64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linux-arm64/-/sharp-linux-arm64-0.35.4.tgz", - "integrity": "sha512-De4jpEnAU8Hd5oT0j1G3uL4ZvTuipVMn7YC6vPaJhy6/7EwEae0SVAoBrUMYQbkLGDm85taVWwuPc1a44LTzCQ==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linux-arm64/-/sharp-linux-arm64-0.35.5.tgz", + "integrity": "sha512-LYVx5JTsOM2CBzmxreh+nl64/3H6Xb09iSLknqH47z2T2DFFxDeFLP5y4dJwe6H7uGQlHPyEEtIqyo3DYsRwdQ==", "cpu": [ "arm64" ], @@ -1673,13 +1673,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linux-arm64": "1.3.3" + "@img/sharp-libvips-linux-arm64": "1.3.4" } }, "node_modules/@img/sharp-linux-ppc64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linux-ppc64/-/sharp-linux-ppc64-0.35.4.tgz", - "integrity": "sha512-2oYZJeIl4kCcMGk4ouZVjnkCtFrpQFlNEtJ6GbxzhHQchwH0NH/qEb9ykmOl29dqwMq+JhFdZn+1ak2FKhI9fQ==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linux-ppc64/-/sharp-linux-ppc64-0.35.5.tgz", + "integrity": "sha512-QVxAAq8evVRI9ia2vqgwrmWucn5Dfv+JdWzj75pD8omHLPSP7f8p20O8jxzjCcuCEQEOtYOZUmX1hkiZ0kdevA==", "cpu": [ "ppc64" ], @@ -1698,13 +1698,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linux-ppc64": "1.3.3" + "@img/sharp-libvips-linux-ppc64": "1.3.4" } }, "node_modules/@img/sharp-linux-riscv64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linux-riscv64/-/sharp-linux-riscv64-0.35.4.tgz", - "integrity": "sha512-cPbNChoRURAWdebDIHSenxRpgEdy7JkPydSnUxRm9VvKD7m0/xVaR/8Fzlu81pk5nHEvHH87UZUA7cTtwnbJSA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linux-riscv64/-/sharp-linux-riscv64-0.35.5.tgz", + "integrity": "sha512-LtdreXguaavKODPIfzJ4kffx7UNt1omwtK0rch4EBbbSTXPnxWmYSayXdLJw0fJzQ97kHt1gL/yh4tvU+nCyRQ==", "cpu": [ "riscv64" ], @@ -1723,13 +1723,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linux-riscv64": "1.3.3" + "@img/sharp-libvips-linux-riscv64": "1.3.4" } }, "node_modules/@img/sharp-linux-s390x": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linux-s390x/-/sharp-linux-s390x-0.35.4.tgz", - "integrity": "sha512-RY0JFY8Fd6RonCBtHz+DvadaPkXDSI1AUn6yWL9TipqkZ1vY8w8evqdgyDFnkm4/K1ve1TvZiaePP5oSd4+WVQ==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linux-s390x/-/sharp-linux-s390x-0.35.5.tgz", + "integrity": "sha512-UZasTOFiYzotTsGOCu42BfUzP6Tu6Do/947iRm1RsLKvlllxwGcn4RN27LibGWceix4Y+Pmw3jsnTcCQIgWjqA==", "cpu": [ "s390x" ], @@ -1748,13 +1748,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linux-s390x": "1.3.3" + "@img/sharp-libvips-linux-s390x": "1.3.4" } }, "node_modules/@img/sharp-linux-x64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linux-x64/-/sharp-linux-x64-0.35.4.tgz", - "integrity": "sha512-9qvvEAuk8k89TfWUoX2htWjbAMX8p+NxCppjpcg5k6xMsjhBQPTsoIh36h9Qde4WRuGpJeYnOjdosDn/cnv+OA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linux-x64/-/sharp-linux-x64-0.35.5.tgz", + "integrity": "sha512-SxFtLTeJInhAA9Q836kux2vZNeOBQEx658qvbboZScr0wIARym3IcGmW7KpVD5sbVg0Ojy+udFQdayYIZyoNog==", "cpu": [ "x64" ], @@ -1773,13 +1773,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linux-x64": "1.3.3" + "@img/sharp-libvips-linux-x64": "1.3.4" } }, "node_modules/@img/sharp-linuxmusl-arm64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linuxmusl-arm64/-/sharp-linuxmusl-arm64-0.35.4.tgz", - "integrity": "sha512-KB5jxpfWQTr0nc3xdHtWChdbifHrBGsd2SM62Eyxrl8afikm+f5qGBU75SJIZBT/S1MC8XyacdlXBMSWq6OURA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linuxmusl-arm64/-/sharp-linuxmusl-arm64-0.35.5.tgz", + "integrity": "sha512-9HbMclmI1zlNkFRs3z9/eBtDjfD0sGlrX1z6b1qwmiFY5ElDLh4BC0LPBdVp7z1DXFiKlIcznf+ZlsuZzLxQqg==", "cpu": [ "arm64" ], @@ -1798,13 +1798,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linuxmusl-arm64": "1.3.3" + "@img/sharp-libvips-linuxmusl-arm64": "1.3.4" } }, "node_modules/@img/sharp-linuxmusl-x64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-linuxmusl-x64/-/sharp-linuxmusl-x64-0.35.4.tgz", - "integrity": "sha512-f+eZJZIQNEEd26RPSW+76chwOf1XtA2Y/O+5ocVyLliHkeih3e+jhLVBdNTd2rS3IbNXK8+ug93Vf5ZXtF5Lxg==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-linuxmusl-x64/-/sharp-linuxmusl-x64-0.35.5.tgz", + "integrity": "sha512-4KOphqB035HrVdqLZfCgMzzERrQkkzOwRhl4OAkRO1YCldbaFjySXMaK534Mo0V+LndnlJk+sbUyLeU0ULyD1A==", "cpu": [ "x64" ], @@ -1823,13 +1823,13 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-libvips-linuxmusl-x64": "1.3.3" + "@img/sharp-libvips-linuxmusl-x64": "1.3.4" } }, "node_modules/@img/sharp-wasm32": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-wasm32/-/sharp-wasm32-0.35.4.tgz", - "integrity": "sha512-zQnl4Kwp7Q6NHsENtU2T/00Zi+w3AQNwz3+UaTyVBy2FpXrzXzGjndpK61onhZjRtRpQXxCTeqw19bVyXOh7jA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-wasm32/-/sharp-wasm32-0.35.5.tgz", + "integrity": "sha512-Ptsga1su4tQx+LLF1ECS9U6nz5kmrXKo6XVbtR48Ke3ZRxxgaWBu7IDtEe1quo8hiupwm6WFqxVlXaSf7IINGQ==", "license": "Apache-2.0 AND LGPL-3.0-or-later AND MIT", "optional": true, "dependencies": { @@ -1843,16 +1843,16 @@ } }, "node_modules/@img/sharp-webcontainers-wasm32": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-webcontainers-wasm32/-/sharp-webcontainers-wasm32-0.35.4.tgz", - "integrity": "sha512-ESfNkywmCfPNyaZjxooddJQiQ+l/nTpGEOGthxiLnIHXC/CmcBixnfwUleX9mCz9ovrUUvKMap/pm8RYbzfwaA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-webcontainers-wasm32/-/sharp-webcontainers-wasm32-0.35.5.tgz", + "integrity": "sha512-hfhF/FmoQyTUkA0bIKFOtw536BQSeBMe6BF6QyWlrPxT754+TFLaZ7sKKTfvvM0yJgKgaYTwnFCIZ/GuDw5SUA==", "cpu": [ "wasm32" ], "license": "Apache-2.0", "optional": true, "dependencies": { - "@img/sharp-wasm32": "0.35.4" + "@img/sharp-wasm32": "0.35.5" }, "engines": { "node": ">=20.9.0" @@ -1862,9 +1862,9 @@ } }, "node_modules/@img/sharp-win32-arm64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-win32-arm64/-/sharp-win32-arm64-0.35.4.tgz", - "integrity": "sha512-iNdlBX9gLVvqe2I3uIJSIKTq6wckP/DYxZtcqxm09x5Gi24DnFBmPAWZmr60ZyYMG0xlzo6goG3670ar+RXvRw==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-win32-arm64/-/sharp-win32-arm64-0.35.5.tgz", + "integrity": "sha512-X4t7g+7ZA5DKblCBEXGjUqqemj4vczING/5viFwAL8h4N3qYeyjwdCvRLHi4EdOUI+2Z7UFlp1VM+p/AuEtm6Q==", "cpu": [ "arm64" ], @@ -1881,9 +1881,9 @@ } }, "node_modules/@img/sharp-win32-ia32": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-win32-ia32/-/sharp-win32-ia32-0.35.4.tgz", - "integrity": "sha512-kqRsbaa5CS6KHlpxnN7WhE6vAAugXyZButpRdvDWetlv6Qv4N9WTcrWzF7tXfB9T7MsoadqdI8hmwLq6UlLvtw==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-win32-ia32/-/sharp-win32-ia32-0.35.5.tgz", + "integrity": "sha512-5Zm82LoBc43nhwNybZlG7Y1KO//Zhsn306fQl29ZOuStHLGTo3BWL83q3cznX0poxSAMuYL1On/BHBxkBeKr6A==", "cpu": [ "ia32" ], @@ -1900,9 +1900,9 @@ } }, "node_modules/@img/sharp-win32-x64": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/@img/sharp-win32-x64/-/sharp-win32-x64-0.35.4.tgz", - "integrity": "sha512-XtmnYhBcrORsJ4XJngyzr/EWP0hRZLAZRFaApdKuviyqF78+ylxh2y06ZmtULAMOnObJ3ucpN0AcwSWnMowTRg==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/@img/sharp-win32-x64/-/sharp-win32-x64-0.35.5.tgz", + "integrity": "sha512-x76eH0vEiHlcMQu8Y8IenntaACtddpT6W0wmXtWrnKcnKI7ME5DdgqhAD6SEWOEl1v2zDvkZDhFA9KnURwpfqg==", "cpu": [ "x64" ], @@ -9440,9 +9440,9 @@ "license": "ISC" }, "node_modules/proxy-addr": { - "version": "2.0.7", - "resolved": "https://registry.npmjs.org/proxy-addr/-/proxy-addr-2.0.7.tgz", - "integrity": "sha512-llQsMLSUDUPT44jdrU/O37qlnifitDP+ZwrmmZcoSKyLKvtZxpyV0n2/bD/N4tBAAZ/gJEdZU7KMraoK1+XYAg==", + "version": "2.0.8", + "resolved": "https://registry.npmjs.org/proxy-addr/-/proxy-addr-2.0.8.tgz", + "integrity": "sha512-5nnx0yGyVUcY6t9RnWcARWtwT9F1D8O9rt08htPvnd49W1IgZtmLkhu9WfMzQj1cFxjHIO6connUNVW5k7AVyQ==", "license": "MIT", "dependencies": { "forwarded": "0.2.0", @@ -9450,6 +9450,10 @@ }, "engines": { "node": ">= 0.10" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/express" } }, "node_modules/punycode": { @@ -10048,9 +10052,9 @@ "license": "ISC" }, "node_modules/sharp": { - "version": "0.35.4", - "resolved": "https://registry.npmjs.org/sharp/-/sharp-0.35.4.tgz", - "integrity": "sha512-n++8XWcj+jCOr2IOl7h8LbKnGBDY4aPbmprMONBNFdn0ImXqpGVv5zliDs0V9HbmbCQLpbuo2ej9rAoOQTvMDA==", + "version": "0.35.5", + "resolved": "https://registry.npmjs.org/sharp/-/sharp-0.35.5.tgz", + "integrity": "sha512-Ywn4OnzGukp7CDMrp08RQ50YKmuwG47brZgIVPTvBaaAfQlRlygrRqSrxdCiL9M+LlzLBiJ68IR1QqvzHyjC7g==", "license": "Apache-2.0", "optional": true, "dependencies": { @@ -10065,31 +10069,31 @@ "url": "https://opencollective.com/libvips" }, "optionalDependencies": { - "@img/sharp-darwin-arm64": "0.35.4", - "@img/sharp-darwin-x64": "0.35.4", - "@img/sharp-freebsd-wasm32": "0.35.4", - "@img/sharp-libvips-darwin-arm64": "1.3.3", - "@img/sharp-libvips-darwin-x64": "1.3.3", - "@img/sharp-libvips-linux-arm": "1.3.3", - "@img/sharp-libvips-linux-arm64": "1.3.3", - "@img/sharp-libvips-linux-ppc64": "1.3.3", - "@img/sharp-libvips-linux-riscv64": "1.3.3", - "@img/sharp-libvips-linux-s390x": "1.3.3", - "@img/sharp-libvips-linux-x64": "1.3.3", - "@img/sharp-libvips-linuxmusl-arm64": "1.3.3", - "@img/sharp-libvips-linuxmusl-x64": "1.3.3", - "@img/sharp-linux-arm": "0.35.4", - "@img/sharp-linux-arm64": "0.35.4", - "@img/sharp-linux-ppc64": "0.35.4", - "@img/sharp-linux-riscv64": "0.35.4", - "@img/sharp-linux-s390x": "0.35.4", - "@img/sharp-linux-x64": "0.35.4", - "@img/sharp-linuxmusl-arm64": "0.35.4", - "@img/sharp-linuxmusl-x64": "0.35.4", - "@img/sharp-webcontainers-wasm32": "0.35.4", - "@img/sharp-win32-arm64": "0.35.4", - "@img/sharp-win32-ia32": "0.35.4", - "@img/sharp-win32-x64": "0.35.4" + "@img/sharp-darwin-arm64": "0.35.5", + "@img/sharp-darwin-x64": "0.35.5", + "@img/sharp-freebsd-wasm32": "0.35.5", + "@img/sharp-libvips-darwin-arm64": "1.3.4", + "@img/sharp-libvips-darwin-x64": "1.3.4", + "@img/sharp-libvips-linux-arm": "1.3.4", + "@img/sharp-libvips-linux-arm64": "1.3.4", + "@img/sharp-libvips-linux-ppc64": "1.3.4", + "@img/sharp-libvips-linux-riscv64": "1.3.4", + "@img/sharp-libvips-linux-s390x": "1.3.4", + "@img/sharp-libvips-linux-x64": "1.3.4", + "@img/sharp-libvips-linuxmusl-arm64": "1.3.4", + "@img/sharp-libvips-linuxmusl-x64": "1.3.4", + "@img/sharp-linux-arm": "0.35.5", + "@img/sharp-linux-arm64": "0.35.5", + "@img/sharp-linux-ppc64": "0.35.5", + "@img/sharp-linux-riscv64": "0.35.5", + "@img/sharp-linux-s390x": "0.35.5", + "@img/sharp-linux-x64": "0.35.5", + "@img/sharp-linuxmusl-arm64": "0.35.5", + "@img/sharp-linuxmusl-x64": "0.35.5", + "@img/sharp-webcontainers-wasm32": "0.35.5", + "@img/sharp-win32-arm64": "0.35.5", + "@img/sharp-win32-ia32": "0.35.5", + "@img/sharp-win32-x64": "0.35.5" }, "peerDependenciesMeta": { "@types/node": { @@ -10216,9 +10220,9 @@ } }, "node_modules/source-map-js": { - "version": "1.2.1", - "resolved": "https://registry.npmjs.org/source-map-js/-/source-map-js-1.2.1.tgz", - "integrity": "sha512-UXWMKhLOwVKb728IUtQPXxfYU+usdybtUrK/8uGE8CQMvrhOpwvzDBwj0QhSL7MQc7vIsISBG8VQ8+IDQxpfQA==", + "version": "1.2.2", + "resolved": "https://registry.npmjs.org/source-map-js/-/source-map-js-1.2.2.tgz", + "integrity": "sha512-KGj/8Y43x35aZVDtt+J4mK1hoLGHULMYfSkODJNQjNDC3oW1PqPoxMwo0pLUsWM/UEGzON/NxeHywEfNXNP3Vw==", "license": "BSD-3-Clause", "engines": { "node": ">=0.10.0" From e90dedf3130c63b5ec192fa5dc1bc84e39b11fa9 Mon Sep 17 00:00:00 2001 From: Courier Date: Tue, 6 Oct 2026 22:11:08 +0000 Subject: [PATCH 8/8] feat(groomer): managed decomposition body section and apply-time brief-completeness gate (#1066 human CR) Blocker 1: the children step now writes a concise managed section (dispatch-groomer:decomposition marker pair, one line per child) into the parent body between the decomposition-state write and the final umbrella label, preserving all text outside the section byte-for-byte. It coexists with the enrichment managed section because each writer replaces only its own markers; re-rendering the same child set writes nothing; malformed or oversized bodies refuse only the section write (recorded in the step detail) without blocking the decomposition; model-authored groomer markers are stripped from enrichment content so the namespaces cannot nest. ApplyResult.body reports the real post-apply body for the freshness baseline. Blocker 2: evaluateDecompositionPolicy now enforces the complete bounded implementation brief at the mutation boundary (childBriefCompletenessGaps): non-blank problem/designDecision/verifiedCurrentBehavior and non-empty relevantPaths/inScope/outOfScope/acceptanceCriteria/tests; empty dependencies is legitimate. The system prompt contract matches the gate. --- docs/hosted-groomer.md | 11 +- src/lib/groomer/evals/drafts.ts | 16 +- src/lib/groomer/mutation-applier.test.ts | 382 +++++++++++++++++- src/lib/groomer/mutation-applier.ts | 167 +++++++- src/lib/groomer/mutation-validator.test.ts | 128 +++++- src/lib/groomer/mutation-validator.ts | 55 ++- src/lib/groomer/prompts/system-prompt.test.ts | 10 + src/lib/groomer/prompts/system-prompt.ts | 2 +- src/lib/groomer/run.test.ts | 61 ++- 9 files changed, 791 insertions(+), 41 deletions(-) diff --git a/docs/hosted-groomer.md b/docs/hosted-groomer.md index 1ef93b58..2df16059 100644 --- a/docs/hosted-groomer.md +++ b/docs/hosted-groomer.md @@ -179,10 +179,15 @@ A plan may also split the issue into bounded children instead of (or alongside) - no close in the same plan — a decomposed parent is not closed; - verdict confidence of at least `medium` (a low-confidence split is too speculative to fan out); -- no material uncertainty remaining (the same `verdict.uncertainties[].material` flag the close policy keys on). +- no material uncertainty remaining (the same `verdict.uncertainties[].material` flag the close policy keys on); +- every child brief is a **complete bounded implementation brief**: its `problem`, `designDecision`, and `verifiedCurrentBehavior` are all non-blank, and its `relevantPaths`, `inScope`, `outOfScope`, `acceptanceCriteria`, and `tests` each name at least one entry (`dependencies` may be empty). A child brief that leaves any of those short — a design choice left open, no verified current behavior, no relevant paths, no out-of-scope, no acceptance criteria, no tests — is not bounded, so the whole split is withheld with the gap named in `withheld.decomposition` (`child brief[i] is not a complete bounded implementation brief (missing: …)`). This is an apply-time gate: the validator does not police child-brief completeness (a child brief is not an `ImplementationBrief`), so the gate lives in the decomposition policy the applier evaluates. When it holds, each bounded child brief in `decomposition.childBriefs` becomes its own GitHub issue, created with the child labels (`status/backlog`). Child creation is idempotent: every child is keyed by a stable `childBriefKey` (repository, parent issue number, and the child brief) recorded in the `GroomingChildClaim` table, so a retried attempt reuses a child an earlier attempt already created and opens only the ones still missing. Once every child exists or is reused, the children step records the parent's decomposition state — `decomposed`, `decomposedAt`, `decomposedBy: "hosted-groomer"`, the decomposition reason as the note, and the created child URLs as its `followUpUrls` — through the same `setDecompositionState` helper the operator `POST /api/issues/actions/decompose` route uses, so both paths write the state and its audit entry identically, and **only then** adds the umbrella label, as the step's final write (an additive `addLabel`, not part of the labels write). The ordering matters: the umbrella label removes the issue from groomer selection on every path (the selector excludes `umbrella`-labeled issues on every path, including a targeted re-groom), so it must be the step's last write; any earlier failure (a child create, the state write) keeps the parent re-selectable, so a partially applied decomposition converges on retry. Two convergence behaviors of the child-claim hold are accepted: a fresh null claim under a different application key is held as another in-flight attempt's claim, so if an LLM re-plan produces a new key shortly after a failed attempt, that child's convergence waits out the active-claim window (about 10 minutes) rather than failing forever; and if the final umbrella label add fails after the decomposition state was recorded, a later retry replays the created children but re-records the decomposition state, which appends another `issue_decomposed` audit entry — accepted as audit noise on an already-rare failure path. +Alongside creating the children, the step writes a **managed decomposition section** into the parent's body between the `dispatch-groomer:decomposition:start` and `dispatch-groomer:decomposition:end` markers — a short heading, one line that says what the section is, and one `- #: ` line per created-or-reused child, in brief order. The section renders into the body as it stands after the content step, so it **coexists** with the managed enrichment section that step writes — each is parsed and rendered independently. The write only happens when the section content actually changed (no digests, no timestamps), so re-rendering the same children is a byte-for-byte no-op and a well-groomed parent is never churned; the step records the outcome in its detail (`written`, `unchanged`, or `refused`). A body whose markers a human edit broke (unpaired, repeated, or out of order) is **refused rather than guessed at**, and so is a rendered body that would exceed GitHub's body cap (a write that large would fail on every retry and the umbrella would never land) — in either case the step records the refusal reason in its detail, but the children, the state write, and the umbrella still land, so the decomposition converges on retry. + +Three consequences of the managed section are accepted. Re-planning is **last-write-wins**: a fresh plan with different child briefs replaces the section and the recorded `followUpUrls` with the new child set, and children a previous plan already created stay open as `status/backlog` issues, still traceable to the parent by the `Parent:` backlink in their body rather than by any link on the parent. A single well-formed marker pair in the body is treated as **Dispatch-owned** and replaced in place — only a broken or duplicated pair (unpaired, repeated, or out of order) is refused. And because the section's fixed prose makes the parent body permanently non-sparse, a decomposed parent is **never enriched afterwards**: the content step's sparsity gate sees the section's text and skips the write. + A decomposition that fails any rule is withheld: no child is created and the parent is not decorated, and `withheld.decomposition` records why. ### Compatibility @@ -219,7 +224,7 @@ The diff is computed against the live issue, never Dispatch's cache, and only wh 1. **labels**: priority/type changes and the derived status. For an `already_done` plan the status stays as it was here. 2. **comment**: at most one, with `@` mentions neutralized and a hidden `` marker at its end. Any marker the model wrote into its own text is stripped, and a marker only counts on a comment by an automation author, so nobody else can forge one to suppress or impersonate a groomer comment. 3. **title/body**: one write. A title is rewritten only when the current one is bad (the existing guard). The body is never replaced: enrichment goes into one Dispatch-managed section between `` and `` markers, appended after the human text on first write and replaced in place afterwards. Text outside the section is kept byte for byte. Enrichment still applies only when the human-authored text is sparse, and a body whose markers are unpaired or repeated is left alone. -4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, and no material uncertainty). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and once every child exists or is reused the parent is recorded as decomposed with the child URLs as its follow-ups, and only then decorated as an `umbrella` — the step's final write (see [Decomposition](#decomposition)). +4. **children**, only for a plan that decomposes and still satisfies the decomposition policy (no close in the same plan, at least medium confidence, no material uncertainty, and every child brief a complete bounded implementation brief). Each bounded child brief becomes its own issue (created with the child labels), an already-created child is reused rather than re-opened, and once every child exists or is reused the parent is recorded as decomposed with the child URLs as its follow-ups; the step then writes a managed decomposition section into the parent body (one `- #: ` line per child) and, only then, adds the `umbrella` label — the step's final write (see [Decomposition](#decomposition)). 5. **close**, only for an `already_done` plan that still satisfies the close policy. 6. **status/done**, only once the close has landed, so a failed close leaves the issue open in its previous (groomable) status rather than open with `status/done`, which the selector would skip forever. @@ -240,7 +245,7 @@ Every run captures its own snapshot, so the key only recurs when the issue, its Dry runs use the same preconditions, diff and policies without writing: `mutationPlan.applyOutcome` is `dry_run`, `stale` or `unverifiable` (with `preconditionFailures`), or `would_replay` when the key was already applied. A dry run never claims a key, so it never reports `busy`, and it never writes a backoff. -Out of scope here, and still to come: worker admission gating on these results (#1065), child issue creation (#1066), and UI exposure of the new history fields (#1067). +Out of scope here, and still to come: worker admission gating on these results (#1065), and UI exposure of the new history fields (#1067). ## History and Audit diff --git a/src/lib/groomer/evals/drafts.ts b/src/lib/groomer/evals/drafts.ts index 673f738e..ff409a94 100644 --- a/src/lib/groomer/evals/drafts.ts +++ b/src/lib/groomer/evals/drafts.ts @@ -141,18 +141,22 @@ export function alreadyDone( return draft; } -/** Child briefs for decomposition fixtures. */ +/** + * Child briefs for decomposition fixtures. Every field the apply-time + * decomposition policy requires a complete bounded brief to carry is filled in + * (a brief left incomplete would withhold the split, not create children). + */ export function children(titles: string[]): ChildBrief[] { return titles.map((title) => ({ title, problem: `${title}, as its own bounded change.`, - designDecision: null, - verifiedCurrentBehavior: null, - relevantPaths: [], + designDecision: `${title} follows the existing pattern; no design choice is left open.`, + verifiedCurrentBehavior: `${title} is not implemented yet; the current code does not cover it.`, + relevantPaths: ["src/admin/Dashboard.tsx"], inScope: [title.toLowerCase()], - outOfScope: [], + outOfScope: [`${title} does not change unrelated areas`], dependencies: [], acceptanceCriteria: [`${title} works end to end`], - tests: [], + tests: [`${title} is covered by an automated test`], })); } diff --git a/src/lib/groomer/mutation-applier.test.ts b/src/lib/groomer/mutation-applier.test.ts index e55261f6..cf2b83c0 100644 --- a/src/lib/groomer/mutation-applier.test.ts +++ b/src/lib/groomer/mutation-applier.test.ts @@ -7,15 +7,21 @@ import { validateGroomingPlan, type GroomingPlan, type GroomingPlanDraft } from import type { LiveComment, LiveIssueState } from "./mutation-validator"; import { ACTIVE_CLAIM_MS, + DECOMPOSITION_BODY_END, + DECOMPOSITION_BODY_START, MANAGED_BODY_END, MANAGED_BODY_START, applyGroomingMutations, + parseDecompositionBody, + renderDecompositionBody, + renderDecompositionSection, commentMarker, commentMarkerKey, computeApplicationKey, groomerCommentKey, computeMutationDiff, makePrismaApplicationStore, + MAX_GITHUB_BODY_CHARS, parseManagedBody, renderManagedBody, type ApplicationRecord, @@ -143,17 +149,19 @@ function decomposeDraft(count = 2): GroomingPlanDraft { reason: `splits into ${count} bounded children`, childBriefs: Array.from({ length: count }, (_, i) => { const l = letter(i + 1); + // Every field the apply-time decomposition policy requires a complete + // bounded brief to carry: an incomplete brief withholds the split. return { title: `Child issue ${l}`, problem: `Child ${l} problem`, - designDecision: null, - verifiedCurrentBehavior: null, - relevantPaths: [], + designDecision: `Child ${l} follows the existing pattern; no design choice is left open.`, + verifiedCurrentBehavior: `Child ${l} is not implemented yet; login.ts drops the return URL.`, + relevantPaths: ["src/auth/login.ts"], inScope: [l], - outOfScope: [], + outOfScope: [`Child ${l} does not touch authentication`], dependencies: [], acceptanceCriteria: [`Child ${l} works`], - tests: [], + tests: [`Child ${l} is covered by an automated test`], }; }), }, @@ -210,6 +218,15 @@ describe("managed body section", () => { }); }); +describe("parseDecompositionBody", () => { + it("refuses a section whose end marker precedes its start", () => { + expect(parseDecompositionBody(`${DECOMPOSITION_BODY_END}\nx\n${DECOMPOSITION_BODY_START}`)).toMatchObject({ + ok: false, + reason: "the decomposition section end marker precedes its start", + }); + }); +}); + describe("computeMutationDiff", () => { it("diffs labels from the live issue and leaves the derived status as the only status", () => { const snap = snapshot({ issue: { ...snapshot().issue, labels: ["priority/p1", "status/backlog", "status/needs-review"] } }); @@ -402,6 +419,7 @@ function memoryStore(initial?: ApplicationRecord): ApplicationStore & { function fakeGitHub(overrides: Partial = {}) { const calls: string[] = []; + const bodies: string[] = []; const github: ApplierGitHub = { updateLabels: vi.fn(async (_r, _n, labels: string[]) => { calls.push(`labels:${labels.filter((l) => l.startsWith("status/")).join(",")}`); @@ -415,6 +433,7 @@ function fakeGitHub(overrides: Partial = {}) { }), updateTitleAndBody: vi.fn(async (_r, _n, fields) => { calls.push(`content:${Object.keys(fields).join("+")}`); + if (typeof fields.body === "string") bodies.push(fields.body); }), closeIssue: vi.fn(async () => { calls.push("close"); @@ -426,7 +445,23 @@ function fakeGitHub(overrides: Partial = {}) { fetchRecentComments: vi.fn(async () => []), ...overrides, }; - return { github, calls }; + return { github, calls, bodies }; +} + +/** + * A fakeGitHub whose child creates get a distinct number per brief title + * (the plain fake returns 43 for every child), still recording + * `child:` in `calls` for ordering. + */ +function distinctChildGitHub(overrides: Partial<ApplierGitHub> = {}) { + const base = fakeGitHub(overrides); + const numbers: Record<string, number> = { "Child issue A": 43, "Child issue B": 44, "Child issue C": 45 }; + const createIssue = vi.fn(async (r: string, input: { title: string }) => { + base.calls.push(`child:${input.title}`); + const number = numbers[input.title] ?? 43; + return { number, url: `https://github.com/org/repo/issues/${number}` }; + }); + return { github: { ...base.github, createIssue } as ApplierGitHub, calls: base.calls, bodies: base.bodies, createIssue }; } function applyInput(diff: GroomingMutationDiff, overrides: Partial<ApplyInput> = {}): ApplyInput { @@ -738,7 +773,7 @@ describe("applyGroomingMutations → decomposition", () => { expect(result.children).toHaveLength(2); }); - it("records the decomposition state before adding the umbrella label (step ordering)", async () => { + it("orders the children step's writes: child creates → decomposition state → body section → umbrella label", async () => { const { github, calls } = fakeGitHub(); const store = memoryStore(); const recordState = store.setDecompositionState; @@ -749,12 +784,19 @@ describe("applyGroomingMutations → decomposition", () => { const diff = diffFor(decomposeDraft()); const result = await applyGroomingMutations(applyInput(diff), github, store); expect(result.outcome).toBe("applied"); - // The umbrella, which removes the parent from every selection path, must - // be the children step's final write: the state lands before it. + const childAIdx = calls.indexOf("child:Child issue A"); + const childBIdx = calls.indexOf("child:Child issue B"); const stateIdx = calls.indexOf("decompositionState"); + const bodyIdx = calls.indexOf("content:body"); const addLabelIdx = calls.indexOf("addLabel:umbrella"); - expect(stateIdx).toBeGreaterThan(-1); - expect(addLabelIdx).toBeGreaterThan(stateIdx); + for (const idx of [childAIdx, childBIdx, stateIdx, bodyIdx, addLabelIdx]) expect(idx).toBeGreaterThan(-1); + // Both creates precede the state write; the state write precedes the + // section body write; the umbrella — which removes the parent from every + // selection path — is the step's final write. + expect(stateIdx).toBeGreaterThan(childAIdx); + expect(stateIdx).toBeGreaterThan(childBIdx); + expect(bodyIdx).toBeGreaterThan(stateIdx); + expect(addLabelIdx).toBeGreaterThan(bodyIdx); }); it("reuses a child an earlier attempt already created, creating only the missing one", async () => { @@ -1018,6 +1060,324 @@ describe("applyGroomingMutations → decomposition", () => { }); }); +describe("applyGroomingMutations → decomposition body section", () => { + it("writes one parent body whose section lists every child, keeping the human text byte for byte", async () => { + const { github, bodies } = distinctChildGitHub(); + const store = memoryStore(); + const diff = diffFor(decomposeDraft()); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + // One parent body write for the whole application: the children step's section. + expect(bodies).toHaveLength(1); + expect(github.updateTitleAndBody).toHaveBeenCalledTimes(1); + const body = bodies[0]; + // One managed section: the parse succeeding means exactly one of each marker. + const parsed = parseDecompositionBody(body); + expect(parsed.ok).toBe(true); + if (!parsed.ok) throw new Error("unreachable"); + expect(parsed.decomposition).toContain("## Decomposition"); + // One "- #<n>: <url>" line per child, in brief order. + expect(body).toContain("- #43: https://github.com/org/repo/issues/43"); + expect(body).toContain("- #44: https://github.com/org/repo/issues/44"); + for (const child of result.children) expect(body).toContain(`- #${child.number}: ${child.url}`); + // The original human text is preserved byte for byte before the appended section. + expect(body.startsWith("Broken.\n\n")).toBe(true); + expect(parsed.human).toBe("Broken."); + // Re-rendering the same children is a fixed point. + expect(renderDecompositionBody(parsed, renderDecompositionSection(result.children))).toBe(body); + // The step's detail records the counts and the section outcome; the result + // reports the real post-apply body. + expect(result.steps.children?.detail).toBe("2 created, 0 reused; decomposition section written"); + expect(result.body).toBe(body); + }); + + it("coexists with the managed enrichment section: the children step renders into the post-content body", async () => { + const base = decomposeDraft(); + const diff = diffFor({ ...base, mutations: { ...base.mutations, proposedBody: "Notes." } }); + expect(diff.body).not.toBeNull(); + const { github, calls, bodies } = distinctChildGitHub(); + const result = await applyGroomingMutations(applyInput(diff), github, memoryStore()); + expect(result.outcome).toBe("applied"); + // Two parent body writes: the content step's enrichment, then the children + // step's section rendered into the body as it stands after that write. + expect(bodies).toHaveLength(2); + const firstBodyIdx = calls.indexOf("content:body"); + const lastBodyIdx = calls.lastIndexOf("content:body"); + expect(lastBodyIdx).toBeGreaterThan(firstBodyIdx); + expect(calls.indexOf("addLabel:umbrella")).toBeGreaterThan(lastBodyIdx); + // The first is the managed enrichment section alone. + expect(bodies[0]).toBe(`Broken.\n\n${MANAGED_BODY_START}\nNotes.\n${MANAGED_BODY_END}`); + expect(bodies[0]).not.toContain(DECOMPOSITION_BODY_START); + // The last is both sections: the enrichment section kept, the decomposition + // section appended after it. + const section = + "## Decomposition\n\n" + + "Created from this parent's grooming decomposition plan. Each child starts `status/backlog` and needs its own grooming pass before pickup.\n\n" + + "- #43: https://github.com/org/repo/issues/43\n" + + "- #44: https://github.com/org/repo/issues/44"; + const expected = `${bodies[0]}\n\n${DECOMPOSITION_BODY_START}\n${section}\n${DECOMPOSITION_BODY_END}`; + expect(bodies[bodies.length - 1]).toBe(expected); + // The human text survives both managed sections. + const managed = parseManagedBody(bodies[bodies.length - 1]); + expect(managed.ok).toBe(true); + if (!managed.ok) throw new Error("unreachable"); + expect(managed.managed).toBe("Notes."); + const decomp = parseDecompositionBody(managed.human); + expect(decomp.ok).toBe(true); + if (!decomp.ok) throw new Error("unreachable"); + expect(decomp.human).toBe("Broken."); + expect(result.body).toBe(bodies[bodies.length - 1]); + expect(result.steps.children?.detail).toBe("2 created, 0 reused; decomposition section written"); + }); + + it("writes no body when the section already lists the same children (a fixed point), and still lands the umbrella", async () => { + const { github, bodies } = distinctChildGitHub(); + const store = memoryStore(); + const briefs = decomposeDraft().decomposition.childBriefs; + // Every child was already created by an earlier application. + const links = [ + { key: childBriefKey("org/repo", 42, briefs[0]), number: 99, url: "https://github.com/org/repo/issues/99" }, + { key: childBriefKey("org/repo", 42, briefs[1]), number: 100, url: "https://github.com/org/repo/issues/100" }, + ]; + for (const link of links) { + store.children.set(link.key, { + childKey: link.key, + childNumber: link.number, + childUrl: link.url, + applicationKey: KEY, + }); + } + // The live body already carries the section rendered from those same links. + const parsed = parseDecompositionBody("Broken."); + if (!parsed.ok) throw new Error("unreachable"); + const liveBody = renderDecompositionBody(parsed, renderDecompositionSection(links)); + const diff = diffFor(decomposeDraft(), snapshot({ issue: { ...snapshot().issue, body: liveBody } })); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + // Re-rendering the same section is a fixed point: no body write at all. + expect(bodies).toHaveLength(0); + expect(github.updateTitleAndBody).not.toHaveBeenCalled(); + // The post-apply body is the (unchanged) live body. + expect(result.body).toBe(liveBody); + // The children are reused, the state is recorded, and the umbrella still lands. + expect(result.steps.children?.detail).toBe("0 created, 2 reused; decomposition section unchanged"); + expect(result.children.map((l) => l.number)).toEqual([99, 100]); + expect(store.decompositionStates).toHaveLength(1); + expect(store.decompositionStates[0].followUpUrls).toEqual(["https://github.com/org/repo/issues/99", "https://github.com/org/repo/issues/100"]); + expect(github.addLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); + }); + + it("refuses a malformed section (two start markers): no body write, the umbrella still lands, and the detail says so", async () => { + const { github, bodies } = distinctChildGitHub(); + const store = memoryStore(); + // A human edit broke the section markers: two starts, one end. + const liveBody = + `Broken.\n\n` + + `${DECOMPOSITION_BODY_START}\n- #99: https://github.com/org/repo/issues/99\n${DECOMPOSITION_BODY_END}\n\n` + + `${DECOMPOSITION_BODY_START}\n- #100: https://github.com/org/repo/issues/100`; + const snap = snapshot({ issue: { ...snapshot().issue, body: liveBody } }); + const diff = diffFor(decomposeDraft(), snap); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + // The section write is refused, not guessed at: the body is untouched. + expect(bodies).toHaveLength(0); + expect(github.updateTitleAndBody).not.toHaveBeenCalled(); + expect(result.body).toBe(liveBody); + // The children, the state and the umbrella still land. + expect(github.createIssue).toHaveBeenCalledTimes(2); + expect(store.decompositionStates).toHaveLength(1); + expect(github.addLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); + expect(result.steps.children).toMatchObject({ status: "applied" }); + expect(result.steps.children?.detail).toBe( + "2 created, 0 reused; decomposition section write refused: the decomposition section markers are unpaired or repeated", + ); + }); + + it("records the created/reused counts and the section outcome on the step detail in all three outcomes", async () => { + // Written. + const written = await applyGroomingMutations( + applyInput(diffFor(decomposeDraft())), + distinctChildGitHub().github, + memoryStore(), + ); + expect(written.steps.children?.detail).toBe("2 created, 0 reused; decomposition section written"); + // Unchanged: every child reused, section already current. + const briefs = decomposeDraft().decomposition.childBriefs; + const store = memoryStore(); + const links = [ + { key: childBriefKey("org/repo", 42, briefs[0]), number: 99, url: "https://github.com/org/repo/issues/99" }, + { key: childBriefKey("org/repo", 42, briefs[1]), number: 100, url: "https://github.com/org/repo/issues/100" }, + ]; + for (const link of links) { + store.children.set(link.key, { + childKey: link.key, + childNumber: link.number, + childUrl: link.url, + applicationKey: KEY, + }); + } + const parsed = parseDecompositionBody("Broken."); + if (!parsed.ok) throw new Error("unreachable"); + const liveBody = renderDecompositionBody(parsed, renderDecompositionSection(links)); + const snap = snapshot({ issue: { ...snapshot().issue, body: liveBody } }); + const unchanged = await applyGroomingMutations( + applyInput(diffFor(decomposeDraft(), snap)), + distinctChildGitHub().github, + store, + ); + expect(unchanged.steps.children?.detail).toBe("0 created, 2 reused; decomposition section unchanged"); + // Refused: malformed markers in the live body. + const broken = `${DECOMPOSITION_BODY_START}\nx\n${DECOMPOSITION_BODY_START}\ny`; + const refused = await applyGroomingMutations( + applyInput(diffFor(decomposeDraft(), snapshot({ issue: { ...snapshot().issue, body: broken } }))), + distinctChildGitHub().github, + memoryStore(), + ); + expect(refused.steps.children?.detail).toBe( + "2 created, 0 reused; decomposition section write refused: the decomposition section markers are unpaired or repeated", + ); + }); + + it("strips decomposition markers the model embedded in its enrichment, so the children section lands exactly once", async () => { + // A full marker pair the model embedded in its enrichment, and an + // unpaired start marker: without stripping, the pair would be parsed as + // the children step's own section (its model text between the markers + // replaced) and the unpaired one would poison the body into a refusal. + const variants: Array<[proposedBody: string, modelText: string]> = [ + [`Before ${DECOMPOSITION_BODY_START}\nnested model text\n${DECOMPOSITION_BODY_END} After`, "nested model text"], + [`Before ${DECOMPOSITION_BODY_START} orphan marker text After`, "orphan marker text"], + ]; + for (const [proposedBody, modelText] of variants) { + const base = decomposeDraft(); + const diff = diffFor({ ...base, mutations: { ...base.mutations, proposedBody } }); + const { github, bodies } = distinctChildGitHub(); + const result = await applyGroomingMutations(applyInput(diff), github, memoryStore()); + expect(result.outcome).toBe("applied"); + // The content step's enrichment write carries no decomposition marker: + // the model's text survives, the markers do not. + expect(bodies).toHaveLength(2); + expect(bodies[0]).not.toContain(DECOMPOSITION_BODY_START); + expect(bodies[0]).not.toContain(DECOMPOSITION_BODY_END); + expect(bodies[0]).toContain(modelText); + // The children step's section lands exactly once, at the top level. + const finalBody = bodies[bodies.length - 1]; + expect(finalBody.split(DECOMPOSITION_BODY_START).length - 1).toBe(1); + expect(finalBody.split(DECOMPOSITION_BODY_END).length - 1).toBe(1); + const managed = parseManagedBody(finalBody); + expect(managed.ok).toBe(true); + if (!managed.ok) throw new Error("unreachable"); + expect(managed.managed).toContain(modelText); + expect(managed.managed).not.toContain(DECOMPOSITION_BODY_START); + expect(managed.managed).not.toContain(DECOMPOSITION_BODY_END); + const decomp = parseDecompositionBody(finalBody); + expect(decomp.ok).toBe(true); + if (!decomp.ok) throw new Error("unreachable"); + expect(decomp.decomposition).toContain("## Decomposition"); + // The section is not nested inside the managed one. + expect(decomp.decomposition).not.toContain(MANAGED_BODY_START); + // The human text is intact. + expect(decomp.human).toContain("Broken."); + expect(result.steps.children?.detail).toBe("2 created, 0 reused; decomposition section written"); + } + }); + + it("refuses a section write that would exceed GitHub's body cap, and still lands the umbrella", async () => { + const { github, bodies } = distinctChildGitHub(); + const store = memoryStore(); + // A body at the cap: appending the section would exceed it, so the write + // is refused rather than failing on every retry. + const huge = "A".repeat(MAX_GITHUB_BODY_CHARS); + const snap = snapshot({ issue: { ...snapshot().issue, body: huge } }); + const diff = diffFor(decomposeDraft(), snap); + const result = await applyGroomingMutations(applyInput(diff), github, store); + expect(result.outcome).toBe("applied"); + // No body write at all: the body is left exactly as it was. + expect(bodies).toHaveLength(0); + expect(github.updateTitleAndBody).not.toHaveBeenCalled(); + expect(result.body).toBe(huge); + // The children, the state and the umbrella still land. + expect(github.createIssue).toHaveBeenCalledTimes(2); + expect(store.decompositionStates).toHaveLength(1); + expect(github.addLabel).toHaveBeenCalledWith("org/repo", 42, "umbrella"); + expect(result.steps.children).toMatchObject({ status: "applied" }); + expect(result.steps.children?.detail).toBe( + `2 created, 0 reused; decomposition section write refused: body would exceed ${MAX_GITHUB_BODY_CHARS} characters`, + ); + }); + + it("keeps an existing decomposition section byte for byte when the content step replaces the managed section", async () => { + // Cross-application: the parent was enriched by one application and + // decomposed by another; the body carries both managed sections. A fresh + // plan that enriches again must replace only the managed section. + const origDecomp = `${DECOMPOSITION_BODY_START}\n- #99: https://github.com/org/repo/issues/99\n${DECOMPOSITION_BODY_END}`; + const baseBody = `Broken.\n\n${MANAGED_BODY_START}\nold enrichment\n${MANAGED_BODY_END}\n\n${origDecomp}`; + const snap = snapshot({ issue: { ...snapshot().issue, body: baseBody } }); + const base = decomposeDraft(); + const diff = diffFor({ ...base, mutations: { ...base.mutations, proposedBody: "Notes." } }, snap); + const { github, bodies } = distinctChildGitHub(); + const result = await applyGroomingMutations(applyInput(diff), github, memoryStore()); + expect(result.outcome).toBe("applied"); + expect(bodies).toHaveLength(2); + // The content step replaces only the managed section: the pre-existing + // decomposition section survives the enrichment write byte for byte. + expect(bodies[0]).toBe(`Broken.\n\n${MANAGED_BODY_START}\nNotes.\n${MANAGED_BODY_END}\n\n${origDecomp}`); + // The children step then re-renders the decomposition section in place: + // the old child lines are replaced, and both managed sections coexist in + // the final body, each holding its own. + const section = + "## Decomposition\n\n" + + "Created from this parent's grooming decomposition plan. Each child starts `status/backlog` and needs its own grooming pass before pickup.\n\n" + + "- #43: https://github.com/org/repo/issues/43\n" + + "- #44: https://github.com/org/repo/issues/44"; + const finalBody = `Broken.\n\n${MANAGED_BODY_START}\nNotes.\n${MANAGED_BODY_END}\n\n${DECOMPOSITION_BODY_START}\n${section}\n${DECOMPOSITION_BODY_END}`; + expect(bodies[bodies.length - 1]).toBe(finalBody); + const managed = parseManagedBody(bodies[bodies.length - 1]); + expect(managed.ok).toBe(true); + if (!managed.ok) throw new Error("unreachable"); + expect(managed.managed).toBe("Notes."); + const decomp = parseDecompositionBody(bodies[bodies.length - 1]); + expect(decomp.ok).toBe(true); + if (!decomp.ok) throw new Error("unreachable"); + expect(decomp.decomposition).toBe(section); + expect(decomp.human).toContain("Broken."); + expect(result.body).toBe(bodies[bodies.length - 1]); + }); +}); + +describe("applyGroomingMutations → ApplyResult.body", () => { + it("is the newly written section body when the children step wrote the section", async () => { + const { github, bodies } = distinctChildGitHub(); + const result = await applyGroomingMutations(applyInput(diffFor(decomposeDraft())), github, memoryStore()); + expect(result.outcome).toBe("applied"); + expect(result.body).not.toBeNull(); + expect(result.body).toBe(bodies[bodies.length - 1]); + expect(result.body).toContain(DECOMPOSITION_BODY_START); + expect(result.body).toContain(DECOMPOSITION_BODY_END); + }); + + it("is the content step's body when the content step wrote and the plan does not decompose", async () => { + const { github, bodies } = fakeGitHub(); + const diff = diffFor(draft({}, { proposedBody: "Notes." })); + const result = await applyGroomingMutations(applyInput(diff), github, memoryStore()); + expect(result.outcome).toBe("applied"); + expect(result.steps.content).toMatchObject({ status: "applied" }); + expect(result.steps.children).toMatchObject({ status: "noop" }); + expect(bodies).toHaveLength(1); + expect(result.body).toBe(diff.body); + expect(result.body).toContain(MANAGED_BODY_START); + expect(result.body).not.toContain(DECOMPOSITION_BODY_START); + }); + + it("is null when neither the content nor the children step wrote a body", async () => { + const snap = snapshot({ issue: { ...snapshot().issue, labels: ["priority/p1", "status/ready", "type/bug"] } }); + const { github, bodies } = fakeGitHub(); + const result = await applyGroomingMutations(applyInput(diffFor(draft(), snap)), github, memoryStore()); + expect(result.outcome).toBe("noop"); + expect(bodies).toHaveLength(0); + expect(result.body).toBeNull(); + }); +}); + function addCommentBody(github: ApplierGitHub): string { return (github.addComment as ReturnType<typeof vi.fn>).mock.calls[0][2] as string; } diff --git a/src/lib/groomer/mutation-applier.ts b/src/lib/groomer/mutation-applier.ts index 8f5973ab..35ee400d 100644 --- a/src/lib/groomer/mutation-applier.ts +++ b/src/lib/groomer/mutation-applier.ts @@ -43,6 +43,13 @@ import { export const APPLICATION_KEY_VERSION = 1; export const MAX_GITHUB_COMMENT_CHARS = 4096; +/** + * The largest body the applier will write to GitHub. GitHub's issue body cap + * is ~125k characters; a write at or over it fails, so a section render that + * would cross this cap is refused (like a malformed marker) rather than + * failing on every retry and never landing the umbrella. + */ +export const MAX_GITHUB_BODY_CHARS = 120_000; // ─── Label helpers (moved from run.ts) ─────────────────────────────────────── @@ -151,6 +158,77 @@ export function renderManagedBody(parsed: Extract<ManagedBody, { ok: true }>, co return `${human}${separator}${section}`; } +// ─── Managed decomposition section ───────────────────────────────────────────── + +export const DECOMPOSITION_BODY_START = "<!-- dispatch-groomer:decomposition:start -->"; +export const DECOMPOSITION_BODY_END = "<!-- dispatch-groomer:decomposition:end -->"; + +export type DecompositionBody = + | { ok: true; human: string; before: string; after: string; decomposition: string | null } + | { ok: false; reason: string }; + +/** + * Split an issue body around its one managed decomposition section, if + * present. The section the children step writes after a decomposition (the + * children the issue was split into): the same marker discipline as the + * managed section — malformed markers (unpaired, repeated, or out of order) + * are reported rather than guessed at, so a human edit that broke them never + * makes the groomer rewrite text it does not own. + */ +export function parseDecompositionBody(body: string | null): DecompositionBody { + const text = body ?? ""; + const starts = text.split(DECOMPOSITION_BODY_START).length - 1; + const ends = text.split(DECOMPOSITION_BODY_END).length - 1; + if (starts === 0 && ends === 0) return { ok: true, human: text, before: text, after: "", decomposition: null }; + if (starts !== 1 || ends !== 1) return { ok: false, reason: "the decomposition section markers are unpaired or repeated" }; + const start = text.indexOf(DECOMPOSITION_BODY_START); + const end = text.indexOf(DECOMPOSITION_BODY_END); + if (end < start) return { ok: false, reason: "the decomposition section end marker precedes its start" }; + const before = text.slice(0, start); + const after = text.slice(end + DECOMPOSITION_BODY_END.length); + const decomposition = text.slice(start + DECOMPOSITION_BODY_START.length, end).trim(); + const human = [before.trim(), after.trim()].filter((part) => part.length > 0).join("\n\n"); + return { ok: true, human, before, after, decomposition }; +} + +function decompositionSection(content: string): string { + const cleaned = content.split(DECOMPOSITION_BODY_START).join("").split(DECOMPOSITION_BODY_END).join("").trim(); + return `${DECOMPOSITION_BODY_START}\n${cleaned}\n${DECOMPOSITION_BODY_END}`; +} + +/** + * The body with `content` as its one managed decomposition section. An + * existing section is replaced in place; otherwise the section is appended + * after the text that precedes it. Everything outside the section — including + * the managed enrichment section — is kept byte for byte, so re-rendering the + * same content is a fixed point. + */ +export function renderDecompositionBody(parsed: Extract<DecompositionBody, { ok: true }>, content: string): string { + const section = decompositionSection(content); + if (parsed.decomposition !== null) return `${parsed.before}${section}${parsed.after}`; + const human = parsed.before; + if (human.trim().length === 0) return section; + const separator = human.endsWith("\n\n") ? "" : human.endsWith("\n") ? "\n" : "\n\n"; + return `${human}${separator}${section}`; +} + +/** + * The section content: a human-readable heading and the one sentence that + * says what the section is, then one line per created-or-reused child link, + * in brief order. Deterministic — no timestamps, no digests — so re-rendering + * the same children is a fixed point. + */ +export function renderDecompositionSection(children: ChildIssueLink[]): string { + const bullets = children.map((child) => `- #${child.number}: ${child.url}`).join("\n"); + return [ + "## Decomposition", + "", + "Created from this parent's grooming decomposition plan. Each child starts `status/backlog` and needs its own grooming pass before pickup.", + "", + bullets, + ].join("\n"); +} + // ─── Comment marker ─────────────────────────────────────────────────────────── /** Only the marker Dispatch appends counts: it must end the comment. */ @@ -205,6 +283,16 @@ export interface GroomingMutationDiff { title: string | null; /** New full body; null when unchanged. */ body: string | null; + /** + * The live body the diff was computed from, before any of this diff's + * writes; null when the issue has no body. The children step renders its + * managed section into the body as it stands after the content step — the + * post-content body when that step has a write, this live body otherwise. + * The apply preconditions verify the live body equals the evidence + * snapshot's, so this is exactly the body the children step starts from + * when the content step writes nothing. + */ + bodyBefore: string | null; /** Why proposed body enrichment was not applied, when it was not. */ bodySkippedReason: string | null; close: boolean; @@ -344,7 +432,13 @@ export function computeMutationDiff(input: MutationDiffInput): GroomingMutationD } else if (!shouldEnrichBody(parsed.human)) { bodySkippedReason = "the human-authored body is not sparse"; } else { - const rendered = renderManagedBody(parsed, neutralizeMentions(output.proposedBody)); + // A marker the model wrote into its enrichment is stripped before it + // lands (like the comment's, below): a decomposition marker in + // particular would otherwise be parsed as the children step's own + // section — an embedded pair would have the model's text between it + // replaced, an unpaired one would poison the body into a refusal. + const enrichment = neutralizeMentions(output.proposedBody.replace(ANY_GROOMER_MARKER, "")); + const rendered = renderManagedBody(parsed, enrichment); if (rendered !== (live.body ?? "")) body = rendered; else bodySkippedReason = "the managed section already holds this content"; } @@ -364,6 +458,7 @@ export function computeMutationDiff(input: MutationDiffInput): GroomingMutationD comment, title, body, + bodyBefore: live.body, bodySkippedReason, close: done && live.state === "open" && inFlightStatus(live.labels) === null, children, @@ -843,6 +938,15 @@ export async function applyGroomingMutations( // before the close, so a close is never applied on top of a decomposition // that failed to land. const children = diff.children; + // The body the children step starts from: the post-content body when the + // content step has a write (the managed enrichment section, if it lands), + // the live body as diffed otherwise. `childrenFinalBody` records what the + // step's section write leaves the body as (the unchanged body when the + // write is refused or a no-op), so the result can report the real + // post-apply body for the freshness baseline. + const baseBody = diff.body !== null ? diff.body : (diff.bodyBefore ?? ""); + const liveUnchangedBody = diff.body !== null ? diff.body : diff.bodyBefore; + let childrenFinalBody: string | null = null; await run( "children", children !== null, @@ -927,12 +1031,50 @@ export async function applyGroomingMutations( note: children.reason, followUpUrls: links.map((child) => child.url), }); - // The umbrella lands last, now that every child exists or is reused and - // the decomposition state is recorded; a failed create or a failed - // state write throws before this line, so a partial decomposition never + // The managed decomposition section: one section in the parent's body + // listing the children this decomposition created or reused. It + // renders into the body as it stands after the content step, so the + // managed enrichment section and this one coexist, each parsed and + // rendered independently. A failed write here throws like a failed + // child create above, so the umbrella never lands on a decomposition + // whose section write failed, and a retry converges; a body whose + // markers a human edit broke is refused, not guessed at, and a + // rendered body that would exceed GitHub's body cap is refused + // likewise (the write would fail on every retry, and the umbrella + // would never land) — in either case the children, the state and the + // umbrella still land. + // The step's detail carries the created/reused counts plus the + // section outcome (written, unchanged, or refused with the reason), + // so the run record shows what the body write did without re-reading + // the issue. + let detail = `${created.length} created, ${reused.length} reused`; + const parsedSection = parseDecompositionBody(baseBody); + if (!parsedSection.ok) { + detail += `; decomposition section write refused: ${parsedSection.reason}`; + childrenFinalBody = liveUnchangedBody; + } else { + const rendered = renderDecompositionBody(parsedSection, renderDecompositionSection(links)); + if (rendered !== baseBody) { + if (rendered.length > MAX_GITHUB_BODY_CHARS) { + detail += `; decomposition section write refused: body would exceed ${MAX_GITHUB_BODY_CHARS} characters`; + childrenFinalBody = liveUnchangedBody; + } else { + await github.updateTitleAndBody(repoFullName, issueNumber, { body: rendered }); + childrenFinalBody = rendered; + detail += "; decomposition section written"; + } + } else { + childrenFinalBody = liveUnchangedBody; + detail += "; decomposition section unchanged"; + } + } + // The umbrella lands last, now that every child exists or is reused, + // the decomposition state is recorded and the section is written or + // refused; a failed create, a failed state write or a failed section + // write throws before this line, so a partial decomposition never // lands the umbrella. await github.addLabel(repoFullName, issueNumber, UMBRELLA_LABEL); - return { children: { created, reused }, detail: `${created.length} created, ${reused.length} reused` }; + return { children: { created, reused }, detail }; } catch (err) { // Carry what completed so far so the failed step record (and a retry) // can see the children that did land. @@ -970,6 +1112,19 @@ export async function applyGroomingMutations( const outcome: ApplyOutcome = failure ? (wrote ? "partial" : "failed") : wrote ? "applied" : "noop"; await persist(failure ? (wrote ? "partial" : "failed") : "applied"); + // The real post-apply body, for the freshness baseline (run.ts): the + // children step's section write is the last word on the body, so what it + // left behind wins whenever it was reached (even if the step then failed at + // the umbrella — the write itself landed); otherwise the content step's + // write; otherwise null (nothing written, the baseline falls back to the + // snapshot). + const finalBody = + childrenFinalBody !== null + ? childrenFinalBody + : landed(steps.content) && diff.body !== null + ? diff.body + : null; + return { outcome, steps, @@ -978,7 +1133,7 @@ export async function applyGroomingMutations( labels, children: appliedChildren, title: landed(steps.content) && diff.title !== null ? diff.title : null, - body: landed(steps.content) && diff.body !== null ? diff.body : null, + body: finalBody, closed, failure, }; diff --git a/src/lib/groomer/mutation-validator.test.ts b/src/lib/groomer/mutation-validator.test.ts index edc55a88..dd719d92 100644 --- a/src/lib/groomer/mutation-validator.test.ts +++ b/src/lib/groomer/mutation-validator.test.ts @@ -2,8 +2,9 @@ import { describe, expect, it, vi } from "vitest"; import type { GroomingEvidenceSnapshot } from "./evidence-snapshot"; import { buildEvidenceCatalog } from "./plan-evidence"; import { collectPinnedReadContent } from "./close-grounding"; -import { validateGroomingPlan, type GroomingPlan, type GroomingPlanDraft } from "./plan"; +import { validateGroomingPlan, type ChildBrief, type GroomingPlan, type GroomingPlanDraft } from "./plan"; import { + childBriefCompletenessGaps, evaluateClosePolicy, evaluateDecompositionPolicy, evaluateReadyPolicy, @@ -129,14 +130,14 @@ function decomposeDraft(): GroomingPlanDraft { { title: "Child issue A", problem: "Child A problem", - designDecision: null, - verifiedCurrentBehavior: null, - relevantPaths: [], + designDecision: "No design choice; follow the existing returnTo handling", + verifiedCurrentBehavior: "redirectAfterLogin drops session.returnTo", + relevantPaths: ["src/auth/login.ts"], inScope: ["A"], - outOfScope: [], - dependencies: [], + outOfScope: ["B"], + dependencies: ["child B: the migration"], acceptanceCriteria: ["A works"], - tests: [], + tests: ["reset-then-login test"], }, ], }, @@ -149,6 +150,17 @@ function planFor(draft: GroomingPlanDraft, snap = snapshot()): GroomingPlan { return result.plan!; } +/** The plan with its first child brief overridden, built as if valid. */ +function withChildBrief(base: GroomingPlan, brief: Partial<ChildBrief>): GroomingPlan { + return { + ...base, + decomposition: { + ...base.decomposition, + childBriefs: base.decomposition.childBriefs.map((b, i) => (i === 0 ? { ...b, ...brief } : b)), + }, + }; +} + const WINDOW_START = new Date("2026-09-26T00:00:00.000Z"); // Fixed clock for the search-index grace window tests (#1116). const NOW = new Date("2026-09-26T02:00:00.000Z"); @@ -537,7 +549,7 @@ describe("evaluateReadyPolicy", () => { describe("evaluateDecompositionPolicy", () => { const plan = planFor(decomposeDraft()); - it("allows a decisive decomposition with no close and no material uncertainty", () => { + it("allows a complete bounded child brief with no close and no material uncertainty", () => { expect(evaluateDecompositionPolicy(plan)).toEqual([]); }); @@ -578,4 +590,104 @@ describe("evaluateDecompositionPolicy", () => { }; expect(evaluateDecompositionPolicy(forged)).toEqual([]); }); + + const fieldCases: Array<[string, Partial<ChildBrief>, string]> = [ + ["problem", { problem: " " }, "problem"], + ["designDecision", { designDecision: null }, "designDecision"], + ["verifiedCurrentBehavior", { verifiedCurrentBehavior: null }, "verifiedCurrentBehavior"], + ["relevantPaths", { relevantPaths: [] }, "relevantPaths"], + ["inScope", { inScope: [] }, "inScope"], + ["outOfScope", { outOfScope: [] }, "outOfScope"], + ["acceptanceCriteria", { acceptanceCriteria: [] }, "acceptanceCriteria"], + ["tests", { tests: [] }, "tests"], + ]; + + it.each(fieldCases)("rejects a child brief missing %s", (_field, patch, missing) => { + const forged = withChildBrief(plan, patch); + expect(evaluateDecompositionPolicy(forged)).toEqual([ + `child brief[0] is not a complete bounded implementation brief (missing: ${missing})`, + ]); + }); + + it("does not treat empty dependencies as a gap", () => { + const forged = withChildBrief(plan, { dependencies: [] }); + expect(evaluateDecompositionPolicy(forged)).toEqual([]); + }); + + it("treats whitespace-only entries as absent", () => { + const forged = withChildBrief(plan, { + designDecision: " ", + verifiedCurrentBehavior: " ", + relevantPaths: ["\t"], + inScope: [" "], + outOfScope: ["\n"], + acceptanceCriteria: [" "], + tests: [" "], + }); + expect(evaluateDecompositionPolicy(forged)).toEqual([ + "child brief[0] is not a complete bounded implementation brief (missing: designDecision, verifiedCurrentBehavior, relevantPaths, inScope, outOfScope, acceptanceCriteria, tests)", + ]); + }); + + it("reports every incomplete child in its own reason, in brief order", () => { + const full = plan.decomposition.childBriefs[0]; + const forged: GroomingPlan = { + ...plan, + decomposition: { + ...plan.decomposition, + childBriefs: [ + { ...full, designDecision: null, tests: [] }, + { ...full, verifiedCurrentBehavior: null, outOfScope: [] }, + ], + }, + }; + expect(evaluateDecompositionPolicy(forged)).toEqual([ + "child brief[0] is not a complete bounded implementation brief (missing: designDecision, tests)", + "child brief[1] is not a complete bounded implementation brief (missing: verifiedCurrentBehavior, outOfScope)", + ]); + }); +}); + +describe("childBriefCompletenessGaps", () => { + const full: ChildBrief = { + title: "Child", + problem: "Child problem", + designDecision: "follow the existing pattern", + verifiedCurrentBehavior: "redirectAfterLogin drops session.returnTo", + relevantPaths: ["src/auth/login.ts"], + inScope: ["A"], + outOfScope: ["B"], + dependencies: [], + acceptanceCriteria: ["A works"], + tests: ["reset-then-login test"], + }; + + it("lists no gaps for a complete brief, whatever its dependencies are", () => { + expect(childBriefCompletenessGaps(full)).toEqual([]); + }); + + it("lists the missing fields in field order and never reports dependencies", () => { + const brief: ChildBrief = { + ...full, + problem: " ", + designDecision: null, + verifiedCurrentBehavior: null, + relevantPaths: [], + inScope: [], + outOfScope: [], + dependencies: [], + acceptanceCriteria: [], + tests: [], + }; + expect(childBriefCompletenessGaps(brief)).toEqual([ + "problem", + "designDecision", + "verifiedCurrentBehavior", + "relevantPaths", + "inScope", + "outOfScope", + "acceptanceCriteria", + "tests", + ]); + }); }); diff --git a/src/lib/groomer/mutation-validator.ts b/src/lib/groomer/mutation-validator.ts index a7406555..65ec4791 100644 --- a/src/lib/groomer/mutation-validator.ts +++ b/src/lib/groomer/mutation-validator.ts @@ -25,7 +25,7 @@ import { type ExplorationToolCallLike, type NegativeSearchRecheckProbe, } from "./freshness"; -import { evaluateReadiness, type GroomingPlan } from "./plan"; +import { evaluateReadiness, type ChildBrief, type GroomingPlan } from "./plan"; import type { EvidenceCatalog } from "./plan-evidence"; import { evaluateCloseGrounding } from "./close-grounding"; @@ -485,6 +485,33 @@ export function evaluateReadyPolicy(plan: GroomingPlan, catalog: EvidenceCatalog return reasons; } +/** + * The child-brief fields that are missing from this brief, in field order + * (dispatch#1066). A field counts as present when it is non-blank after + * trimming; a list field needs at least one entry that trims non-empty + * (whitespace-only entries do not count). `dependencies` is deliberately + * never reported: a child may legitimately depend on nothing. + */ +export function childBriefCompletenessGaps(brief: ChildBrief): string[] { + const gaps: string[] = []; + if (brief.problem.trim().length === 0) gaps.push("problem"); + if (brief.designDecision === null || brief.designDecision.trim().length === 0) gaps.push("designDecision"); + if (brief.verifiedCurrentBehavior === null || brief.verifiedCurrentBehavior.trim().length === 0) { + gaps.push("verifiedCurrentBehavior"); + } + const lists: Array<[string, string[]]> = [ + ["relevantPaths", brief.relevantPaths], + ["inScope", brief.inScope], + ["outOfScope", brief.outOfScope], + ["acceptanceCriteria", brief.acceptanceCriteria], + ["tests", brief.tests], + ]; + for (const [name, entries] of lists) { + if (!entries.some((entry) => entry.trim().length > 0)) gaps.push(name); + } + return gaps; +} + /** * Decomposition re-checked at apply time (dispatch#1066). Returns every * reason the plan's children may not be created; empty means they may. @@ -496,6 +523,26 @@ export function evaluateReadyPolicy(plan: GroomingPlan, catalog: EvidenceCatalog * - the verdict confidence is not low; * - no material uncertainty of any kind remains. * The apply preconditions separately guarantee the issue is still open. + * + * Every child brief must also be a COMPLETE bounded implementation brief, + * because each one becomes a real child GitHub issue that a fresh worker + * has to implement with no other context: + * - `problem` is non-blank (the plan parser already guarantees this; it is + * enforced again so a forged plan cannot slip past); + * - `designDecision` is a non-blank string; null is rejected. Even the + * outcome "no design choice, follow existing pattern X" must be stated, + * so the worker knows nothing is left to decide; + * - `verifiedCurrentBehavior` is a non-blank string; null is rejected — + * the brief must carry the current behavior the parent's analysis + * verified; + * - `relevantPaths`, `inScope`, `outOfScope`, `acceptanceCriteria` and + * `tests` each contain at least one entry whose trimmed value is + * non-empty (whitespace-only entries do not count as present); + * - `dependencies` may legitimately be empty: a child may depend on + * nothing, so it is never a reason. + * Each incomplete brief is a reason of its own naming its index and + * missing fields (see childBriefCompletenessGaps), so the run records + * exactly which children and which fields are short. */ export function evaluateDecompositionPolicy(plan: GroomingPlan): string[] { const reasons: string[] = []; @@ -508,5 +555,11 @@ export function evaluateDecompositionPolicy(plan: GroomingPlan): string[] { plan.verdict.uncertainties.forEach((u, i) => { if (u.material) reasons.push(`material uncertainty remains (verdict.uncertainties[${i}]): ${u.question}`); }); + plan.decomposition.childBriefs.forEach((brief, i) => { + const gaps = childBriefCompletenessGaps(brief); + if (gaps.length > 0) { + reasons.push(`child brief[${i}] is not a complete bounded implementation brief (missing: ${gaps.join(", ")})`); + } + }); return reasons; } diff --git a/src/lib/groomer/prompts/system-prompt.test.ts b/src/lib/groomer/prompts/system-prompt.test.ts index 5e5a46e3..9de4d303 100644 --- a/src/lib/groomer/prompts/system-prompt.test.ts +++ b/src/lib/groomer/prompts/system-prompt.test.ts @@ -156,6 +156,16 @@ describe("buildGroomerSystemPrompt", () => { expect(prompt).toContain("start as `status/backlog`"); }); + it("makes every child brief field required, matching the apply-time completeness gate (dispatch#1066)", () => { + const prompt = buildGroomerSystemPrompt(baseParams); + expect(prompt).toContain("every field is required and non-blank"); + expect(prompt).toContain("every list except dependencies must hold at least one entry"); + expect(prompt).toContain("dependencies is the only one that may be empty"); + expect(prompt).toContain("designDecision and verifiedCurrentBehavior must not be null"); + expect(prompt).toContain("no design choice; follow the existing pattern"); + expect(prompt).toContain("withholds the whole split"); + }); + it("lets unknowns be recorded instead of guessed", () => { const prompt = buildGroomerSystemPrompt(baseParams); expect(prompt).toContain("Unknown is an answer."); diff --git a/src/lib/groomer/prompts/system-prompt.ts b/src/lib/groomer/prompts/system-prompt.ts index 8de84e80..3c2e7d81 100644 --- a/src/lib/groomer/prompts/system-prompt.ts +++ b/src/lib/groomer/prompts/system-prompt.ts @@ -65,7 +65,7 @@ Return ONLY valid JSON: a grooming plan with this shape (the response schema set "decomposition": { "required": false, "reason": null, "childBriefs": [] }, "relatedWork": [] } -Use null for implementationBrief when you cannot write one honestly (for example needs_info or design work). "close" is null or { "reason": "already_done|duplicate|superseded", "rationale": "...", "evidenceRefs": [...], "criteria": [{ "criterion": "...", "evidenceRef": "repo:path/you/read.ts", "excerpt": "..." }] } (criteria is [] unless the reason is already_done). relatedWork entries are { "ref": "github:issue:owner/repo#12", "relation": "duplicate_of|superseded_by|related", "note": "..." }: the ref is always a "github:" id from the evidence list. childBriefs entries are { "title": "...", "problem": "what the child must fix, in repo terms", "designDecision": "the settled design decision the child implements" or null, "verifiedCurrentBehavior": "what the parent's analysis verified the code does today" or null, "relevantPaths": ["repo:path/the/child/touches.ts"], "inScope": ["what the child may do"], "outOfScope": ["what the child must not do"], "dependencies": ["sibling or external work this child depends on"], "acceptanceCriteria": ["deterministic, observable result"], "tests": ["test to add or update"] }. +Use null for implementationBrief when you cannot write one honestly (for example needs_info or design work). "close" is null or { "reason": "already_done|duplicate|superseded", "rationale": "...", "evidenceRefs": [...], "criteria": [{ "criterion": "...", "evidenceRef": "repo:path/you/read.ts", "excerpt": "..." }] } (criteria is [] unless the reason is already_done). relatedWork entries are { "ref": "github:issue:owner/repo#12", "relation": "duplicate_of|superseded_by|related", "note": "..." }: the ref is always a "github:" id from the evidence list. childBriefs entries are { "title": "...", "problem": "what the child must fix, in repo terms", "designDecision": "the settled design decision the child implements", "verifiedCurrentBehavior": "what the parent's analysis verified the code does today", "relevantPaths": ["repo:path/the/child/touches.ts"], "inScope": ["what the child may do"], "outOfScope": ["what the child must not do"], "dependencies": ["sibling or external work this child depends on"], "acceptanceCriteria": ["deterministic, observable result"], "tests": ["test to add or update"] }; every field is required and non-blank, and every list except dependencies must hold at least one entry — dependencies is the only one that may be empty. designDecision and verifiedCurrentBehavior must not be null: state the settled decision even when it is simply "no design choice; follow the existing pattern", and ground the current behavior in what the code was verified to do. A child brief missing any of these withholds the whole split, so write every child complete. Evidence rules: - The user message ends with "Evidence you can cite": the only valid evidence ids for this run. Cite ids exactly as listed wherever the plan asks for evidenceRefs or a ref. Never invent an id; an unknown id rejects the whole plan. diff --git a/src/lib/groomer/run.test.ts b/src/lib/groomer/run.test.ts index a80e5ecb..704bac5a 100644 --- a/src/lib/groomer/run.test.ts +++ b/src/lib/groomer/run.test.ts @@ -2637,14 +2637,16 @@ Investigate session handling in auth module.`; const childBrief = (n: number) => ({ title: `Bounded child ${n}`, problem: `Child ${n} problem, as its own bounded change.`, - designDecision: null, - verifiedCurrentBehavior: null, - relevantPaths: [], + // Every field the apply-time decomposition policy requires a complete + // bounded brief to carry: an incomplete brief withholds the split. + designDecision: `Child ${n} follows the existing pattern; no design choice is left open.`, + verifiedCurrentBehavior: `Child ${n} is not implemented yet; login.ts drops the return URL.`, + relevantPaths: ["src/auth/login.ts"], inScope: [`child ${n}`], - outOfScope: [], + outOfScope: [`Child ${n} does not touch authentication`], dependencies: [], acceptanceCriteria: [`Child ${n} works end to end`], - tests: [], + tests: [`Child ${n} is covered by an automated test`], }); /** A non-ready (backlog) verdict that splits the issue into `count` bounded children. */ @@ -2684,6 +2686,18 @@ Investigate session handling in auth module.`; ); // The run records the created child links, in brief order. expect(result!.appliedMutations!.childrenCreated).toHaveLength(2); + // The children step writes a managed decomposition section into the + // parent body: the marker pair plus one `- #<n>: <url>` line per + // created child. The content step wrote nothing (no title/body in the + // plan), so this is the only body write. + expect(mocks.updateIssueTitleAndBody).toHaveBeenCalledTimes(1); + const written = mocks.updateIssueTitleAndBody.mock.calls[0][2] as { body?: string | null }; + expect(written.body).toContain("<!-- dispatch-groomer:decomposition:start -->"); + expect(written.body).toContain("<!-- dispatch-groomer:decomposition:end -->"); + const created = result!.appliedMutations!.childrenCreated as { number: number; url: string }[]; + for (const child of created) { + expect(written.body).toContain(`- #${child.number}: ${child.url}`); + } }); it("an exact retry is a replay: it creates no new child and re-surfaces the created links", async () => { @@ -2773,6 +2787,43 @@ Investigate session handling in auth module.`; expect(mocks.addIssueLabel).not.toHaveBeenCalled(); expect(mocks.prisma.groomingChildClaim.create).not.toHaveBeenCalled(); }); + + it("withholds the decomposition end to end when a child brief is not a complete bounded implementation brief", async () => { + // Everything complete except the settled design decision: a child that + // leaves a design choice open is not bounded, so the split is withheld. + mocks.callGroomerLLM.mockResolvedValue( + notReadyDraft("backlog", { + decomposition: { + required: true, + reason: "split it", + childBriefs: [{ ...childBrief(1), designDecision: null }], + }, + }), + ); + const result = await runHostedGroomer(); + + // The plan still lands (as backlog); only the decomposition is withheld. + expect(result!.appliedMutations).toMatchObject({ outcome: "applied" }); + // The run records exactly which child and which field is short. + const withheld = result!.appliedMutations!.withheld as { decomposition: string[] }; + expect(withheld.decomposition).toEqual([ + "child brief[0] is not a complete bounded implementation brief (missing: designDecision)", + ]); + // No child is created, no umbrella lands, and the parent is not decomposed. + expect(mocks.createIssue).not.toHaveBeenCalled(); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(mocks.prisma.groomingChildClaim.create).not.toHaveBeenCalled(); + expect( + (mocks.prisma.issue.update.mock.calls.map((c) => c[0].data as Record<string, unknown>) ?? []).some( + (data) => data.decomposed === true, + ), + ).toBe(false); + expect( + (mocks.prisma.auditLog.create.mock.calls.map((c) => c[0].data as Record<string, unknown>) ?? []).some( + (data) => data.action === "issue_decomposed", + ), + ).toBe(false); + }); }); it("strips a status the groomer does not own, so the derived status is the only one", async () => {