From 33f3d93fc765505e52d1816f332364cf0f46ee9b Mon Sep 17 00:00:00 2001 From: debuggingfuture Date: Tue, 18 Aug 2026 05:03:19 +0800 Subject: [PATCH 1/2] =?UTF-8?q?feat(org-spec-audit):=20one=20question,=20o?= =?UTF-8?q?ne=20issue=20=E2=80=94=20the=20ledger=20becomes=20the=20artifac?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep filed a dated `.md` in a draft PR every morning. Two sweeps two days apart asked three of the same questions twice — two of them under a byte-identical `maintenance-key` (org#147, org#148). An identical key that re-proposes rules out the obvious cause and leaves the real one: **the ledger could only remember "no", never "asked".** Both suppression rules key off a TERMINATED proposal — declined in the ledger, or a PR closed unmerged — and the first PR was neither. It was open, unanswered, in the queue, which the design read as no signal at all. The unstable key was the second cause and would have fixed one duplicate of the three. So the unit is the question and the artifact is a GitHub issue. open = asked (write nothing), closed = decided (never re-file, never reopen), absent = new. - `createIssue` on the github-app surface, `openIssue` on `GithubService`. It FAILS rather than degrading to a logged no-op, unlike `openDraftPullRequest`: a PR write that no-ops leaves its content in git and its branch idempotent, while a no-op here drops the only copy of the question. - `listIssues` gains `strict`, which throws instead of returning a list the page ceiling cut short. A dedup read cannot use a partial list — it answers "not filed" for everything it never reached — and a bare array cannot distinguish "ended on a short page" from "ended on the ceiling". - `IssueRef.closedAt` — the column `pulls` lacks and `issues` has. - Reconciliation: exact key match first, then ONE model call over the residue asking "one of these, or new?". Every answer is checked against the keys actually read from the control repo, so a hallucinated match files the question rather than silently burying it. The prompt now asks for noun-phrase keys (`pipeline-dags`, not `adopt-pipeline-dags`). - The dedup read happens BEFORE the sweep and fails CLOSED, inverting the suppression primitive's posture: an unreadable ledger cost one duplicate PR, an unreadable issue set costs a duplicate of every question at once. The declines ledger still fails open — its failure costs one issue a human closes, and that close then suppresses it for good. - `checkSuppression`'s `headBranchPrefix` is optional; omitted, the PR-history read is skipped entirely rather than made and ignored. - The notice is a delta: filed today with links, standing open count, what the per-sweep cap held. `max-new-questions` bounds the writes; the old per-group rendering cap is gone. - The github fake appends every `openIssue` to the list `issues` reads back, so "the second sweep sees the first sweep's issue" is testable at all. Config: `questions-dir` retires for `questions-label`, `lane-label-prefix` and `max-new-questions`. A label that is set-and-unusable fails the run instead of falling back — it is both the filter the read applies and the label the write applies, and a comma makes those two different things silently. Design: fractalboxdev/org#150. --- packages/core/src/fakes/github-fake.ts | 58 +- packages/core/src/index.ts | 1 + .../src/primitives/automerge-gate.test.ts | 1 + .../core/src/primitives/suppression.test.ts | 1 + packages/core/src/primitives/suppression.ts | 59 +- packages/core/src/services/github.ts | 58 ++ packages/github-app/src/index.ts | 3 + packages/github-app/src/issues.test.ts | 83 ++ packages/github-app/src/issues.ts | 133 ++- packages/runtime-cf/src/deferred.ts | 5 + packages/runtime-cf/src/github-live.ts | 61 +- runs/org-spec-audit.test.ts | 976 ++++++++++++------ runs/org-spec-audit.ts | 801 ++++++++++---- 13 files changed, 1699 insertions(+), 541 deletions(-) diff --git a/packages/core/src/fakes/github-fake.ts b/packages/core/src/fakes/github-fake.ts index eb4dd4a..64fc369 100644 --- a/packages/core/src/fakes/github-fake.ts +++ b/packages/core/src/fakes/github-fake.ts @@ -50,6 +50,13 @@ export type GithubFakeState = { state: "open" | "closed" | "all"; labels?: readonly string[]; updatedWithinDays?: number; + /** + * Recorded, never simulated — the fake holds one page, so it can never + * truncate. A dedup read's correctness depends on ASKING for `strict`, and + * that is the half a test can pin here; the truncation itself is pinned + * against the wire in `issues.test.ts`. + */ + strict?: boolean; }>; /** Every `actionRuns` call, in order. */ readonly actionRunsCalls: Array<{ @@ -72,6 +79,20 @@ export type GithubFakeState = { readonly pullReviewCalls: PullReviewRequest[]; /** Every `openDraftPullRequest` call, in order. */ readonly openDraftPullRequestCalls: OpenDraftPullRequest[]; + /** + * Every `openIssue` call, in order — and each one also lands in `issues`. + * + * That second half is the point. A run whose whole job is "do not file what I + * already filed" can only be tested if a filed issue is visible to the next + * read, so the fake appends it rather than merely recording the call. Without + * that, every test would pass against a run that dedups against nothing. + */ + readonly openIssueCalls: Array<{ + repo: string; + title: string; + body: string; + labels?: readonly string[]; + }>; /** Every `createRelease` call, in order — lets a test assert a release published. */ readonly createReleaseCalls: CreateRelease[]; /** Every label ADD, in order. */ @@ -133,6 +154,7 @@ export const makeGithubFake = ( readTextFileCalls: [], pullReviewCalls: [], openDraftPullRequestCalls: [], + openIssueCalls: [], createReleaseCalls: [], addIssueLabelsCalls: [], removeIssueLabelCalls: [], @@ -146,9 +168,9 @@ export const makeGithubFake = ( const openedBranches = new Set(); const service: GithubService = { - issues: ({ repo, state: want = "open", labels, updatedWithinDays }) => + issues: ({ repo, state: want = "open", labels, updatedWithinDays, strict }) => Effect.sync(() => { - state.issuesCalls.push({ repo, state: want, labels, updatedWithinDays }); + state.issuesCalls.push({ repo, state: want, labels, updatedWithinDays, strict }); const need = labels === undefined ? undefined : new Set(labels); return state.issues.filter((i) => { if (i.repo !== repo) return false; @@ -164,6 +186,34 @@ export const makeGithubFake = ( }); }), + openIssue: ({ repo, title, body, labels }) => + Effect.sync(() => { + state.openIssueCalls.push({ repo, title, body, labels }); + // Numbered above every issue the fake knows about, in this repo or any + // other, because GitHub's numbering is per repo but a test asserting on + // `#3` should not have it mean two different issues. + const number = state.issues.reduce((max, i) => Math.max(max, i.number), 0) + 1; + const url = `https://github.com/${repo}/issues/${number}`; + state.issues = [ + ...state.issues, + { + repo, + number, + title, + body, + state: "open" as const, + labels: [...(labels ?? [])], + author: "flare-dispatch[bot]", + authorAssociation: "OWNER", + url, + commentCount: 0, + createdAt: now, + updatedAt: now, + }, + ]; + return { number, url }; + }), + // The writes record and mutate the seeded issue, so a test can assert both // "the call was made" and "the state machine advanced". addIssueLabels: ({ repo, issue, labels }) => @@ -195,7 +245,9 @@ export const makeGithubFake = ( Effect.sync(() => { state.closeIssueAsDuplicateCalls.push({ repo, issue, duplicateOf }); state.issues = state.issues.map((i) => - i.repo === repo && i.number === issue ? { ...i, state: "closed" as const } : i, + i.repo === repo && i.number === issue + ? { ...i, state: "closed" as const, closedAt: now } + : i, ); }), diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index a22e4c0..d2f8807 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -153,6 +153,7 @@ export { github, Github, type GithubService, + type IssueCreated, type IssueRef, type PullRequestHistoryRef, type ReadTextFileRequest, diff --git a/packages/core/src/primitives/automerge-gate.test.ts b/packages/core/src/primitives/automerge-gate.test.ts index e010108..13d83c4 100644 --- a/packages/core/src/primitives/automerge-gate.test.ts +++ b/packages/core/src/primitives/automerge-gate.test.ts @@ -372,6 +372,7 @@ describe("evaluateAutomerge — a self-declared run marker is not authorship", ( const githubService = (over: Partial): GithubService => ({ issues: () => Effect.succeed([]), + openIssue: () => Effect.succeed({ number: 1, url: "" }), addIssueLabels: () => Effect.void, removeIssueLabel: () => Effect.void, commentOnIssue: () => Effect.void, diff --git a/packages/core/src/primitives/suppression.test.ts b/packages/core/src/primitives/suppression.test.ts index a78d75f..a7aa387 100644 --- a/packages/core/src/primitives/suppression.test.ts +++ b/packages/core/src/primitives/suppression.test.ts @@ -296,6 +296,7 @@ describe("decideSuppression", () => { /** A `GithubService` with every method stubbed, overridable per test. */ const githubService = (over: Partial): GithubService => ({ issues: () => Effect.succeed([]), + openIssue: () => Effect.succeed({ number: 1, url: "" }), addIssueLabels: () => Effect.void, removeIssueLabel: () => Effect.void, commentOnIssue: () => Effect.void, diff --git a/packages/core/src/primitives/suppression.ts b/packages/core/src/primitives/suppression.ts index 70d3c6a..69a62da 100644 --- a/packages/core/src/primitives/suppression.ts +++ b/packages/core/src/primitives/suppression.ts @@ -412,8 +412,18 @@ export type CheckSuppressionArgs = { readonly ledgerRef?: string; /** The repo prior proposals were opened against — defaults to `ledgerRepo`. */ readonly proposalRepo?: string; - /** The head-branch prefix every proposal of this kind shares. */ - readonly headBranchPrefix: string; + /** + * The head-branch prefix every proposal of this kind shares. + * + * **Omit it for a consumer that opens no PRs**, and the cooldown half is + * skipped entirely — no PR-history read, no spend, no cooldown. That is not a + * degraded mode: a cooldown dated from a closed PR is meaningless where no PR + * exists, and passing a prefix that matches nothing would answer "no prior + * proposals" every tick for a reason no reader could tell apart from "the + * feature is off". The spec-audit sweep is the first such consumer — its + * memory is the issue it filed, and only the ledger half applies here. + */ + readonly headBranchPrefix?: string; /** Now, in epoch ms — passed in so a run's clock is the one that decides. */ readonly nowMs: number; /** Cooldown length — defaults to {@link COOLDOWN_DAYS_DEFAULT}. */ @@ -464,25 +474,32 @@ export const checkSuppression = (args: CheckSuppressionArgs) => ); } - // 2. The cooldowns. Paginate no further back than the cooldown window — - // `updatedAt >= closedAt`, so nothing closed inside it can be missed. - const priorProposals = yield* github - .pullRequestHistory({ - repo: proposalRepo, - headBranchPrefix: args.headBranchPrefix, - state: "all", - updatedWithinDays: cooldownDays, - }) - .pipe( - Effect.catchTag("GitHubApiError", (err) => - Effect.gen(function* () { - const why = `PR history for ${proposalRepo} (${args.headBranchPrefix}*) unreadable (GitHub ${err.status} ${err.reason}) — cooldowns NOT applied this tick`; - degraded.push(why); - yield* io.log("warn", `suppression: ${why}`); - return [] as readonly PullRequestHistoryRef[]; - }), - ), - ); + // 2. The cooldowns — only for a consumer that opens PRs. With no prefix + // there is no PR to have been closed, so the read is skipped rather than + // made and ignored: an empty history would otherwise be recorded as "no + // prior proposals", which reads the same as a working cooldown finding + // nothing. + const headBranchPrefix = args.headBranchPrefix; + const priorProposals = + headBranchPrefix === undefined + ? ([] as readonly PullRequestHistoryRef[]) + : yield* github + .pullRequestHistory({ + repo: proposalRepo, + headBranchPrefix, + state: "all", + updatedWithinDays: cooldownDays, + }) + .pipe( + Effect.catchTag("GitHubApiError", (err) => + Effect.gen(function* () { + const why = `PR history for ${proposalRepo} (${headBranchPrefix}*) unreadable (GitHub ${err.status} ${err.reason}) — cooldowns NOT applied this tick`; + degraded.push(why); + yield* io.log("warn", `suppression: ${why}`); + return [] as readonly PullRequestHistoryRef[]; + }), + ), + ); const verdicts = decideSuppression({ candidates: args.keys, diff --git a/packages/core/src/services/github.ts b/packages/core/src/services/github.ts index ebcc45e..7e3574d 100644 --- a/packages/core/src/services/github.ts +++ b/packages/core/src/services/github.ts @@ -50,6 +50,22 @@ export type IssueRef = { readonly createdAt: number; /** epoch ms. */ readonly updatedAt: number; + /** + * epoch ms when the issue was closed, or `undefined` while it is open. + * + * `updatedAt` cannot stand in: any touch resets it, so a window dated from it + * never expires. This is the column the org store's `pulls` table lacks and + * `issues` has — the whole reason an issue-shaped ledger needs no workaround + * where a PR-shaped one needed a file in git. + */ + readonly closedAt?: number; +}; + +/** The outcome of {@link GithubService.openIssue}. */ +export type IssueCreated = { + readonly number: number; + /** The issue's web URL — what a notice links, so it is never empty on success. */ + readonly url: string; }; /** @@ -394,9 +410,43 @@ export interface GithubService { labels?: readonly string[]; updatedWithinDays?: number; maxPages?: number; + /** + * Fail rather than return a list the page ceiling cut short. + * + * A triage pass wants the default: the 500 most recently updated issues are + * the tick's work and a longer backlog waits. A **deduplication** read + * cannot — it asks "have I filed this already?", and a truncated list says + * "no" for every issue it did not reach, so the caller duplicates whatever + * fell off the end. Set it wherever an absent row is read as a fact. + */ + strict?: boolean; installationId?: number; }) => Effect.Effect; + /** + * Open one issue — the write the spec-audit sweep files an open question with. + * + * **Fails rather than degrading to a logged no-op**, which is the opposite of + * `openDraftPullRequest` below, and the difference is what the artifact is. A + * PR write that no-ops loses nothing: the branch is idempotent and the content + * is a file that still exists in the commit the next tick will re-derive. Here + * the issue *is* the question — there is no file, no branch, and no second + * copy — so a silent no-op drops it, and the loop then has no record that it + * ever had something to ask. + * + * Narrow on purpose: a title, a body, and labels. No assignee, no milestone, + * no template. What bounds it is the caller — the sweep files into one control + * repo resolved from config, and never files a question it did not first fail + * to find among that repo's existing issues. + */ + readonly openIssue: (req: { + repo: string; + title: string; + body: string; + labels?: readonly string[]; + installationId?: number; + }) => Effect.Effect; + /** Add labels to an issue — the state machine's write (§5). */ readonly addIssueLabels: (req: { repo: string; @@ -497,8 +547,16 @@ export const github = { labels?: readonly string[]; updatedWithinDays?: number; maxPages?: number; + strict?: boolean; installationId?: number; }) => Effect.flatMap(Github, (g) => g.issues(opts)), + openIssue: (req: { + repo: string; + title: string; + body: string; + labels?: readonly string[]; + installationId?: number; + }) => Effect.flatMap(Github, (g) => g.openIssue(req)), addIssueLabels: (req: { repo: string; issue: number; diff --git a/packages/github-app/src/index.ts b/packages/github-app/src/index.ts index e0fa053..8ae3bfb 100644 --- a/packages/github-app/src/index.ts +++ b/packages/github-app/src/index.ts @@ -70,6 +70,7 @@ export { export { addIssueLabels, closeIssueAsDuplicate, + createIssue, createIssueComment, listIssues, removeIssueLabel, @@ -77,6 +78,8 @@ export { type AuthorAssociation, type CloseIssueAsDuplicateOptions, type CreateIssueCommentOptions, + type CreateIssueOptions, + type CreateIssueResult, type IssueListItem, type ListIssuesOptions, type RemoveIssueLabelOptions, diff --git a/packages/github-app/src/issues.test.ts b/packages/github-app/src/issues.test.ts index c77209c..c31b5cc 100644 --- a/packages/github-app/src/issues.test.ts +++ b/packages/github-app/src/issues.test.ts @@ -22,6 +22,7 @@ import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; import { addIssueLabels, closeIssueAsDuplicate, + createIssue, createIssueComment, listIssues, removeIssueLabel, @@ -64,6 +65,13 @@ const server = setupServer( await capture(request, "list"); return HttpResponse.json(pages.shift() ?? []); }), + http.post("https://api.github.com/repos/:owner/:repo/issues", async ({ request }) => { + await capture(request, "create"); + return HttpResponse.json( + { number: 41, html_url: "https://github.com/owner/name/issues/41" }, + { status: 201 }, + ); + }), http.post( "https://api.github.com/repos/:owner/:repo/issues/:n/labels", async ({ request, params }) => { @@ -197,6 +205,81 @@ describe("listIssues", () => { ); await expect(listIssues({ ...base })).rejects.toThrow(); }); + + it("reads closed_at, and leaves it empty while the issue is open", async () => { + pages = [[rawIssue({ state: "closed", closed_at: "2026-08-10T09:00:00Z" }), rawIssue()]]; + const out = await listIssues({ ...base, state: "all" }); + expect(out[0]?.closedAt).toBe("2026-08-10T09:00:00Z"); + expect(out[1]?.closedAt).toBe(""); + }); + + // The dedup read's whole job is answering "have I already filed this?", and a + // list the ceiling cut short answers "no" for everything it never reached. + it("throws under `strict` when the page ceiling cut the list short", async () => { + const full = Array.from({ length: 100 }, (_, i) => rawIssue({ number: i + 1 })); + pages = [full, full]; + await expect(listIssues({ ...base, maxPages: 1, strict: true })).rejects.toThrow( + /page ceiling/, + ); + }); + + it("does not throw under `strict` when a short page ended the list", async () => { + pages = [[rawIssue()]]; + await expect(listIssues({ ...base, maxPages: 1, strict: true })).resolves.toHaveLength(1); + }); + + // The boundary case: a full last page that happens to be the last page. GitHub + // cannot say so, and neither can we — a strict caller is told to look again + // rather than shown a list nobody can vouch for. + it("throws under `strict` on an exactly-full last page", async () => { + pages = [Array.from({ length: 100 }, (_, i) => rawIssue({ number: i + 1 })), []]; + await expect(listIssues({ ...base, maxPages: 1, strict: true })).rejects.toThrow(); + }); +}); + +describe("createIssue", () => { + it("POSTs title, body and labels to /issues and returns the number and url", async () => { + const out = await createIssue({ + ...base, + title: "Is ADR-0002 accepted?", + body: "maintenance-key: org-spec-audit/memory-capability", + labels: ["maintenance:open-question", "question:decide"], + }); + + expect(out).toEqual({ number: 41, url: "https://github.com/owner/name/issues/41" }); + expect(calls[0]).toMatchObject({ method: "POST", path: "create" }); + expect(calls[0]?.body).toEqual({ + title: "Is ADR-0002 accepted?", + body: "maintenance-key: org-spec-audit/memory-capability", + labels: ["maintenance:open-question", "question:decide"], + }); + expect(calls[0]?.authorization).toBe("Bearer inst-token-abc"); + }); + + it("omits `labels` entirely when none were given", async () => { + await createIssue({ ...base, title: "t", body: "b" }); + expect(calls[0]?.body).toEqual({ title: "t", body: "b" }); + }); + + it("fails when the create returns no issue number — #0 links nowhere", async () => { + server.use( + http.post("https://api.github.com/repos/:owner/:repo/issues", () => + HttpResponse.json({ html_url: "https://github.com/owner/name/issues/?" }, { status: 201 }), + ), + ); + await expect(createIssue({ ...base, title: "t", body: "b" })).rejects.toThrow( + /no issue number/, + ); + }); + + it("surfaces a non-2xx", async () => { + server.use( + http.post("https://api.github.com/repos/:owner/:repo/issues", () => + HttpResponse.json({ message: "Resource not accessible by integration" }, { status: 403 }), + ), + ); + await expect(createIssue({ ...base, title: "t", body: "b" })).rejects.toThrow(); + }); }); describe("addIssueLabels", () => { diff --git a/packages/github-app/src/issues.ts b/packages/github-app/src/issues.ts index 01eb7f0..288d576 100644 --- a/packages/github-app/src/issues.ts +++ b/packages/github-app/src/issues.ts @@ -1,6 +1,7 @@ -// @fractalboxdev/flare-dispatch-github-app — the issue surface: one read, four writes. +// @fractalboxdev/flare-dispatch-github-app — the issue surface: one read, five writes. // // GET /repos/{o}/{r}/issues — list, carrying labels +// POST /repos/{o}/{r}/issues — CREATE one issue // POST /repos/{o}/{r}/issues/{n}/labels — add labels // DELETE /repos/{o}/{r}/issues/{n}/labels/{name} — remove one label // POST /repos/{o}/{r}/issues/{n}/comments — comment @@ -9,9 +10,25 @@ // // Why these and not a general issue API: `process/content/maintenance-loop.md` // §5 says **labels are the state machine**, and a classifier whose verdict -// cannot be recorded is spend with no artifact. This is exactly the set §5's -// issue machine names and nothing else — no create, no reopen, no edit, no -// milestone. A capability nobody needs is a capability waiting to be mis-wired. +// cannot be recorded is spend with no artifact. Still no reopen, no edit, no +// milestone, no assignee-clearing — a capability nobody needs is a capability +// waiting to be mis-wired. +// +// --- Why `createIssue` exists now, having deliberately not existed ----------- +// +// This module shipped with "no create" as a stated rule, because the triage desk +// only ever *routes* issues other people open. The spec-audit sweep changed what +// an issue is for: it files **one issue per open question**, and that issue is +// the loop's ledger entry rather than a report of anything. The state a decline +// needs — closed, and still closed a year later — is then the artifact itself, +// where the PR-shaped version needed a key, a second file in git, and a cooldown +// dated from a column the org store does not carry. +// +// The narrowness survives the addition. `createIssue` takes a title, a body and +// labels; there is no assignee, no milestone, no template, and no way to spell +// "create in a repo I was not given". What bounds it is the caller: the sweep +// files into ONE control repo it read from config, and it files nothing it did +// not first fail to find in that repo's existing issues. // // --- `closeIssueAsDuplicate` is not `closeIssue` ------------------------------ // @@ -42,6 +59,7 @@ // Provider-neutral plain `async`, no Effect — the Layer (`makeGithubLive`) wraps // these, the same split every other module in this package keeps. +import { GithubApiError } from "./errors"; import { assertOk, API_BASE_DEFAULT, ghHeaders, resolveClient, splitRepo } from "./http"; /** How GitHub describes the author's standing in the repo. */ @@ -69,6 +87,16 @@ export type IssueListItem = { readonly authorAssociation: AuthorAssociation; readonly createdAt: string; readonly updatedAt: string; + /** + * When the issue was closed, or `""` while it is open. + * + * Read because a cooldown needs it and `updatedAt` cannot substitute — any + * touch resets that one, so a window computed from it never expires. The org + * store's `pulls` table lacks this column entirely, which is why suppression + * reads GitHub rather than the store; `issues` has it, so an issue-shaped + * ledger needs no workaround. + */ + readonly closedAt: string; readonly url: string; readonly commentCount: number; }; @@ -95,6 +123,8 @@ type RawIssue = { readonly author_association?: unknown; readonly created_at?: unknown; readonly updated_at?: unknown; + /** `null` while the issue is open. */ + readonly closed_at?: unknown; readonly html_url?: unknown; readonly comments?: unknown; /** Present iff this "issue" is really a pull request. */ @@ -128,6 +158,7 @@ const normalizeIssue = (raw: RawIssue): IssueListItem | undefined => { authorAssociation: (ASSOCIATIONS.has(association) ? association : "NONE") as AuthorAssociation, createdAt: str(raw.created_at), updatedAt: str(raw.updated_at), + closedAt: str(raw.closed_at), url: str(raw.html_url), commentCount: typeof raw.comments === "number" ? raw.comments : 0, }; @@ -152,6 +183,23 @@ export type ListIssuesOptions = IssueCallBase & { readonly updatedSince?: number; /** Page ceiling — bounds a sweep over a large backlog. Default 5 (500 issues). */ readonly maxPages?: number; + /** + * Throw instead of returning a list the page ceiling cut short. + * + * A triage pass wants the default: reading the 500 most recently updated + * issues is the bound, and a backlog past it is simply not this tick's work. + * A **deduplication** read cannot live with that. It asks "have I already + * filed this question?", and a truncated list answers "no" for everything it + * did not reach — so the caller files a duplicate of every question that fell + * off the last page, which is the exact failure the read exists to prevent. + * + * Two returns are otherwise indistinguishable: the loop stops on a short page + * (the list is complete) or on the ceiling with a full page (there is more), + * and a bare array cannot tell them apart. Rather than return a shape every + * caller must then remember to check, the caller that cannot tolerate a + * partial read asks for `strict` and gets an error it already handles. + */ + readonly strict?: boolean; }; const PER_PAGE = 100; @@ -163,13 +211,16 @@ const MAX_PAGES_DEFAULT = 5; * Paginates to `maxPages` and stops early on a short page. The ceiling is a * bound on a scheduled sweep, not a correctness property: a backlog past it is * simply not read this tick, which is visible rather than silent because the - * caller knows what it asked for. + * caller knows what it asked for. Callers that need the *whole* set — see + * `strict` — get an error instead of a short list. */ export const listIssues = async (opts: ListIssuesOptions): Promise => { const { apiBase, doFetch } = resolveClient(opts); const { owner, name } = splitRepo(opts.repo); const maxPages = opts.maxPages ?? MAX_PAGES_DEFAULT; const out: IssueListItem[] = []; + /** Did the pagination end because GitHub ran out, or because we did? */ + let exhausted = false; for (let page = 1; page <= maxPages; page++) { const url = new URL(`${apiBase ?? API_BASE_DEFAULT}/repos/${owner}/${name}/issues`); @@ -188,16 +239,84 @@ export const listIssues = async (opts: ListIssuesOptions): Promise => { + const { apiBase, doFetch } = resolveClient(opts); + const { owner, name } = splitRepo(opts.repo); + const res = await doFetch(`${apiBase ?? API_BASE_DEFAULT}/repos/${owner}/${name}/issues`, { + method: "POST", + headers: ghHeaders(opts.token, { json: true }), + body: JSON.stringify({ + title: opts.title, + body: opts.body, + ...(opts.labels !== undefined && opts.labels.length > 0 ? { labels: opts.labels } : {}), + }), + }); + await assertOk(res, `issue create failed for ${opts.repo}`); + const created = (await res.json()) as { readonly number?: unknown; readonly html_url?: unknown }; + if (typeof created.number !== "number") { + throw new GithubApiError( + `issue create for ${opts.repo} returned no issue number`, + res.status, + "", + ); + } + return { number: created.number, url: str(created.html_url) }; +}; + /** The number identifying one issue, on every write below. */ type IssueTarget = IssueCallBase & { readonly issue: number }; diff --git a/packages/runtime-cf/src/deferred.ts b/packages/runtime-cf/src/deferred.ts index d20756e..3b42912 100644 --- a/packages/runtime-cf/src/deferred.ts +++ b/packages/runtime-cf/src/deferred.ts @@ -126,6 +126,11 @@ export const GithubDeferred: Layer.Layer = Layer.succeed( // `issues` is the same class: an empty list reads as "nothing to triage", // which a scheduled run would act on by reporting a clean estate. issues: () => Effect.fail(new GitHubApiError({ status: 0, reason: "unauthorized" })), + // `openIssue` is the one WRITE in the fail class, because the issue it opens + // is the artifact and not a report of one. A logged skip here discards the + // question with nothing left holding it — no branch, no file, no second + // copy — and the run would report a clean sweep having asked nothing. + openIssue: () => Effect.fail(new GitHubApiError({ status: 0, reason: "unauthorized" })), // The state-machine writes degrade to a logged no-op, like `pullReview`. addIssueLabels: ({ repo, issue }) => Effect.logInfo( diff --git a/packages/runtime-cf/src/github-live.ts b/packages/runtime-cf/src/github-live.ts index 38217de..5c9807a 100644 --- a/packages/runtime-cf/src/github-live.ts +++ b/packages/runtime-cf/src/github-live.ts @@ -27,6 +27,7 @@ import { addIssueLabels, closeIssueAsDuplicate, + createIssue, createIssueComment, createPullReview, createRelease, @@ -46,6 +47,7 @@ import { Github, GitHubApiError, type GithubService, + type IssueCreated, type IssueRef, type PullRequestHistoryRef, type ReleaseResult, @@ -267,7 +269,7 @@ export const makeGithubLive = (config: GithubLiveConfig | undefined): Layer.Laye ); }), - issues: ({ repo, state, labels, updatedWithinDays, maxPages, installationId }) => + issues: ({ repo, state, labels, updatedWithinDays, maxPages, strict, installationId }) => Effect.gen(function* () { if (config === undefined) return yield* readNeedsCredentials(); const token = yield* mintToken(config, repo, installationId); @@ -281,22 +283,51 @@ export const makeGithubLive = (config: GithubLiveConfig | undefined): Layer.Laye ? { updatedSince: Date.now() - updatedWithinDays * 86_400_000 } : {}), ...(maxPages !== undefined ? { maxPages } : {}), + ...(strict === true ? { strict: true } : {}), + }), + ); + return raw.map((i): IssueRef => { + // `closed_at` is absent on an open issue and unparseable on a malformed + // one; both leave the field off rather than dating a close at the + // epoch, which would read as "closed in 1970" to anything computing a + // window from it. + const closedAt = i.closedAt === "" ? Number.NaN : Date.parse(i.closedAt); + return { + repo, + number: i.number, + title: i.title, + body: i.body, + state: i.state, + labels: i.labels, + author: i.author, + authorAssociation: i.authorAssociation, + url: i.url, + commentCount: i.commentCount, + createdAt: Date.parse(i.createdAt) || 0, + updatedAt: Date.parse(i.updatedAt) || 0, + ...(Number.isFinite(closedAt) ? { closedAt } : {}), + }; + }); + }), + + // `openIssue` is the exception to the paragraph below: it FAILS without + // credentials rather than logging a skip. The state-machine writes annotate + // an issue that exists whether or not the write lands, and the PR write + // leaves its content in git — but this one *is* the artifact, so a no-op + // discards the question and leaves the loop with no record it had one. + openIssue: ({ repo, title, body, labels, installationId }) => + Effect.gen(function* () { + if (config === undefined) return yield* readNeedsCredentials(); + const token = yield* mintToken(config, repo, installationId); + return yield* ghCall(() => + createIssue({ + token, + repo, + title, + body, + ...(labels !== undefined && labels.length > 0 ? { labels } : {}), }), ); - return raw.map((i): IssueRef => ({ - repo, - number: i.number, - title: i.title, - body: i.body, - state: i.state, - labels: i.labels, - author: i.author, - authorAssociation: i.authorAssociation, - url: i.url, - commentCount: i.commentCount, - createdAt: Date.parse(i.createdAt) || 0, - updatedAt: Date.parse(i.updatedAt) || 0, - })); }), // The four state-machine writes + the one close. Each degrades to a logged diff --git a/runs/org-spec-audit.test.ts b/runs/org-spec-audit.test.ts index 409dd6a..a3b7866 100644 --- a/runs/org-spec-audit.test.ts +++ b/runs/org-spec-audit.test.ts @@ -1,6 +1,11 @@ // Run-level unit tests for `org-spec-audit` — drive the run against the // in-memory test runtime (`makeCFRuntimeTest`) with seeded config + sandbox + // model fakes. No CF, no Docker, no model provider. +// +// The property most of this file exists to pin: **a question already on file is +// not filed again.** The `github` fake appends every `openIssue` to the same +// list `issues` reads back, so "the second sweep sees the first sweep's issue" +// is expressible here rather than only in production. import { it } from "@effect/vitest"; import { Effect } from "effect"; @@ -9,15 +14,25 @@ import { makeCFRuntimeTest } from "@fractalboxdev/flare-dispatch-core/testing"; import { Github, GitHubApiError, + type IssueRef, type ModelCompletionResult, - type PullRequestHistoryRef, } from "@fractalboxdev/flare-dispatch-core"; import type { SuppressionReport } from "@fractalboxdev/flare-dispatch-core/primitives"; -import { mergeAcrossRepos, orgSpecAudit, parseWindowHours, renderMessage } from "./org-spec-audit"; +import { + firstMaintenanceKey, + indexFiledQuestions, + issueTitle, + mergeAcrossRepos, + orgSpecAudit, + parseLabel, + parsePositiveInt, + parseWindowHours, + renderIssueBody, + renderNotice, +} from "./org-spec-audit"; const firedAt = Date.UTC(2026, 7, 8); // 2026-08-08 const input = { firedAt } as const; -const DAY = 86_400_000; /** Nothing suppressed, nothing broken — the shape most render tests want. */ const noSuppression: SuppressionReport = { allowed: [], suppressed: [], degraded: [] }; @@ -28,6 +43,12 @@ const reported = (questions: unknown[]): ModelCompletionResult => ({ text: "", }); +/** A tools-mode result for the reconcile call — minted key → on-file key. */ +const matched = (matches: Array<{ minted: string; existing: string }>): ModelCompletionResult => ({ + toolCalls: [{ name: "report_key_matches", arguments: { matches } }], + text: "", +}); + const question = (over: Record = {}) => ({ group: "decide", question: "Does the dispatcher still commit to per-run spend caps?", @@ -38,6 +59,30 @@ const question = (over: Record = {}) => ({ ...over, }); +const KEY = "org-spec-audit/per-run-spend-caps"; +const LABEL = "maintenance:open-question"; + +/** A question already on file in the control repo — the ledger, seeded. */ +const filedIssue = (over: Partial & { key?: string } = {}): IssueRef => { + const { key = KEY, ...rest } = over; + return { + repo: "owner/control", + number: 41, + title: "Does the dispatcher still commit to per-run spend caps?", + // The key on the FIRST line, which is where `renderIssueBody` puts it. + body: `maintenance-key: ${key}\n\n\nprose`, + state: "open" as const, + labels: [LABEL, "question:decide"], + author: "flare-dispatch[bot]", + authorAssociation: "OWNER", + url: "https://github.com/owner/control/issues/41", + commentCount: 0, + createdAt: firedAt - 86_400_000, + updatedAt: firedAt - 86_400_000, + ...rest, + }; +}; + const baseConfig = { "org-spec-audit.repos": "owner/alpha owner/beta", "org-spec-audit.control-repo": "owner/control", @@ -65,12 +110,12 @@ describe("org-spec-audit", () => { return Effect.gen(function* () { const out = yield* orgSpecAudit.run(input); expect(out.reposSwept).toBe(0); - expect(out.prOpened).toBe(false); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(out.questionsFiled).toBe(0); + expect(handles.github.openIssueCalls).toHaveLength(0); }).pipe(Effect.provide(layer)); }); - it.effect("merges the same question across repos into one control-plane PR", () => { + it.effect("merges the same question across repos into ONE issue", () => { const { layer, handles } = makeCFRuntimeTest({ config: baseConfig, sandboxProgram: activeSandbox, @@ -78,6 +123,7 @@ describe("org-spec-audit", () => { // Both repos raise the same key — the merge is the point of sweeping. responses: [reported([question()]), reported([question()])], }, + github: { now: firedAt }, }); return Effect.gen(function* () { @@ -85,25 +131,148 @@ describe("org-spec-audit", () => { expect(out.reposSwept).toBe(2); expect(out.questionsRaised).toBe(2); expect(out.questionsAfterMerge).toBe(1); + expect(out.questionsFiled).toBe(1); - const calls = handles.github.openDraftPullRequestCalls; + const calls = handles.github.openIssueCalls; expect(calls).toHaveLength(1); expect(calls[0]!.repo).toBe("owner/control"); - expect(calls[0]!.headBranch).toBe("flare-dispatch/spec-audit-questions-2026-08-08"); - // The neutral default — `questions-dir` is unset in `baseConfig`, and no - // value in this repo names any particular operator's layout. - expect(calls[0]!.files[0]!.path).toBe("maintenance/questions/2026-08-08.md"); - // Both repos are named as sources on the single merged line. - expect(calls[0]!.files[0]!.content).toContain("owner/alpha"); - expect(calls[0]!.files[0]!.content).toContain("owner/beta"); - expect(calls[0]!.body).toContain("auto-merge: never"); + expect(calls[0]!.title).toBe("Does the dispatcher still commit to per-run spend caps?"); + // Both repos are named as sources on the single merged issue. + expect(calls[0]!.body).toContain("owner/alpha"); + expect(calls[0]!.body).toContain("owner/beta"); + // The index label and the lane label, from the defaults. + expect(calls[0]!.labels).toEqual([LABEL, "question:decide"]); + }).pipe(Effect.provide(layer)); + }); + + // The property the whole redesign exists for. #147 and #148 asked three of the + // same questions two days apart, two of them under a byte-identical key, + // because nothing read the key back against a question that was merely OPEN. + it.effect("files nothing when every question is already on file", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt, issues: [filedIssue()] }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsAfterMerge).toBe(1); + expect(out.questionsAlreadyFiled).toBe(1); + expect(out.questionsFiled).toBe(0); + expect(handles.github.openIssueCalls).toHaveLength(0); + // Silent in the channel too: re-announcing a standing question is the + // daily file again, one line long. + expect(handles.notice.published).toHaveLength(0); + }).pipe(Effect.provide(layer)); + }); + + it.effect("never re-files or reopens a question that was answered and closed", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { + now: firedAt, + issues: [filedIssue({ state: "closed", closedAt: firedAt - 200 * 86_400_000 })], + }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + // Closed means decided. Not a cooldown, not a 30-day window — a question + // answered 200 days ago is still answered. + expect(out.questionsAlreadyFiled).toBe(1); + expect(out.questionsFiled).toBe(0); + expect(handles.github.openIssueCalls).toHaveLength(0); + // And nothing reopens it: the run has no reopen, and does not comment. + expect(handles.github.addIssueLabelsCalls).toHaveLength(0); + expect(handles.github.commentOnIssueCalls).toHaveLength(0); + expect(handles.github.closeIssueAsDuplicateCalls).toHaveLength(0); + }).pipe(Effect.provide(layer)); + }); + + it.effect("asks for every state, un-windowed, strictly, under the questions label", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, + }); + + return Effect.gen(function* () { + yield* orgSpecAudit.run(input); + const [call] = handles.github.issuesCalls; + // Each of these is one edit away from silently breaking dedup: `open` would + // re-file every answered question, a window would resurrect the old ones, + // and a non-strict read answers "not filed" for whatever the page ceiling + // cut off. + expect(call).toMatchObject({ repo: "owner/control", state: "all", strict: true }); + expect(call!.labels).toEqual([LABEL]); + expect(call!.updatedWithinDays).toBeUndefined(); + }).pipe(Effect.provide(layer)); + }); + + // The inversion. With one PR a day an unreadable ledger cost one duplicate PR, + // so failing open was right. An unreadable ISSUE SET costs a duplicate of + // every question at once. + it.effect("files nothing at all when it cannot read what it already asked", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, + }); + + return Effect.gen(function* () { + const fake = yield* Github; + const exit = yield* Effect.exit( + orgSpecAudit.run(input).pipe( + Effect.provideService(Github, { + ...fake, + issues: () => Effect.fail(new GitHubApiError({ status: 500, reason: "transient" })), + }), + ), + ); + + expect(exit._tag).toBe("Failure"); + expect(handles.github.openIssueCalls).toHaveLength(0); + // It loses a day and no facts: the questions are re-derived tomorrow. + expect(handles.notice.published).toHaveLength(0); + }).pipe(Effect.provide(layer)); + }); + + // The read is deliberately ahead of the sweep: it is one cheap call whose + // failure ends the tick, and reading it afterwards would mean paying for an + // estate of model calls and discarding every one. + it.effect("reads the ledger before spending a single model call", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, + }); + + return Effect.gen(function* () { + const fake = yield* Github; + yield* Effect.exit( + orgSpecAudit.run(input).pipe( + Effect.provideService(Github, { + ...fake, + issues: () => Effect.fail(new GitHubApiError({ status: 500, reason: "transient" })), + }), + ), + ); + expect(handles.modelGateway.requests).toHaveLength(0); + expect(handles.sandbox.clones).toHaveLength(0); }).pipe(Effect.provide(layer)); }); // The run holds no default control repo on purpose: a default is a repo - // somebody else's deployment files pull requests against. Unset must stop the - // run, and stop it BEFORE the sweep — an hour of model calls whose output has - // nowhere to go is the expensive way to learn a key is missing. + // somebody else's deployment files issues on. Unset must stop the run, and + // stop it BEFORE the sweep — an hour of model calls whose output has nowhere + // to go is the expensive way to learn a key is missing. it.effect("fails when no control repo is configured, before sweeping anything", () => { const { layer, handles } = makeCFRuntimeTest({ config: withoutKey(baseConfig, "org-spec-audit.control-repo"), @@ -115,30 +284,43 @@ describe("org-spec-audit", () => { const exit = yield* Effect.exit(orgSpecAudit.run(input)); expect(exit._tag).toBe("Failure"); expect(JSON.stringify(exit)).toContain("org-spec-audit.control-repo"); - // Nothing was cloned, nothing was executed, nothing was proposed. - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + // Nothing was cloned, nothing was executed, nothing was filed. + expect(handles.github.openIssueCalls).toHaveLength(0); expect(handles.sandbox.clones).toHaveLength(0); expect(handles.sandbox.execs).toHaveLength(0); }).pipe(Effect.provide(layer)); }); - it.effect("writes where `questions-dir` says, not where the run was born", () => { + it.effect("labels issues the way config says, not the way the run was born", () => { const { layer, handles } = makeCFRuntimeTest({ - config: { ...baseConfig, "org-spec-audit.questions-dir": "infra/loop/open-questions/" }, + config: { + ...baseConfig, + "org-spec-audit.questions-label": "loop:question", + "org-spec-audit.lane-label-prefix": "answer-by-", + }, sandboxProgram: activeSandbox, modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { yield* orgSpecAudit.run(input); - const calls = handles.github.openDraftPullRequestCalls; - expect(calls[0]!.files[0]!.path).toBe("infra/loop/open-questions/2026-08-08.md"); + expect(handles.github.openIssueCalls[0]!.labels).toEqual([ + "loop:question", + "answer-by-decide", + ]); + // The read filters on the same label it writes — the two being one value + // is what makes dedup work at all. + expect(handles.github.issuesCalls[0]!.labels).toEqual(["loop:question"]); }).pipe(Effect.provide(layer)); }); - it.effect("refuses a questions-dir that escapes the repo root", () => { + // A comma makes the filter and the write two different things: GitHub's list + // query joins labels on commas, so the read would filter on two labels while + // the write applied one — and every question would re-file forever, silently. + it.effect("refuses a questions label carrying a comma, before reading anything", () => { const { layer, handles } = makeCFRuntimeTest({ - config: { ...baseConfig, "org-spec-audit.questions-dir": "../../etc" }, + config: { ...baseConfig, "org-spec-audit.questions-label": "loop:question,bug" }, sandboxProgram: activeSandbox, modelGateway: { responses: [reported([question()])] }, }); @@ -146,36 +328,36 @@ describe("org-spec-audit", () => { return Effect.gen(function* () { const exit = yield* Effect.exit(orgSpecAudit.run(input)); expect(exit._tag).toBe("Failure"); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(JSON.stringify(exit)).toContain("org-spec-audit.questions-label"); + expect(handles.github.issuesCalls).toHaveLength(0); + expect(handles.github.openIssueCalls).toHaveLength(0); }).pipe(Effect.provide(layer)); }); - it.effect("announces the same text it committed, under a use case", () => { + it.effect("announces the delta, links each issue, and names no channel", () => { const { layer, handles } = makeCFRuntimeTest({ config: baseConfig, sandboxProgram: activeSandbox, modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { yield* orgSpecAudit.run(input); const [notice] = handles.notice.published; - const file = handles.github.openDraftPullRequestCalls[0]!.files[0]!; if (notice === undefined) throw new Error("no notice was published"); - // One rendering, two destinations. A second wording would be a second - // thing to keep true, and the first question a reader asks about a - // digest is which copy is the real one. - expect(notice.text).toBe(file.content); + expect(notice.text).toContain("1 new question(s)"); + expect(notice.text).toContain("1 open"); // A KIND of message, never a room. The receiver maps this to a channel // from its own config; nothing here can name one. expect(notice.useCase).toBe("org-spec-audit"); expect(JSON.stringify(notice)).not.toMatch(/channel/i); - // The PR link rides as a typed entry, because markup inside `text` would - // be escaped by the receiver along with everything else. + // Issue links ride as typed entries, because markup inside `text` would be + // escaped by the receiver along with everything else. expect(notice.links).toEqual([ - { url: "https://github.com/owner/control/pull/1", label: "the questions PR" }, + { url: "https://github.com/owner/control/issues/1", label: "#1" }, ]); }).pipe(Effect.provide(layer)); }); @@ -188,6 +370,7 @@ describe("org-spec-audit", () => { config: baseConfig, sandboxProgram: activeSandbox, modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { @@ -199,21 +382,21 @@ describe("org-spec-audit", () => { }).pipe(Effect.provide(layer)); }); - it.effect("keeps the file and the verdict when the notice does not land", () => { - // The digest is already in git, which is the copy that has to survive. An - // announcement that failed must not retroactively make the sweep a failure. + it.effect("keeps the issues and the verdict when the notice does not land", () => { + // The questions are already on GitHub, which is the copy that has to + // survive. An announcement that failed must not make the sweep a failure. const { layer, handles } = makeCFRuntimeTest({ config: baseConfig, sandboxProgram: activeSandbox, modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, notice: { outcome: "failed" }, }); return Effect.gen(function* () { const out = yield* orgSpecAudit.run(input); - expect(out.prOpened).toBe(true); - expect(out.questionsAfterMerge).toBe(1); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(1); + expect(out.questionsFiled).toBe(1); + expect(handles.github.openIssueCalls).toHaveLength(1); }).pipe(Effect.provide(layer)); }); @@ -222,13 +405,14 @@ describe("org-spec-audit", () => { config: baseConfig, sandboxProgram: { ...activeSandbox, "git log --oneline": { exitCode: 0, stdout: "" } }, modelGateway: { responses: [] }, + github: { now: firedAt }, }); return Effect.gen(function* () { const out = yield* orgSpecAudit.run(input); expect(out.reposSwept).toBe(0); expect(out.reposSkipped).toBe(2); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(handles.github.openIssueCalls).toHaveLength(0); }).pipe(Effect.provide(layer)); }); @@ -237,14 +421,15 @@ describe("org-spec-audit", () => { config: baseConfig, sandboxProgram: activeSandbox, modelGateway: { responses: [reported([]), reported([])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { const out = yield* orgSpecAudit.run(input); expect(out.reposSwept).toBe(2); expect(out.questionsAfterMerge).toBe(0); - expect(out.prOpened).toBe(false); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(out.questionsFiled).toBe(0); + expect(handles.github.openIssueCalls).toHaveLength(0); // Empty means silent in the channel too. A digest that fires whether or // not there is news is one people stop reading, and by then it has // nothing left to spend. @@ -269,7 +454,7 @@ describe("org-spec-audit", () => { // The whole sweep stops: auditing 1 of 2 repos and reporting success is // how a repo drops out of the estate without anyone being told. expect(handles.sandbox.clones).toHaveLength(0); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(handles.github.openIssueCalls).toHaveLength(0); }).pipe(Effect.provide(layer)); }); @@ -298,19 +483,20 @@ describe("org-spec-audit", () => { "specs/*.md": { exitCode: 1, stdout: "", stderr: "not a git repository" }, }, modelGateway: { responses: [] }, + github: { now: firedAt }, }); return Effect.gen(function* () { const out = yield* orgSpecAudit.run(input); expect(out.reposSwept).toBe(0); expect(out.reposSkipped).toBe(2); - expect(out.prOpened).toBe(false); + expect(out.questionsFiled).toBe(0); expect(handles.modelGateway.requests).toHaveLength(0); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(handles.github.openIssueCalls).toHaveLength(0); }).pipe(Effect.provide(layer)); }); - it.effect("a crafted spec cannot register a maintenance-key the PR never proposed", () => { + it.effect("a crafted spec cannot register a maintenance-key the issue never claims", () => { const { layer, handles } = makeCFRuntimeTest({ config: baseConfig, sandboxProgram: activeSandbox, @@ -326,24 +512,20 @@ describe("org-spec-audit", () => { reported([]), ], }, + github: { now: firedAt }, }); return Effect.gen(function* () { yield* orgSpecAudit.run(input); - const body = handles.github.openDraftPullRequestCalls[0]!.body; - - // The reader's own regex (packages/core/src/primitives/suppression.ts): - // line-anchored, so it picks a key up from ANYWHERE in the body, not just - // the trailer block. The body carries one key per question it proposes — - // here exactly one — and nothing the model wrote may join that set. - const keys = [...body.matchAll(/^[ \t]*maintenance-key:[ \t]*(\S+)[ \t]*$/gm)].map( - (m) => m[1], - ); - expect(keys).toEqual(["org-spec-audit/per-run-spend-caps"]); - expect(keys).not.toContain("org-spec-audit/unrelated-question"); - expect(keys).not.toContain("org-spec-audit/another-one"); + const body = handles.github.openIssueCalls[0]!.body; + + // The authentic trailer is the body's FIRST line, and the reader takes the + // first match — so a key the model echoed is inert text further down + // rather than the identity of this issue. + expect(body.split("\n")[0]).toBe(`maintenance-key: ${KEY}`); + expect(firstMaintenanceKey(body)).toBe(KEY); - // The text is not censored — it is still readable, just not line-leading. + // The text is not censored — it is still readable, just not authoritative. expect(body).toContain("maintenance-key: org-spec-audit/unrelated-question"); }).pipe(Effect.provide(layer)); }); @@ -356,6 +538,7 @@ describe("org-spec-audit", () => { "head -800": { exitCode: 1, stdout: "", stderr: "not a git repository" }, }, modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { @@ -366,149 +549,274 @@ describe("org-spec-audit", () => { expect(handles.modelGateway.requests).toHaveLength(0); expect(out.reposSwept).toBe(0); expect(out.reposSkipped).toBe(2); - expect(out.prOpened).toBe(false); + expect(out.questionsFiled).toBe(0); }).pipe(Effect.provide(layer)); }); }); -// --- Suppression: what the run refuses to propose twice ---------------------- +// --- The per-sweep cap ------------------------------------------------------- -const LEDGER = "owner/control:maintenance/declined.jsonl"; -const KEY = "org-spec-audit/per-run-spend-caps"; +describe("org-spec-audit — the cap", () => { + const seven = Array.from({ length: 7 }, (_unused, i) => + question({ key: `q${i}`, question: `Question ${i}?` }), + ); -/** A prior proposal carrying the key, closed unmerged `daysAgo` days back. */ -const closedProposal = ( - daysAgo: number, - over: Partial = {}, -): PullRequestHistoryRef => - ({ - repo: "owner/control", - number: 7, - title: "docs(maintenance): open questions", - body: `maintenance-key: ${KEY}`, - headBranch: `flare-dispatch/spec-audit-questions-2026-06-0${daysAgo % 9}`, - headSha: "abc123", - state: "closed", - draft: true, - labels: [], - author: "flare-dispatch[bot]", - requestedReviewers: [], - url: "https://github.com/owner/control/pull/7", - createdAt: firedAt - (daysAgo + 5) * DAY, - // Touched today on purpose: a cooldown dated from `updated_at` would never - // expire, which is the whole reason `closed_at` is the field that counts. - updatedAt: firedAt, - closedAt: firedAt - daysAgo * DAY, - ...over, - }) satisfies PullRequestHistoryRef; - -/** The runtime the suppression tests share — one question, one control repo. */ -const suppressionRuntime = ( - github: NonNullable[0]>["github"], -) => - makeCFRuntimeTest({ - config: baseConfig, - sandboxProgram: activeSandbox, - modelGateway: { responses: [reported([question()]), reported([question()])] }, - github: { now: firedAt, ...github }, + it.effect("files up to the cap and says what it held, never truncating silently", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: { ...baseConfig, "org-spec-audit.max-new-questions": "2" }, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported(seven), reported([])] }, + github: { now: firedAt }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsAfterMerge).toBe(7); + expect(out.questionsFiled).toBe(2); + expect(out.questionsHeldByCap).toBe(5); + expect(handles.github.openIssueCalls).toHaveLength(2); + // A shorter list that does not say it is shorter reads as fewer problems. + expect(handles.notice.published[0]!.text).toContain("Held by the per-sweep cap: 5"); + }).pipe(Effect.provide(layer)); }); -describe("org-spec-audit — suppression", () => { - it.effect("never re-proposes a question the ledger declined", () => { - const { layer, handles } = suppressionRuntime({ - files: { - [LEDGER]: JSON.stringify({ - key: KEY, - reason: "answered in ADR-0011; the spec is right", - by: "@ada", - at: "2026-08-01", - }), + it.effect("files the held-back questions on the next sweep, since none are on file", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: { ...baseConfig, "org-spec-audit.max-new-questions": "2" }, + sandboxProgram: activeSandbox, + // Two ticks: sweeps for run 1, sweeps for run 2, then run 2's reconcile. + // The fake repeats its last entry, so `matched([])` covers that and after. + modelGateway: { + responses: [reported(seven), reported([]), reported(seven), reported([]), matched([])], }, + github: { now: firedAt }, }); return Effect.gen(function* () { + yield* orgSpecAudit.run(input); + expect(handles.github.openIssueCalls).toHaveLength(2); + // Second tick: the two filed are found on file, the other five are not. const out = yield* orgSpecAudit.run(input); - expect(out.questionsAfterMerge).toBe(1); - expect(out.questionsSuppressed).toBe(1); - expect(out.prOpened).toBe(false); - // Nothing left to ask ⇒ no PR at all, and the count still reports why. - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); + expect(out.questionsAlreadyFiled).toBe(2); + expect(out.questionsFiled).toBe(2); + expect(handles.github.openIssueCalls).toHaveLength(4); }).pipe(Effect.provide(layer)); }); - // The ledger's location is the operator's, like the questions dir. The - // default this repo ships is a placeholder, and an operator who moves the - // file must have the run follow it — including in the sentence the PR body - // prints telling a reviewer where to record a permanent decline. - it.effect("reads the ledger where `declined-path` says, and says so in the body", () => { + it.effect("defaults the cap when the value is nonsense", () => { const { layer, handles } = makeCFRuntimeTest({ - config: { ...baseConfig, "org-spec-audit.declined-path": "infra/loop/declined.jsonl" }, + config: { ...baseConfig, "org-spec-audit.max-new-questions": "-3" }, sandboxProgram: activeSandbox, - modelGateway: { responses: [reported([question()]), reported([question()])] }, - github: { - now: firedAt, - files: { "owner/control:infra/loop/declined.jsonl": "" }, + modelGateway: { responses: [reported(seven), reported([])] }, + github: { now: firedAt }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsFiled).toBe(5); + expect(handles.github.openIssueCalls).toHaveLength(5); + }).pipe(Effect.provide(layer)); + }); +}); + +// --- Key reconciliation: the same question, worded differently --------------- + +describe("org-spec-audit — key reconciliation", () => { + /** The same question as `filedIssue()`, minted under a different verb. */ + const rephrased = question({ + key: "spend-caps-per-run", + question: "Are per-run spend caps still committed to?", + }); + + it.effect("does not re-file a question the model matches onto one on file", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { + responses: [ + reported([rephrased]), + reported([]), + matched([{ minted: "spend-caps-per-run", existing: "per-run-spend-caps" }]), + ], + }, + github: { now: firedAt, issues: [filedIssue()] }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsAlreadyFiled).toBe(1); + expect(out.questionsFiled).toBe(0); + expect(handles.github.openIssueCalls).toHaveLength(0); + expect( + handles.io.logs.some((l) => l.msg.includes("reconciled onto") && l.msg.includes(KEY)), + ).toBe(true); + }).pipe(Effect.provide(layer)); + }); + + // The containment: an answer naming a key that was never read decides nothing. + // A missed match costs a human one click; an accepted hallucination is a + // question that is never asked again. + it.effect("files anyway when the model matches onto a key nobody has on file", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { + responses: [ + reported([rephrased]), + reported([]), + matched([{ minted: "spend-caps-per-run", existing: "a-question-nobody-asked" }]), + ], }, + github: { now: firedAt, issues: [filedIssue()] }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsFiled).toBe(1); + expect(handles.github.openIssueCalls).toHaveLength(1); + }).pipe(Effect.provide(layer)); + }); + + it.effect("files anyway when the reconcile call fails", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + // The third call returns a sweep payload, which cannot parse as a + // reconciliation — the same shape a model failure takes here. + modelGateway: { responses: [reported([rephrased]), reported([]), reported([])] }, + github: { now: firedAt, issues: [filedIssue()] }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsFiled).toBe(1); + expect( + handles.io.logs.some((l) => l.level === "warn" && l.msg.includes("all 1 as new")), + ).toBe(true); + }).pipe(Effect.provide(layer)); + }); + + it.effect("spends nothing on reconciliation when nothing is on file", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { yield* orgSpecAudit.run(input); - const calls = handles.github.openDraftPullRequestCalls; - expect(calls).toHaveLength(1); - expect(calls[0]!.body).toContain("`infra/loop/declined.jsonl`"); - expect(calls[0]!.body).not.toContain("maintenance/declined.jsonl"); + // Two sweeps, no third call: on a fresh control repo every question is new + // by construction and there is nothing to match against. + expect(handles.modelGateway.requests).toHaveLength(2); + expect( + handles.modelGateway.requests.some((r) => JSON.stringify(r).includes("report_key_matches")), + ).toBe(false); + }).pipe(Effect.provide(layer)); + }); + + it.effect("skips reconciliation for a question whose key already matches exactly", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([])] }, + github: { now: firedAt, issues: [filedIssue()] }, + }); + + return Effect.gen(function* () { + const out = yield* orgSpecAudit.run(input); + expect(out.questionsAlreadyFiled).toBe(1); + // An exact key match IS the same question. Paying a model to confirm it + // would be spend with no decision attached. + expect(handles.modelGateway.requests).toHaveLength(2); }).pipe(Effect.provide(layer)); }); +}); + +// --- Suppression: the pre-emptive half, and the half that retired ------------ - it.effect("honours a cooldown dated from when the proposal was closed", () => { - const { layer, handles } = suppressionRuntime({ - pullRequestHistory: [closedProposal(5)], +const LEDGER = "owner/control:maintenance/declined.jsonl"; + +describe("org-spec-audit — suppression", () => { + it.effect("never files a question the ledger declined", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { + now: firedAt, + files: { + [LEDGER]: JSON.stringify({ + key: KEY, + reason: "answered in ADR-0011; the spec is right", + by: "@ada", + at: "2026-08-01", + }), + }, + }, }); return Effect.gen(function* () { const out = yield* orgSpecAudit.run(input); + expect(out.questionsAfterMerge).toBe(1); expect(out.questionsSuppressed).toBe(1); - expect(out.prOpened).toBe(false); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(0); - // And nothing is announced either. Suppression runs BEFORE the notice, so - // a question a human declined is not re-broadcast into a channel — which - // is the louder half of re-proposing it, and the one nobody can close. + expect(out.questionsFiled).toBe(0); + expect(handles.github.openIssueCalls).toHaveLength(0); + // Nothing announced either: re-broadcasting a declined question into a + // channel is the louder half of re-proposing it, and the one nobody can + // close. expect(handles.notice.published).toHaveLength(0); }).pipe(Effect.provide(layer)); }); - it.effect("asks again once the cooldown has expired", () => { - const { layer, handles } = suppressionRuntime({ - pullRequestHistory: [closedProposal(45, { updatedAt: firedAt - 45 * DAY })], + // No PRs ⇒ no PR history ⇒ no cooldown. A prefix matching nothing would answer + // "no prior proposals" every tick, for a reason no reader could tell apart + // from the feature being off. + it.effect("reads no PR history at all", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, }); return Effect.gen(function* () { - const out = yield* orgSpecAudit.run(input); - expect(out.questionsSuppressed).toBe(0); - expect(out.prOpened).toBe(true); - expect(handles.github.openDraftPullRequestCalls).toHaveLength(1); + yield* orgSpecAudit.run(input); + expect(handles.github.pullRequestHistoryCalls).toHaveLength(0); + expect(handles.github.openIssueCalls).toHaveLength(1); }).pipe(Effect.provide(layer)); }); - it.effect("does not suppress on a proposal that was merged", () => { - const { layer } = suppressionRuntime({ - pullRequestHistory: [closedProposal(5, { mergedAt: firedAt - 5 * DAY })], + it.effect("reads the ledger where `declined-path` says, and says so in the issue", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: { ...baseConfig, "org-spec-audit.declined-path": "infra/loop/declined.jsonl" }, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt, files: { "owner/control:infra/loop/declined.jsonl": "" } }, }); return Effect.gen(function* () { - const out = yield* orgSpecAudit.run(input); - expect(out.questionsSuppressed).toBe(0); - expect(out.prOpened).toBe(true); + yield* orgSpecAudit.run(input); + const calls = handles.github.openIssueCalls; + expect(calls).toHaveLength(1); + expect(calls[0]!.body).toContain("`infra/loop/declined.jsonl`"); + expect(calls[0]!.body).not.toContain("maintenance/declined.jsonl"); }).pipe(Effect.provide(layer)); }); it.effect("skips a malformed ledger line and honours the rest", () => { - const { layer, handles } = suppressionRuntime({ - files: { - [LEDGER]: [ - "}}} not json at all", - JSON.stringify({ key: KEY, reason: "settled", by: "@ada", at: "2026-08-01" }), - ].join("\n"), + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { + now: firedAt, + files: { + [LEDGER]: [ + "}}} not json at all", + JSON.stringify({ key: KEY, reason: "settled", by: "@ada", at: "2026-08-01" }), + ].join("\n"), + }, }, }); @@ -521,12 +829,17 @@ describe("org-spec-audit — suppression", () => { }).pipe(Effect.provide(layer)); }); - it.effect("proposes anyway — and says so — when the ledger cannot be read", () => { - const { layer, handles } = suppressionRuntime({}); + // The ledger read still fails OPEN, and the asymmetry with the issue read is + // the point: one issue a human closes, versus a duplicate of everything. + it.effect("files anyway — and says so — when the ledger cannot be read", () => { + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, + }); return Effect.gen(function* () { - // Wrap the fake so only the ledger read fails; every other `github` call - // (notably the draft-PR write this test asserts on) still records. const fake = yield* Github; const out = yield* orgSpecAudit.run(input).pipe( Effect.provideService(Github, { @@ -536,20 +849,18 @@ describe("org-spec-audit — suppression", () => { ); expect(out.questionsSuppressed).toBe(0); - expect(out.prOpened).toBe(true); - const calls = handles.github.openDraftPullRequestCalls; - expect(calls).toHaveLength(1); - expect(calls[0]!.body).toContain("Suppression degraded"); + expect(out.questionsFiled).toBe(1); + expect(handles.notice.published[0]!.text).toContain("Suppression degraded"); expect(handles.io.logs.some((l) => l.level === "warn" && l.msg.includes("unreadable"))).toBe( true, ); }).pipe(Effect.provide(layer)); }); - it.effect("reports the suppressed count and reason in the PR body and the file", () => { - // Two questions: one declined, one still open — so a PR is opened AND has - // something to explain. A shorter list with no explanation reads as "fewer - // problems", which is the opposite of true. + it.effect("reports the suppressed count and reason in the notice", () => { + // Two questions: one declined, one still open — so something is filed AND + // there is something to explain. A shorter list with no explanation reads as + // "fewer problems", which is the opposite of true. const { layer, handles } = makeCFRuntimeTest({ config: baseConfig, sandboxProgram: activeSandbox, @@ -579,197 +890,265 @@ describe("org-spec-audit — suppression", () => { const out = yield* orgSpecAudit.run(input); expect(out.questionsAfterMerge).toBe(2); expect(out.questionsSuppressed).toBe(1); - expect(out.prOpened).toBe(true); - - const call = handles.github.openDraftPullRequestCalls[0]!; - expect(call.body).toContain("**Suppressed: 1**"); - expect(call.body).toContain("answered in ADR-0011"); - expect(call.body).toContain("suppressed: 1"); - // The message file IS the digest FractalBOT posts — it must say it too. - expect(call.files[0]!.content).toContain("**Suppressed: 1**"); - expect(call.files[0]!.content).toContain("1 suppressed"); + expect(out.questionsFiled).toBe(1); + + const text = handles.notice.published[0]!.text; + expect(text).toContain("**Suppressed: 1**"); + expect(text).toContain("answered in ADR-0011"); + expect(text).toContain("1 declined"); // The surviving question is still asked; the declined one is gone. - expect(call.files[0]!.content).toContain("Who owns egress?"); - expect(call.files[0]!.content).not.toContain("Does the dispatcher still commit"); + expect(text).toContain("Who owns egress?"); + expect(text).not.toContain("Does the dispatcher still commit"); }).pipe(Effect.provide(layer)); }); - it.effect("carries one maintenance-key per question, not one per PR", () => { - // A dated per-PR key would be unique every day and suppress nothing, ever. - const { layer, handles } = suppressionRuntime({}); + it.effect("carries one maintenance-key per issue, keyed on the question", () => { + // A dated key would be unique every day and match nothing, ever. + const { layer, handles } = makeCFRuntimeTest({ + config: baseConfig, + sandboxProgram: activeSandbox, + modelGateway: { responses: [reported([question()]), reported([question()])] }, + github: { now: firedAt }, + }); return Effect.gen(function* () { yield* orgSpecAudit.run(input); - const body = handles.github.openDraftPullRequestCalls[0]!.body; + const body = handles.github.openIssueCalls[0]!.body; expect(body).toContain(`maintenance-key: ${KEY}`); expect(body).not.toContain("maintenance-key: org-spec-audit/2026-08-08"); }).pipe(Effect.provide(layer)); }); - - it.effect("asks about exactly the branch prefix its own proposals use", () => { - const { layer, handles } = suppressionRuntime({}); - - return Effect.gen(function* () { - yield* orgSpecAudit.run(input); - const [call] = handles.github.pullRequestHistoryCalls; - expect(call).toMatchObject({ - repo: "owner/control", - headBranchPrefix: "flare-dispatch/spec-audit-questions-", - state: "all", - }); - expect(handles.github.openDraftPullRequestCalls[0]!.headBranch).toMatch( - new RegExp(`^${call!.headBranchPrefix}`), - ); - }).pipe(Effect.provide(layer)); - }); }); describe("mergeAcrossRepos", () => { - const raised = (over: Record) => - ({ ...question(), ...over }) as Parameters[0][number]; + const raised = (over: Record = {}) => + ({ + repo: "o/a", + group: "decide" as const, + question: "Does X still commit to Y?", + evidence: "spec says A, tree says B", + specPath: "specs/x.md", + assumption: "assume nothing changes", + key: "x-commits-to-y", + ...over, + }) as Parameters[0][number]; - it("merges on a normalized key regardless of the model's punctuation", () => { - const out = mergeAcrossRepos([ - raised({ repo: "o/a", key: "Per-Run Spend Caps" }), - raised({ repo: "o/b", key: "per_run_spend_caps" }), - ]); + it("merges one key raised by two repos into one question with two sources", () => { + const out = mergeAcrossRepos([raised(), raised({ repo: "o/b" })]); expect(out).toHaveLength(1); expect(out[0]!.sources.map((s) => s.repo)).toEqual(["o/a", "o/b"]); }); - it("counts one repo raising the same key twice as one question", () => { - const out = mergeAcrossRepos([raised({ repo: "o/a" }), raised({ repo: "o/a" })]); + it("keeps one repo raising the same key twice as one question", () => { + const out = mergeAcrossRepos([raised(), raised()]); expect(out).toHaveLength(1); expect(out[0]!.sources).toHaveLength(1); }); - it("ranks the most-shared question first", () => { + it("ranks by DISTINCT repos, not by source count", () => { + // One repo raising a question from two specs is still one repo asking. const out = mergeAcrossRepos([ - raised({ repo: "o/a", key: "lonely" }), - raised({ repo: "o/b", key: "shared" }), - raised({ repo: "o/c", key: "shared" }), + raised({ key: "twice-in-one-repo", specPath: "specs/a.md" }), + raised({ key: "twice-in-one-repo", specPath: "specs/b.md" }), + raised({ key: "shared", repo: "o/a" }), + raised({ key: "shared", repo: "o/b" }), ]); expect(out[0]!.key).toBe("shared"); }); - it("ranks by distinct repos, so one repo's two specs can't outrank two repos", () => { - const out = mergeAcrossRepos([ - raised({ repo: "o/a", key: "one-repo-twice", specPath: "specs/x.md" }), - raised({ repo: "o/a", key: "one-repo-twice", specPath: "specs/y.md" }), - raised({ repo: "o/b", key: "two-repos" }), - raised({ repo: "o/c", key: "two-repos" }), - ]); - // Both merge to 2 sources; only `two-repos` is a question the estate shares, - // which is the entire reason this run sweeps rather than running per repo. - expect(out[0]!.key).toBe("two-repos"); + it("strips zero-width and bidi characters from model prose", () => { + const out = mergeAcrossRepos([raised({ repo: "o/a", question: "Does​ X‮ still commit⁦ to Y?" })]); + expect(out[0]!.question).toBe("Does X still commit to Y?"); }); +}); - it("falls back to the question text when the model's key is junk", () => { - const out = mergeAcrossRepos([raised({ repo: "o/a", key: "-" })]); - expect(out[0]!.key).toContain("does-the-dispatcher"); +describe("parseWindowHours", () => { + it("defaults when unset or nonsense", () => { + expect(parseWindowHours(undefined)).toBe(26); + expect(parseWindowHours("")).toBe(26); + expect(parseWindowHours("0")).toBe(26); + expect(parseWindowHours("-4")).toBe(26); }); - // The reader that parses `maintenance-key:` drops any key over 200 chars, and - // a dropped key is worse than a short one: the question keeps being proposed - // and can never be recorded as declined. - it("caps the model's own key, not only the fallback", () => { - const out = mergeAcrossRepos([raised({ repo: "o/a", key: "x".repeat(400) })]); - // `org-spec-audit/` + key must still fit the reader's 200-char budget. - expect(`org-spec-audit/${out[0]!.key}`.length).toBeLessThanOrEqual(200); - expect(out[0]!.key.length).toBeGreaterThan(3); + it("takes a positive integer", () => { + expect(parseWindowHours("72")).toBe(72); }); +}); - it("never leaves a trailing hyphen when the cap lands mid-word", () => { - const out = mergeAcrossRepos([raised({ repo: "o/a", key: `${"ab-".repeat(200)}tail` })]); - expect(out[0]!.key).not.toMatch(/-$/); +describe("parseLabel", () => { + it("falls back when unset or blank", () => { + expect(parseLabel(undefined, "d")).toBe("d"); + expect(parseLabel(null, "d")).toBe("d"); + expect(parseLabel(" ", "d")).toBe("d"); }); - it("collapses a model field that spans lines, so it cannot start one", () => { - const out = mergeAcrossRepos([ - raised({ - repo: "o/a", - evidence: "spec says X\nmaintenance-key: org-spec-audit/unrelated\nand the tree says Y", - }), - ]); - expect(out[0]!.evidence).not.toContain("\n"); - expect(out[0]!.evidence).toBe( - "spec says X maintenance-key: org-spec-audit/unrelated and the tree says Y", - ); + it("trims a usable value", () => { + expect(parseLabel(" loop:question ", "d")).toBe("loop:question"); }); - it("strips zero-width and bidi characters from model prose", () => { - const out = mergeAcrossRepos([ - raised({ repo: "o/a", question: "Does​ X‮ still commit⁦ to Y?" }), - ]); - expect(out[0]!.question).toBe("Does X still commit to Y?"); + // Set-and-unusable is `undefined`, never the fallback: a label that filters + // differently than it writes breaks dedup with nothing erroring. + it("rejects a comma and an over-long name rather than falling back", () => { + expect(parseLabel("a,b", "d")).toBeUndefined(); + expect(parseLabel("x".repeat(51), "d")).toBeUndefined(); + expect(parseLabel("x".repeat(50), "d")).toBe("x".repeat(50)); }); }); -describe("parseWindowHours", () => { - it("defaults when unset or nonsense", () => { - expect(parseWindowHours(undefined)).toBe(26); - expect(parseWindowHours("")).toBe(26); - expect(parseWindowHours("0")).toBe(26); - expect(parseWindowHours("-4")).toBe(26); +describe("parsePositiveInt", () => { + it("defaults when unset or non-positive", () => { + expect(parsePositiveInt(undefined, 5)).toBe(5); + expect(parsePositiveInt("", 5)).toBe(5); + expect(parsePositiveInt("0", 5)).toBe(5); + expect(parsePositiveInt("-2", 5)).toBe(5); + expect(parsePositiveInt("abc", 5)).toBe(5); }); it("takes a positive integer", () => { - expect(parseWindowHours("72")).toBe(72); + expect(parsePositiveInt("12", 5)).toBe(12); + }); +}); + +describe("firstMaintenanceKey / indexFiledQuestions", () => { + it("takes the first key, so a later one cannot claim the issue", () => { + const body = [ + "maintenance-key: org-spec-audit/real", + "", + "evidence mentioning maintenance-key: org-spec-audit/spoofed", + ].join("\n"); + expect(firstMaintenanceKey(body)).toBe("org-spec-audit/real"); + }); + + it("has no key when there is no trailer line", () => { + expect(firstMaintenanceKey("just prose\nand more prose")).toBeUndefined(); + }); + + it("indexes only this run's namespace", () => { + const index = indexFiledQuestions([ + filedIssue({ number: 1, key: "org-spec-audit/mine" }), + filedIssue({ number: 2, key: "some-other-run/theirs" }), + // A human-opened issue carrying the label is not a question this run asked. + filedIssue({ number: 3, body: "no trailer here" }), + ]); + expect([...index.keys()]).toEqual(["org-spec-audit/mine"]); + }); + + it("keeps the first of a duplicated key, deterministically", () => { + const index = indexFiledQuestions([filedIssue({ number: 9 }), filedIssue({ number: 10 })]); + expect(index.get(KEY)?.number).toBe(9); + }); +}); + +describe("issueTitle", () => { + const q = (question: string) => ({ + key: "k", + group: "decide" as const, + question, + assumption: "a", + sources: [{ repo: "o/a", specPath: "specs/x.md" }], + evidence: "e", + }); + + it("is the question as asked", () => { + expect(issueTitle(q("Who owns egress?"))).toBe("Who owns egress?"); + }); + + it("cuts an over-long title where we can see it, not where GitHub does", () => { + const out = issueTitle(q("Q".repeat(400))); + expect(out).toHaveLength(240); + expect(out.endsWith("…")).toBe(true); + }); +}); + +describe("renderIssueBody", () => { + const q = { + key: "spend-caps", + group: "decide" as const, + question: "Who owns egress?", + assumption: "assume nothing changes", + sources: [{ repo: "o/a", specPath: "specs/x.md" }], + evidence: "spec says A, tree says B", + }; + + it("puts the trailer first, then the answer-if-nobody-does and the evidence", () => { + const out = renderIssueBody({ + question: q, + day: "2026-08-08", + declinedPath: "infra/declined.jsonl", + }); + expect(out.split("\n")[0]).toBe("maintenance-key: org-spec-audit/spend-caps"); + expect(out).toContain("**If nobody answers:** assume nothing changes"); + expect(out).toContain("spec says A, tree says B"); + expect(out).toContain("`o/a` (specs/x.md)"); + expect(out).toContain("`infra/declined.jsonl`"); + }); + + it("tells the reader that closing is the record and nothing reopens it", () => { + const out = renderIssueBody({ question: q, day: "2026-08-08", declinedPath: "d.jsonl" }); + expect(out).toContain("Closing is the record"); + expect(out).toContain("never reopen"); }); }); -describe("renderMessage", () => { - const merged = (n: number, group: "decide" | "confirm") => +describe("renderNotice", () => { + const opened = (n: number, group: "decide" | "confirm") => Array.from({ length: n }, (_unused, i) => ({ - key: `k${i}`, - group, - question: `Q${i}?`, - assumption: "assume nothing changes", - sources: [{ repo: "o/a", specPath: "specs/x.md" }], - evidence: "spec says X, tree says Y", + number: 100 + i, + url: `https://github.com/o/c/issues/${100 + i}`, + question: { + key: `k${i}`, + group, + question: `Q${i}?`, + assumption: "assume nothing changes", + sources: [{ repo: "o/a", specPath: "specs/x.md" }], + evidence: "spec says X, tree says Y", + }, })); - it("states what the per-group cap kept out rather than truncating silently", () => { - const out = renderMessage({ - day: "2026-08-08", - merged: merged(7, "decide"), + const base = { + day: "2026-08-08", + openOnFile: 3, + heldByCap: 0, + alreadyFiled: 0, + raised: 1, + suppression: noSuppression, + }; + + it("leads with the delta, and links each new question by number", () => { + const out = renderNotice({ + ...base, + opened: opened(2, "decide"), outcomes: [{ repo: "o/a", skipped: false, questions: [] }], - raised: 7, - suppression: noSuppression, }); - expect(out).toContain("2 more in this group, not shown"); - expect(out).toContain("Below the per-group cap: 2"); + expect(out).toContain("2 new question(s) · 3 open"); + expect(out).toContain("(#100)"); + expect(out).toContain("(#101)"); }); it("names the repos it swept and the ones it skipped", () => { - const out = renderMessage({ - day: "2026-08-08", - merged: merged(1, "confirm"), + const out = renderNotice({ + ...base, + opened: opened(1, "confirm"), outcomes: [ { repo: "o/a", skipped: false, questions: [] }, { repo: "o/dormant", skipped: true, questions: [] }, ], - raised: 1, - suppression: noSuppression, }); expect(out).toContain("Swept: `o/a`"); expect(out).toContain("`o/dormant`"); }); it("separates a repo that failed from one that was quiet", () => { - const out = renderMessage({ - day: "2026-08-08", - merged: merged(1, "confirm"), + const out = renderNotice({ + ...base, + opened: opened(1, "confirm"), outcomes: [ { repo: "o/a", skipped: false, questions: [] }, { repo: "o/dormant", skipped: true, questions: [] }, { repo: "o/broken", skipped: true, failure: "model call failed (429)", questions: [] }, ], - raised: 1, - suppression: noSuppression, }); // A failure counted as "unchanged" turns an outage into good news. - expect(out).toContain("1 unchanged or without specs"); expect(out).toContain("1 failed"); expect(out).toContain("Failed: `o/broken` (model call failed (429))"); expect(out).not.toContain("Failed: none"); @@ -777,15 +1156,24 @@ describe("renderMessage", () => { expect(out.indexOf("could not be swept")).toBeLessThan(out.indexOf("## Confirm")); }); - it("says so explicitly when nothing failed", () => { - const out = renderMessage({ - day: "2026-08-08", - merged: merged(1, "confirm"), + it("says so explicitly when nothing failed and nothing was held", () => { + const out = renderNotice({ + ...base, + opened: opened(1, "confirm"), outcomes: [{ repo: "o/a", skipped: false, questions: [] }], - raised: 1, - suppression: noSuppression, }); expect(out).toContain("Failed: none"); + expect(out).toContain("Nothing held by the cap."); expect(out).not.toContain("could not be swept"); }); + + it("names what the cap held rather than shortening the list in silence", () => { + const out = renderNotice({ + ...base, + opened: opened(2, "decide"), + heldByCap: 5, + outcomes: [{ repo: "o/a", skipped: false, questions: [] }], + }); + expect(out).toContain("Held by the per-sweep cap: 5"); + }); }); diff --git a/runs/org-spec-audit.ts b/runs/org-spec-audit.ts index 7bd9e3f..9aa49c8 100644 --- a/runs/org-spec-audit.ts +++ b/runs/org-spec-audit.ts @@ -3,9 +3,8 @@ // A Schedule-mode run that sweeps every configured repo, reads each one's // `specs/` against its tree, and collects the divergences that CANNOT be // reconciled automatically — the ones where *which side is right* is a -// judgment nobody has made yet. It deduplicates them across the estate, groups -// them by the answer that unblocks them, and opens ONE draft PR against the -// control repo carrying the grouped list as a dated markdown file. +// judgment nobody has made yet. It deduplicates them across the estate and +// files each one as ONE GITHUB ISSUE in the control repo. // // --- Why this is a separate run from `spec-drift-pr` ------------------------- // @@ -19,52 +18,90 @@ // quarters, two specs in different repos that disagree. Per repo those die in // a PR body. Across an estate the same question is usually raised in three // repos, and answering it once closes all three. That merge is the entire -// reason this sweeps instead of running per repo, and it is why the output is -// ONE PR against the control repo rather than one per repo. +// reason this sweeps instead of running per repo, and it is why every question +// lands in ONE control repo rather than in the repo that raised it. // -// --- Delivery: the file is the record, the notice is the announcement -------- +// --- One question, one issue — and why it is not a daily file --------------- +// +// This run shipped filing a dated `.md` in a draft PR: every question it +// found that day, rendered fresh. Two sweeps two days apart then asked three of +// the same questions twice, two of them under a BYTE-IDENTICAL +// `maintenance-key` (`org#147`, `org#148`). +// +// An identical key that re-proposes rules out the obvious cause and leaves the +// real one. The ledger only ever remembered *no*: both suppression rules key +// off a TERMINATED proposal — a decline recorded in the ledger, or a PR closed +// unmerged — and the first PR was neither. It was open, unanswered, sitting in +// the queue, which the design read as no signal at all. +// +// So the unit is now the question, and the artifact is a GitHub issue: +// +// open — asked, unanswered → this run writes NOTHING. Silence is correct. +// closed — answered, or declined → never filed again, and never reopened. +// absent — new → filed. +// +// That collapses the whole apparatus into one field. `open` is a first-class +// answer to "have I raised this?", which no ledger of declines could express, +// and a decline stops needing a second file a human maintains by hand. +// +// Two rules follow, and both are load-bearing: +// +// * NEVER reopen and never re-announce. A run that reopens an issue argues +// with the person who closed it, and a daily "still open" comment is the +// daily file again in miniature. How long a decision has gone unmade is +// already legible from the issue's own age. +// * The dedup read fails CLOSED (below), inverting the suppression +// primitive's posture on purpose. +// +// --- Delivery: the issue is the record, the notice is the announcement ------- // // This run does not post to Slack, and must not be given a way to. Slack bot // tokens live with the Slack ingress and stay there (see // `apps/dispatcher/src/slack-notify.ts`) — a cron run holding a workspace-write // credential is how a token ends up somewhere nobody meant it to be. // -// So it does two things with one rendering. The PR carries the message as a -// dated markdown file: that is the durable artifact and the reviewed record, -// and answering in its thread is how the questions get closed. Then -// `notice.publish` hands the SAME text to the `notice` capability, which names +// The issues are the durable record; answering in a thread and closing is how a +// question ends. `notice.publish` then announces the DELTA — filed today, how +// many stand open, what the cap held — to the `notice` capability, which names // a use case and nothing else; the Slack ingress resolves that to a room and -// posts it with the token it already holds. One direction of trust, no new -// credential here, and no second wording to drift from the first. +// posts it with the token it already holds. +// +// The delta matters as much as the medium. The old message was the day's whole +// file, so its length tracked the sweep rather than the news and a reader who +// had already seen eleven questions was shown eleven questions again. Now most +// days say nothing, and "nothing" means nothing CHANGED rather than nothing +// found — with the open-question count one GitHub query away for anyone who +// wants to tell those apart. // -// Both halves are best-effort in the one direction that matters: a notice that -// did not land is a logged line, never a verdict. The questions are already in -// git, which is the copy that has to survive. +// --- Suppression: two reads, and only one may fail open ---------------------- // -// --- Suppression: the loop's memory ------------------------------------------ +// The issue set is the memory. Before filing, the run reads every issue in the +// control repo carrying the questions label, in ANY state, and matches on the +// `maintenance-key: org-spec-audit/` line in each body. // -// Before it proposes anything, the run asks the `suppression` primitive which -// of today's questions it has already been told no to. A key in the control -// repo's declines ledger is never proposed again; a -// question whose proposal a human closed unmerged waits out a 30-day cooldown -// dated from `closed_at`. Every proposed question carries its own -// `maintenance-key: org-spec-audit/` line in the PR body — that -// line is what both halves match on. +// That read **fails closed**, which reverses what this run used to do. When one +// PR a day carried every question, an unreadable ledger cost one duplicate PR, +// so failing open was right and failing closed would have silenced the loop. +// Now an unreadable issue set costs a duplicate of EVERY question at once — so +// a sweep that cannot enumerate what it has already asked files nothing. It +// loses a day and no facts, because the questions are re-derived tomorrow. +// Same reason the read is `state: "all"`, un-windowed, and `strict` (a list the +// page ceiling cut short answers "not filed" for everything it never reached). // -// The suppressed count and the reason for each appear in the PR body AND in the -// message file, because a silently shorter list reads as "fewer problems", -// which is the opposite of what it means. Both reads fail open: if the ledger -// or the PR history cannot be read, the run proposes anyway and prints the -// warning, since a duplicate PR is a nuisance and a silently disabled loop is -// the failure the mechanism exists to prevent. +// The declines ledger (`declined.jsonl`) is the PRE-EMPTIVE half and still +// fails open: a key there is never filed at all, and if the file cannot be read +// the run files anyway, because the cost is one issue a human closes — and that +// close then suppresses it permanently, which is a better end state than a loop +// silently disabled by an unreachable file. The PR-history half retires with +// the PR: this run opens none, so no cooldown is computed from one. // // --- CONFIG the operator sets (out of band) --------------------------------- // // Every value that names an operator's own estate is a key, not a constant. -// This run is generic machinery — which repos it reads, which repo it writes -// to, and where in that repo the file lands are the operator's business and -// live in their config, never in this file. A default that names somebody's -// repo is a default that files a PR against it. +// This run is generic machinery — which repos it reads, which repo it files +// into, and what it labels the issues are the operator's business and live in +// their config, never in this file. A default that names somebody's repo is a +// default that opens issues on it. // // Unset keys are not uniform, and the split is deliberate. An unset `repos` is // a run nobody has pointed at anything yet: it warns and no-ops, because on a @@ -73,16 +110,18 @@ // opposite — the sweep would do all its work with nowhere to put the answer — // so that one fails the run loudly. // -// CONFIG_KV org-spec-audit.repos comma/space-separated `owner/name` estate to sweep (optional — unset disables the sweep) -// CONFIG_KV org-spec-audit.base base branch to read (default "main") -// CONFIG_KV org-spec-audit.control-repo `owner/name` the questions PR lands in (REQUIRED — no default) -// CONFIG_KV org-spec-audit.questions-dir repo-relative dir for `.md` (default "maintenance/questions") -// CONFIG_KV org-spec-audit.declined-path repo-relative declines ledger (default "maintenance/declined.jsonl") -// CONFIG_KV org-spec-audit.window-hours skip a repo with no commits in this window (default "26") -// CONFIG_KV org-spec-audit.backend "workers-ai" | "anthropic" | "bedrock" (default workers-ai) -// CONFIG_KV org-spec-audit.prompt (optional) override the question-detection system prompt -// CONFIG_KV org-spec-audit.workers-ai.model model id -// CONFIG_KV org-spec-audit.workers-ai.mode "tools" | "json" (default "tools") +// CONFIG_KV org-spec-audit.repos comma/space-separated `owner/name` estate to sweep (optional — unset disables the sweep) +// CONFIG_KV org-spec-audit.base base branch to read (default "main") +// CONFIG_KV org-spec-audit.control-repo `owner/name` the question issues land in (REQUIRED — no default) +// CONFIG_KV org-spec-audit.questions-label label marking a question issue (default "maintenance:open-question") +// CONFIG_KV org-spec-audit.lane-label-prefix prefix + group → the lane label (default "question:") +// CONFIG_KV org-spec-audit.max-new-questions issues filed per sweep; the rest are counted (default 5) +// CONFIG_KV org-spec-audit.declined-path repo-relative declines ledger (default "maintenance/declined.jsonl") +// CONFIG_KV org-spec-audit.window-hours skip a repo with no commits in this window (default "26") +// CONFIG_KV org-spec-audit.backend "workers-ai" | "anthropic" | "bedrock" (default workers-ai) +// CONFIG_KV org-spec-audit.prompt (optional) override the question-detection system prompt +// CONFIG_KV org-spec-audit.workers-ai.model model id +// CONFIG_KV org-spec-audit.workers-ai.mode "tools" | "json" (default "tools") // // Mode: Schedule mode — specs/04-gha-integration.md § Schedule mode. The cron // MUST also be in wrangler.jsonc `triggers.crons`. @@ -99,7 +138,7 @@ import { step, type Container, } from "@fractalboxdev/flare-dispatch-core"; -import type { CheckoutFailed, GitHubApiError } from "@fractalboxdev/flare-dispatch-core"; +import type { CheckoutFailed, GitHubApiError, IssueRef } from "@fractalboxdev/flare-dispatch-core"; import { checkSuppression, DECLINED_LEDGER_PATH, @@ -129,29 +168,36 @@ const key = namespacedKey(NAMESPACE); const REPOS_KEY = key("repos"); const BASE_KEY = key("base"); const CONTROL_REPO_KEY = key("control-repo"); -const QUESTIONS_DIR_KEY = key("questions-dir"); +const QUESTIONS_LABEL_KEY = key("questions-label"); +const LANE_LABEL_PREFIX_KEY = key("lane-label-prefix"); +const MAX_NEW_QUESTIONS_KEY = key("max-new-questions"); const DECLINED_PATH_KEY = key("declined-path"); const WINDOW_HOURS_KEY = key("window-hours"); /** - * Where the dated questions file lands inside the control repo. + * The label every question issue carries — the ledger's INDEX. * - * A directory, not a template: the run appends `.md`, so there is no - * placeholder syntax to get wrong and no way for config to name a single file - * that every day overwrites. + * Machine state, not a human affordance: the dedup read filters on it to find + * every question this run has ever asked, so a question whose label someone + * removes becomes fileable again. The lane labels below are the opposite — + * nothing matches on them, so they are safe to retriage by hand. */ -const QUESTIONS_DIR_DEFAULT = "maintenance/questions"; +const QUESTIONS_LABEL_DEFAULT = "maintenance:open-question"; +const LANE_LABEL_PREFIX_DEFAULT = "question:"; /** - * The `maintenance-key` namespace and the branch prefix every proposal shares. + * How many issues one sweep may open. * - * Both are load-bearing for suppression: the key is what the ledger matches on, - * and the prefix is how a later tick finds the PRs a human already closed - * (each day's proposal gets its own dated branch, so the prefix is all they - * have in common). + * A first sweep of a widened estate can find twenty, and twenty new issues at + * 05:45 is a wall rather than a digest. What the cap holds back is counted and + * named in the notice — never dropped silently, because a shorter list that + * does not say it is shorter reads as fewer problems — and files on the next + * sweep, which finds it un-filed and therefore fresh. */ +const MAX_NEW_QUESTIONS_DEFAULT = 5; + +/** The `maintenance-key` namespace every question is suppressed by. */ const MAINTENANCE_SOURCE = "org-spec-audit"; -const BRANCH_PREFIX = "flare-dispatch/spec-audit-questions-"; /** The stable, repo-independent id a question is suppressed by. */ const maintenanceKey = (questionKey: string): string => `${MAINTENANCE_SOURCE}/${questionKey}`; @@ -172,8 +218,10 @@ const MAX_SPECS_CHARS = 40_000; const MAX_TREE_CHARS = 12_000; const QUESTIONS_MAX_TOKENS = 2048; -/** How many questions per group reach the message. The rest are counted, not dropped silently. */ -const PER_GROUP_CAP = 5; +// A per-group rendering cap used to live here, because the message carried every +// standing question and a long group buried the rest. The notice now lists only +// what was FILED this tick, which `max-new-questions` already bounds — one cap +// instead of two, and the one that bounds the writes is the one that matters. /** * The four groups, in message order. @@ -211,12 +259,61 @@ const AuditQuestions = Schema.Struct({ * A stable, repo-INDEPENDENT slug of the underlying question. Two repos * asking the same thing must produce the same key or the cross-repo * merge — the reason this run sweeps at all — silently does nothing. + * + * Stable across DAYS as well as repos, which is the harder half and was + * once wrong here: the same question arrived as `authorize-pipeline-dags` + * one day and `adopt-pipeline-dags` the next, matching nothing. The prompt + * asks for a noun phrase for that reason, and `reconcileKeys` catches what + * prompt discipline misses — a rule with no mechanism behind it is a rule + * that holds until the model rephrases. */ key: Schema.String, }), ), }); +/** + * One verdict per newly-minted key: the key already on file that it means the + * same question as, or `""` for none. + * + * Deliberately not "is this a duplicate, yes/no" — the model has to NAME the + * question it thinks this duplicates, and that name is then checked against the + * set actually read from the control repo. A key it invents matches nothing and + * the question gets filed, which is the safe direction: the cost of a missed + * match is one duplicate issue a human closes, and the cost of an accepted + * hallucination is a question silently never asked. + */ +const KeyReconciliation = Schema.Struct({ + matches: Schema.Array( + Schema.Struct({ + /** The key this sweep minted, echoed back so the mapping is unambiguous. */ + minted: Schema.String, + /** An existing key from the list given, or `""` when this question is new. */ + existing: Schema.String, + }), + ), +}); + +const RECONCILE_PROMPT = `You are matching newly-raised questions against questions already on file. + +For each NEW question you are given, decide whether it is THE SAME QUESTION as +one of the questions already on file — the same decision, needing the same +answer, however differently it is worded. Wording, framing and the verb used +carry no weight; the subject and the decision it needs are what matter. + +Rules: +- Answer with the existing key when it is the same question, and "" when it is + not. Every new key you were given gets exactly one row. +- Only ever answer with a key from the on-file list, verbatim. Never invent one, + never adjust one, and never answer with a new key. +- Narrower or broader is NOT the same question. "Should we support DAGs" and + "should we deprecate the linear model" need different answers; keep them apart. +- When you are unsure, answer "". A duplicate question costs a human one click; + a question wrongly matched away is never asked again.`; + +const RECONCILE_JSON_CONTRACT = `{"matches":[{"minted":string,"existing":string}]}`; +const RECONCILE_MAX_TOKENS = 1024; + /** The question-detection prompt (operator-overridable). */ const QUESTIONS_PROMPT_DEFAULT = `You read a project's specs/ against its file tree and recent commits, and you report ONLY what cannot be reconciled automatically. @@ -236,8 +333,12 @@ Rules, each of which drops a finding when broken: evidence, no question. - State the assumption we should make if nobody answers. A question without one waits for a meeting. -- The key is a short lowercase slug of the UNDERLYING question, with no repo - name in it, so the same question asked in two repos merges into one line. +- The key is a short lowercase slug NAMING THE SUBJECT of the question, as a + noun phrase and never a verb phrase: "pipeline-dags", not + "adopt-pipeline-dags", "authorize-pipeline-dags" or "should-we-adopt-dags". A + verb encodes the action being proposed, which changes with how you phrase it; + the subject of the question does not. No repo name in it, so the same question + asked in two repos merges into one line. - Report nothing rather than padding. An empty array is a good answer.`; const Input = Schema.Struct({ @@ -249,9 +350,20 @@ const Output = Schema.Struct({ reposSkipped: Schema.Number, questionsRaised: Schema.Number, questionsAfterMerge: Schema.Number, - /** Merged questions the ledger or a cooldown kept out of the proposal. */ + /** + * Merged questions that already have an issue, in any state. + * + * The number this whole design exists to make non-zero on a steady estate: on + * a quiet week every question the sweep raises is one already on file, and the + * correct output is no writes at all. + */ + questionsAlreadyFiled: Schema.Number, + /** Merged questions the declines ledger kept from being filed. */ questionsSuppressed: Schema.Number, - prOpened: Schema.Boolean, + /** Issues actually opened this tick. */ + questionsFiled: Schema.Number, + /** Fresh questions the per-sweep cap held back — they file on the next tick. */ + questionsHeldByCap: Schema.Number, }); export const orgSpecAudit = defineRun({ @@ -309,8 +421,10 @@ export const orgSpecAudit = defineRun({ reposSkipped: 0, questionsRaised: 0, questionsAfterMerge: 0, + questionsAlreadyFiled: 0, questionsSuppressed: 0, - prOpened: false, + questionsFiled: 0, + questionsHeldByCap: 0, }; } @@ -341,13 +455,75 @@ export const orgSpecAudit = defineRun({ // model calls whose output has nowhere to go. See // `primitives/control-plane` for the rule and the failure text. const controlRepo = yield* resolveControlRepo(CONTROL_REPO_KEY); - const questionsDir = yield* resolveRepoRelativePath(QUESTIONS_DIR_KEY, QUESTIONS_DIR_DEFAULT); const declinedPath = yield* resolveRepoRelativePath(DECLINED_PATH_KEY, DECLINED_LEDGER_PATH); + // Set-and-unusable fails; unset takes the default. A label that cannot be + // both filtered on and applied would break dedup silently — see + // `parseLabel`. + const questionsLabel = parseLabel( + yield* step("resolve-questions-label", () => config.get(QUESTIONS_LABEL_KEY)), + QUESTIONS_LABEL_DEFAULT, + ); + if (questionsLabel === undefined) { + return yield* Effect.fail( + new StepFailed({ + step: "resolve-questions-label", + cause: `${QUESTIONS_LABEL_KEY} is not a usable GitHub label (no comma, max ${LABEL_MAX_CHARS} chars)`, + }), + ); + } + const lanePrefix = parseLabel( + yield* step("resolve-lane-prefix", () => config.get(LANE_LABEL_PREFIX_KEY)), + LANE_LABEL_PREFIX_DEFAULT, + ); + if (lanePrefix === undefined) { + return yield* Effect.fail( + new StepFailed({ + step: "resolve-lane-prefix", + cause: `${LANE_LABEL_PREFIX_KEY} is not a usable GitHub label prefix (no comma, max ${LABEL_MAX_CHARS} chars)`, + }), + ); + } + const maxNew = parsePositiveInt( + yield* step("resolve-max-new", () => config.get(MAX_NEW_QUESTIONS_KEY)), + MAX_NEW_QUESTIONS_DEFAULT, + ); + const windowHours = parseWindowHours( yield* step("resolve-window", () => config.get(WINDOW_HOURS_KEY)), ); + // 1b. The ledger read — BEFORE the sweep, not after. + // + // It is one cheap call whose failure ends the tick, so it belongs + // where the other deterministic exits are (§7). Reading it after the + // sweep would mean paying for an estate's worth of model calls and + // then discarding every one of them. + // + // `state: "all"` because a question answered a year ago must still + // suppress; un-windowed for the same reason; `strict` because a list + // the page ceiling cut short answers "not filed" for everything it + // never reached, and this read's answer to that question is what + // decides whether anything is written. + // + // No `catchAll`. A failure here fails the run, which is the whole + // inversion: better a day with no questions filed than a day that + // files a duplicate of every question on file. + const onFile = yield* step("read-question-ledger", () => + github.issues({ + repo: controlRepo, + state: "all", + labels: [questionsLabel], + strict: true, + }), + ); + const filed = indexFiledQuestions(onFile); + yield* io.log( + "info", + `org-spec-audit: ${filed.size} question(s) already on file in ${controlRepo} ` + + `(${onFile.filter((i) => i.state === "open").length} open)`, + ); + // 2. The backend, under THIS run's namespace. A misconfigured backend // fails loudly — an audit that silently answers nothing is worse than // one that does not run. @@ -390,7 +566,7 @@ export const orgSpecAudit = defineRun({ const swept = outcomes.filter((o) => !o.skipped).length; const skipped = outcomes.filter((o) => o.skipped).length; - // 4. Empty means silent — no PR and, now, no notice. A digest that fires + // 4. Empty means silent — no issue and no notice. A digest that fires // whether or not there is news is one people learn to skip, and that // is far more expensive in a channel than in a repo: the day it does // have something to say, nobody is reading. @@ -401,108 +577,156 @@ export const orgSpecAudit = defineRun({ reposSkipped: skipped, questionsRaised: raised.length, questionsAfterMerge: 0, + questionsAlreadyFiled: 0, questionsSuppressed: 0, - prOpened: false, + questionsFiled: 0, + questionsHeldByCap: 0, }; } - // 5. Suppression, BEFORE anything is proposed. A question the ledger - // declined is never asked again; one whose proposal a human closed - // unmerged waits out a cooldown dated from the close. Both reads fail - // OPEN and say so — a duplicate PR is a nuisance, a silently disabled - // loop is the failure this whole mechanism exists to prevent. + // 5. Which of today's questions are already on file? Exact keys first, + // deterministically and for free — a matching key IS the same question + // and needs nothing to confirm it. + const exact = merged.filter((q) => filed.has(maintenanceKey(q.key))); + const residue = merged.filter((q) => !filed.has(maintenanceKey(q.key))); + + // 6. Then reconcile the residue, ONCE, for the whole batch. A key is + // minted from prose, so two sweeps can name one question twice — + // `authorize-pipeline-dags` and `adopt-pipeline-dags` were the same + // question on consecutive days. Asking a model to MATCH against what is + // on file is far more stable than asking it to invent the same slug + // twice, and it is one call for the batch rather than one per question. + // + // Skipped entirely when there is nothing on file to match against — + // on a fresh control repo every question is new by construction. + const matchRows = + residue.length > 0 && filed.size > 0 + ? yield* step("reconcile-keys", () => + reconcileKeys({ + residue, + onFile: [...filed.values()], + resolved, + }), + ).pipe( + // A model that cannot answer must not be able to file duplicates + // OR to suppress questions: falling back to "nothing matched" + // files the residue, which is the direction whose worst case is a + // human closing an issue. + Effect.catchAll((err) => + io + .log( + "warn", + `org-spec-audit: key reconciliation failed (${describe(err)}) — treating all ${residue.length} as new`, + ) + .pipe(Effect.as([] as readonly KeyMatch[])), + ), + ) + : ([] as readonly KeyMatch[]); + const reconciled = new Map(matchRows.map((m) => [m.minted, m.existing])); + + const alreadyFiled = [...exact, ...residue.filter((q) => reconciled.has(q.key))]; + const unfiled = residue.filter((q) => !reconciled.has(q.key)); + + for (const q of residue) { + const match = reconciled.get(q.key); + if (match !== undefined) { + yield* io.log( + "info", + `org-spec-audit: "${q.key}" reconciled onto ${match} — already asked, not re-filed`, + ); + } + } + + // 7. The declines ledger — the pre-emptive half, and the only read here + // that still fails OPEN. A key in it is never filed at all; if the file + // cannot be read the run files anyway, because the cost is one issue a + // human closes and that close then suppresses it for good. + // + // No `headBranchPrefix`: this run opens no PRs, so there is no PR + // history to date a cooldown from. The issue's own state is the memory. const suppression = yield* step("check-suppression", () => checkSuppression({ - keys: merged.map((q) => maintenanceKey(q.key)), - // The ledger and the proposals live in the same control repo, so one - // installation covers both reads. + keys: unfiled.map((q) => maintenanceKey(q.key)), ledgerRepo: controlRepo, ledgerPath: declinedPath, - headBranchPrefix: BRANCH_PREFIX, nowMs: input.firedAt, }), ); const allowed = new Set(suppression.allowed); - const proposed = merged.filter((q) => allowed.has(maintenanceKey(q.key))); + const fresh = unfiled.filter((q) => allowed.has(maintenanceKey(q.key))); - // Every question suppressed is a *good* tick, and a silent one — there is - // nothing new to ask. The count still lands in the output so a digest can - // say "0 new, 3 suppressed" rather than implying a quiet estate. - if (proposed.length === 0) { + // 8. Nothing fresh is the STEADY STATE, not a failure — every question + // the sweep raised is one somebody has already been asked. It is also + // the tick that used to open a duplicate PR, so the log line says which + // of the two reasons produced the silence. + if (fresh.length === 0) { yield* io.log( "info", - `org-spec-audit: ${merged.length} question(s), all suppressed — no PR opened`, + `org-spec-audit: ${merged.length} question(s), ${alreadyFiled.length} already on file, ` + + `${suppression.suppressed.length} declined — nothing to file`, ); return { reposSwept: swept, reposSkipped: skipped, questionsRaised: raised.length, questionsAfterMerge: merged.length, + questionsAlreadyFiled: alreadyFiled.length, questionsSuppressed: suppression.suppressed.length, - prOpened: false, + questionsFiled: 0, + questionsHeldByCap: 0, }; } - // 6. One control-plane PR against the configured control repo. The file - // it carries is the durable record — and the same text is what gets - // announced. - const message = renderMessage({ + // 9. File. One issue per question, capped, sequentially — this is a write + // of at most `maxNew`, and a stable order makes the notice's issue + // numbers read in the same order as its lines. + const toFile = fresh.slice(0, maxNew); + const heldByCap = fresh.length - toFile.length; + + const opened = yield* Effect.forEach( + toFile, + (q) => + step(`file-${q.key}`, () => + github.openIssue({ + repo: controlRepo, + title: issueTitle(q), + body: renderIssueBody({ question: q, day, declinedPath }), + labels: [questionsLabel, `${lanePrefix}${q.group}`], + }), + ).pipe(Effect.map((created) => ({ question: q, ...created }))), + { concurrency: 1 }, + ); + + // 10. Say what CHANGED. Not the standing list — a reader who has already + // seen eleven questions is not helped by being shown eleven questions, + // and a message whose length tracks the sweep rather than the news is + // one people stop opening. + const openOnFile = onFile.filter((i) => i.state === "open").length + opened.length; + const notice_ = renderNotice({ day, - merged: proposed, + opened, + openOnFile, + heldByCap, + alreadyFiled: alreadyFiled.length, outcomes, raised: raised.length, suppression, }); - const result = yield* step("open-questions-pr", () => - github.openDraftPullRequest({ - repo: controlRepo, - baseBranch, - headBranch: `${BRANCH_PREFIX}${day}`, - title: `docs(maintenance): open questions from the spec audit sweep (${day})`, - body: renderPrBody({ - day, - merged: proposed, - outcomes, - raised: raised.length, - suppression, - message, - declinedPath, - }), - commitMessage: `docs(maintenance): spec audit open questions (${day})\n\nGenerated by flare-dispatch org-spec-audit.`, - files: [ - { - path: `${questionsDir}/${day}.md`, - content: message, - }, - ], - }), - ); - // 7. Say it out loud. The same `message`, verbatim — the file and the - // announcement are one rendering on purpose, so nobody has to ask - // which of two wordings is the real one. It is the SUPPRESSION-FILTERED - // rendering, so a tick that proposes nothing new announces nothing new - // either. No markup is built here: - // `text` is data the receiver escapes, and the PR link rides in the - // typed `links` field precisely because markup inside `text` would be - // escaped along with everything else. - // - // `dedupeKey` is the day, which is also this run's schedule - // idempotency key. Deterministic per (run, day) and free of any clock - // read, so a retried step re-sends bytes the receiver has already - // claimed and gets a 409 instead of posting the digest twice. yield* step("publish-notice", () => notice.publish({ useCase: NOTICE_USE_CASE, dedupeKey: day, - text: message, - links: [{ url: result.url, label: "the questions PR" }], + text: notice_, + links: opened.map((o) => ({ url: o.url, label: `#${o.number}` })), }), ); yield* io.log( "info", - `org-spec-audit: ${proposed.length} question(s) from ${raised.length} raised (${suppression.suppressed.length} suppressed) — ${result.created ? "opened" : "updated"} PR #${result.number}`, + `org-spec-audit: filed ${opened.length} question(s) (${opened.map((o) => `#${o.number}`).join(", ")}) ` + + `from ${raised.length} raised — ${alreadyFiled.length} already on file, ` + + `${suppression.suppressed.length} declined, ${heldByCap} held by the cap`, ); return { @@ -510,8 +734,10 @@ export const orgSpecAudit = defineRun({ reposSkipped: skipped, questionsRaised: raised.length, questionsAfterMerge: merged.length, + questionsAlreadyFiled: alreadyFiled.length, questionsSuppressed: suppression.suppressed.length, - prOpened: result.created, + questionsFiled: opened.length, + questionsHeldByCap: heldByCap, }; }), }); @@ -722,8 +948,7 @@ const INVISIBLE = * Collapsing rather than escaping keeps the digest readable and costs nothing * real: the prompt already contracts each of these fields to a single sentence. */ -const oneLine = (raw: string): string => - raw.replace(INVISIBLE, "").replace(/\s+/g, " ").trim(); +const oneLine = (raw: string): string => raw.replace(INVISIBLE, "").replace(/\s+/g, " ").trim(); /** * The longest question-key that survives the round trip. @@ -763,6 +988,166 @@ export const parseWindowHours = (raw: string | undefined | null): number => { return Number.isFinite(n) && n > 0 ? n : WINDOW_HOURS_DEFAULT; }; +/** GitHub's own limit. A longer name is rejected at the API, not truncated here. */ +const LABEL_MAX_CHARS = 50; + +/** + * A label from config. Unset takes the default; **set-and-unusable is + * `undefined`**, which the caller turns into a failed run. + * + * Falling back on a bad value would be the worse of the two behaviours, and not + * by a little: this label is BOTH the filter the dedup read applies and the + * label the write applies. A comma makes those two different things — GitHub's + * list query joins labels on commas, so the read would filter on two labels + * while the write applied one — and the symptom is every question re-filing + * forever with nothing anywhere erroring. Same reasoning as the base ref. + */ +export const parseLabel = ( + raw: string | undefined | null, + fallback: string, +): string | undefined => { + if (raw === undefined || raw === null || raw.trim() === "") return fallback; + const v = raw.trim(); + if (v.includes(",") || v.length > LABEL_MAX_CHARS) return undefined; + return v; +}; + +/** A positive-integer config value, falling back when unset or unparseable. */ +export const parsePositiveInt = (raw: string | undefined | null, fallback: number): number => { + const n = Number.parseInt(raw ?? "", 10); + return Number.isFinite(n) && n > 0 ? n : fallback; +}; + +/** + * The FIRST `maintenance-key` line in an issue body, or `undefined`. + * + * First, not last, and not all of them: an issue body carries model prose + * derived from a swept repo, so a spec can contain a line that looks exactly + * like a trailer. `renderIssueBody` emits the authentic key as the body's first + * line for precisely this reason, so first-match is what makes a spoofed key + * inert rather than authoritative. + */ +export const firstMaintenanceKey = (body: string): string | undefined => { + for (const line of body.split("\n")) { + const m = /^maintenance-key:\s*(\S+)\s*$/.exec(line.trim()); + if (m?.[1] !== undefined) return m[1]; + } + return undefined; +}; + +/** + * Index the questions already on file, by `maintenance-key`. + * + * Keys outside this run's namespace are ignored, so another consumer sharing the + * questions label cannot make this run believe it has already asked something. + * An issue with no key at all is ignored too — a human-opened issue that happens + * to carry the label is a question this run did not ask and cannot match. + * + * On a duplicate key the first wins, which is the most recently updated (GitHub + * lists `sort=updated&direction=desc`). Which one wins does not matter to the + * caller — presence is the whole answer — but it should be deterministic. + */ +export const indexFiledQuestions = (issues: readonly IssueRef[]): Map => { + const byKey = new Map(); + for (const issue of issues) { + const key = firstMaintenanceKey(issue.body); + if (key === undefined || !key.startsWith(`${MAINTENANCE_SOURCE}/`)) continue; + if (!byKey.has(key)) byKey.set(key, issue); + } + return byKey; +}; + +/** Strip the `org-spec-audit/` namespace — the reconcile prompt speaks bare keys. */ +const bareKey = (namespaced: string): string => namespaced.slice(MAINTENANCE_SOURCE.length + 1); + +type ReconcileArgs = { + /** Today's questions with no exact key match — the only ones worth asking about. */ + readonly residue: readonly MergedQuestion[]; + readonly onFile: readonly IssueRef[]; + readonly resolved: { backend: string; model: string; mode: "tools" | "json" }; +}; + +/** One accepted match, as a plain row — see `reconcileKeys` on why not a Map. */ +type KeyMatch = { readonly minted: string; readonly existing: string }; + +/** + * Match today's un-filed questions against the ones already on file, returning + * one row per question that duplicates one. + * + * A plain array rather than a `Map`, because this runs inside `step()` and a + * step's result is checkpointed as JSON — a `Map` serializes to `{}`, which + * would silently mean "nothing matched" on a replay. The caller indexes it. + * + * **Every answer is checked against the set actually read from the control + * repo.** A model fully talked into "this duplicates `org-spec-audit/whatever`" + * produces no match, because `whatever` was never in the list — the same + * containment `closeIssueAsDuplicate`'s `knownNumbers` uses. The failure + * direction is deliberate: an unmatched duplicate costs a human one click, and + * an accepted hallucination is a question that is never asked again. + */ +const reconcileKeys = (args: ReconcileArgs) => + Effect.gen(function* () { + const known = new Map(); + for (const issue of args.onFile) { + const key = firstMaintenanceKey(issue.body); + if (key !== undefined) known.set(bareKey(key), key); + } + const minted = new Set(args.residue.map((q) => q.key)); + + const result = yield* completeStructured({ + backend: args.resolved.backend, + model: args.resolved.model, + mode: args.resolved.mode, + system: RECONCILE_PROMPT, + userBody: renderReconcileBody({ residue: args.residue, onFile: args.onFile }), + jsonContract: RECONCILE_JSON_CONTRACT, + schema: KeyReconciliation, + toolName: "report_key_matches", + toolDescription: 'For each newly-raised key, the on-file key it duplicates, or "".', + surface: "org-spec-audit", + maxTokens: RECONCILE_MAX_TOKENS, + }); + + const matches: KeyMatch[] = []; + const seen = new Set(); + for (const m of result.matches) { + const from = m.minted.trim(); + const onto = m.existing.trim(); + // A row about a key we did not ask about, or an on-file key that does not + // exist, decides nothing. Both are dropped silently rather than logged per + // row: a model listing a stale key is ordinary, and the interesting event + // (a question NOT filed because it matched) is logged by the caller. + if (onto === "" || !minted.has(from) || seen.has(from)) continue; + const resolvedKey = known.get(onto) ?? known.get(bareKey(onto)); + if (resolvedKey === undefined) continue; + seen.add(from); + matches.push({ minted: from, existing: resolvedKey }); + } + return matches; + }); + +/** The reconcile call's data half: what is on file, and what was just raised. */ +const renderReconcileBody = (args: { + readonly residue: readonly MergedQuestion[]; + readonly onFile: readonly IssueRef[]; +}): string => { + const filed = args.onFile.flatMap((issue) => { + const key = firstMaintenanceKey(issue.body); + if (key === undefined) return []; + return [ + `- key: ${bareKey(key)}\n question: ${oneLine(issue.title)}\n status: ${issue.state}`, + ]; + }); + + return [ + "## Questions already on file", + filed.length > 0 ? filed.join("\n") : "(none)", + "", + "## Questions raised just now", + args.residue.map((q) => `- key: ${q.key}\n question: ${q.question}`).join("\n"), + ].join("\n"); +}; + // --- In-container gather scripts (plain `git`, no extra CLI) ----------------- /** @@ -828,38 +1213,101 @@ const renderUserBody = (ctx: { const MARKER = ""; -type RenderArgs = { +/** GitHub truncates a longer title in its own UI; cut it where we can see it. */ +const MAX_TITLE_CHARS = 240; + +/** The issue title — the question as asked, which is what a reader scans. */ +export const issueTitle = (q: MergedQuestion): string => + q.question.length > MAX_TITLE_CHARS ? `${q.question.slice(0, MAX_TITLE_CHARS - 1)}…` : q.question; + +/** + * One question's issue body. + * + * **The trailer block comes first, and that order is load-bearing.** Everything + * below it is model output derived from the contents of a swept repo, so a spec + * crafted to make the model emit `maintenance-key: org-spec-audit/something` + * would — with the trailer last — put a spoofed key ahead of the real one for + * any reader that takes the first match. `indexFiledQuestions` takes exactly + * that first match, so emitting the authentic key first makes anything the model + * echoes inert text further down. + */ +export const renderIssueBody = (args: { + readonly question: MergedQuestion; readonly day: string; - /** The questions actually being proposed — suppressed ones are already out. */ - readonly merged: readonly MergedQuestion[]; + readonly declinedPath: string; +}): string => { + const q = args.question; + return ( + [ + `maintenance-key: ${maintenanceKey(q.key)}`, + MARKER, + "", + `> 🤖 Filed by \`flare-dispatch/org-spec-audit\` on ${args.day} — a divergence where` + + ` *which side is right* is a judgment nobody has made yet, not drift (\`spec-drift-pr\`` + + ` proposes those).`, + "", + `> **Answer in the thread and close this.** Closing is the record: a later sweep that` + + ` finds this question still unsettled will not re-file it and will never reopen it.` + + ` To keep it from ever being asked again, add its key to \`${args.declinedPath}\`.`, + "", + `**If nobody answers:** ${q.assumption}`, + "", + q.evidence, + "", + `Raised by: ${q.sources.map((s) => `\`${s.repo}\` (${s.specPath})`).join(" · ")}`, + ].join("\n") + "\n" + ); +}; + +/** One filed issue, as the notice reports it. */ +type OpenedQuestion = { + readonly question: MergedQuestion; + readonly number: number; + readonly url: string; +}; + +type NoticeArgs = { + readonly day: string; + /** Filed this tick — the news, and the only questions the notice lists. */ + readonly opened: readonly OpenedQuestion[]; + /** Open questions carrying the label after this tick, filed ones included. */ + readonly openOnFile: number; + readonly heldByCap: number; + readonly alreadyFiled: number; readonly outcomes: readonly RepoOutcome[]; readonly raised: number; - /** What suppression kept out, and whether either read degraded. */ readonly suppression: SuppressionReport; }; /** - * The file the PR carries — and the message a Slack consumer posts, verbatim. + * The announcement — the DELTA, not the standing list. + * + * The old rendering was the day's whole file, so its length tracked the sweep + * rather than the news and a reader who had already seen eleven questions was + * shown eleven questions again. This lists what was filed, counts what stands, + * and says what the cap held. * - * Written as GitHub markdown, not Slack mrkdwn: the canonical artifact is the - * reviewed file in git, and the Slack twin is derived at send time by whoever - * holds the token. + * Written as GitHub markdown, not Slack mrkdwn: the receiver holds the token and + * converts at send time, which is where escaping already lives. */ -export const renderMessage = (args: RenderArgs): string => { +export const renderNotice = (args: NoticeArgs): string => { const swept = args.outcomes.filter((o) => !o.skipped).map((o) => o.repo); const failed = args.outcomes.filter((o) => o.failure !== undefined); const quiet = args.outcomes .filter((o) => o.skipped && o.failure === undefined) .map((o) => o.repo); - const dropped = countDropped(args.merged); const lines: string[] = [ `# Spec audit — ${args.day}`, "", - `${swept.length} repo(s) swept · ${quiet.length} unchanged or without specs · ` + - `${failed.length} failed · ${args.merged.length} question(s) from ${args.raised} raised` + + `${args.opened.length} new question(s) · ${args.openOnFile} open · ` + + // The swept / unchanged / failed split stays in the headline: a failure + // counted as "unchanged" turns an outage into a quiet week, which is the + // sentence a reader is least likely to question. + `${swept.length} repo(s) swept · ${quiet.length} unchanged or without specs · ` + + `${failed.length} failed · ${args.raised} raised, ${args.alreadyFiled} already on file` + (args.suppression.suppressed.length > 0 - ? ` · ${args.suppression.suppressed.length} suppressed` + ? ` · ${args.suppression.suppressed.length} declined` : ""), "", // Immediately after the headline count, not in a footer: a reader who sees @@ -879,21 +1327,17 @@ export const renderMessage = (args: RenderArgs): string => { } for (const group of GROUPS) { - const inGroup = args.merged.filter((q) => q.group === group); + const inGroup = args.opened.filter((o) => o.question.group === group); if (inGroup.length === 0) continue; lines.push(`## ${GROUP_HEADING[group]} (${inGroup.length})`, ""); - for (const q of inGroup.slice(0, PER_GROUP_CAP)) { + for (const o of inGroup) { lines.push( - `- **${q.question}**`, - ` - ${q.evidence}`, - ` - raised by: ${q.sources.map((s) => `\`${s.repo}\` (${s.specPath})`).join(" · ")}`, - ` - if nobody answers: ${q.assumption}`, + `- **${o.question.question}** (#${o.number})`, + ` - raised by: ${o.question.sources.map((s) => `\`${s.repo}\` (${s.specPath})`).join(" · ")}`, + ` - if nobody answers: ${o.question.assumption}`, ); } - if (inGroup.length > PER_GROUP_CAP) { - lines.push(`- _${inGroup.length - PER_GROUP_CAP} more in this group, not shown._`); - } lines.push(""); } @@ -903,60 +1347,15 @@ export const renderMessage = (args: RenderArgs): string => { `Swept: ${swept.length > 0 ? swept.map((r) => `\`${r}\``).join(" · ") : "none"}`, `Unchanged or no \`specs/\`: ${quiet.length > 0 ? quiet.map((r) => `\`${r}\``).join(" · ") : "none"}`, `Failed: ${failed.length > 0 ? failed.map((o) => `\`${o.repo}\` (${o.failure})`).join(" · ") : "none"}`, - dropped > 0 ? `Below the per-group cap: ${dropped}` : "Nothing dropped by the cap.", + // Named, never silent: a list the cap shortened reads as fewer problems. + args.heldByCap > 0 + ? `Held by the per-sweep cap: ${args.heldByCap} — they file on the next sweep.` + : "Nothing held by the cap.", ); return `${lines.join("\n")}\n`; }; -/** How many merged questions the per-group cap keeps out of the message. */ -const countDropped = (merged: readonly MergedQuestion[]): number => - GROUPS.reduce((total, group) => { - const n = merged.filter((q) => q.group === group).length; - return total + Math.max(0, n - PER_GROUP_CAP); - }, 0); - -/** - * The PR body. Carries the loop's machine-readable lines plus the message - * itself, so a reviewer decides without opening the diff. - * - * **One `maintenance-key` line per question, not one per PR.** The key is what - * a later tick matches against the ledger and against this PR once it is - * closed, so it has to name the thing a human declines — a question. A dated - * per-PR key would be unique every day and suppress nothing, ever. - */ -const renderPrBody = ( - args: RenderArgs & { message: string; declinedPath: string }, -): string => - [ - "### Spec audit — the questions the sweep could not answer", - "", - "> 🤖 Draft opened by `flare-dispatch/org-spec-audit`. These are divergences where *which side is right* is a judgment nobody has made yet — not drift (`spec-drift-pr` proposes those). Answer in the thread or edit the file; merging records the answers.", - "", - `> Closing this unmerged suppresses every key below for 30 days. To suppress one permanently, add its key to \`${args.declinedPath}\` with a reason.`, - "", - // The trailers precede the message, and that order is load-bearing. Every - // line of `message` below is model output derived from the contents of the - // swept repos, so a spec crafted to make the model emit `auto-merge: yes` - // would, with the trailers last, put a spoofed value ahead of the real one - // for any consumer that reads the first match. Emitted first, the authentic - // trailers win and anything the model echoes is inert text further down. - ...args.merged.map((q) => `maintenance-key: ${maintenanceKey(q.key)}`), - `swept: ${ - args.outcomes - .filter((o) => !o.skipped) - .map((o) => o.repo) - .join(", ") || "none" - }`, - `suppressed: ${args.suppression.suppressed.length}`, - "auto-merge: never (specs are a sensitive path)", - MARKER, - "", - "---", - "", - args.message, - ].join("\n"); - /** The errors `sweepRepo`'s `catchAll` knows how to describe precisely. */ type CaughtError = | BackendUnconfigured From 5754a9d15e668536c3b04fa6ca059d5f072e3ef7 Mon Sep 17 00:00:00 2001 From: debuggingfuture Date: Tue, 18 Aug 2026 05:06:58 +0800 Subject: [PATCH 2/2] fix(org-spec-audit): a strict read needs its own ceiling, and a create needs its url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from pr-review, both real. `strict` inherited the library's 5-page default, so once the control repo held 500 label-filtered issues EVERY sweep would fail — permanently, five API calls in. The ledger read now asks for 20 pages, and the error names the remedy. Read the number as a canary: 2,000 questions asked and never closed is not a pagination problem, it is the loop having outrun the team, and a red run there beats one deciding on the first 500. It costs nothing until then — pagination stops on the first short page. `IssueCreated.url` was documented as never-empty and wasn't. A caller announces the issue by linking it, so `createIssue` now fails on a missing `html_url` the same way it fails on a missing number, rather than publishing a link to nowhere. The fake records `maxPages` and the run test asserts the raised ceiling, so a later edit dropping it cannot silently restore the five-page failure. --- packages/core/src/fakes/github-fake.ts | 5 +++-- packages/core/src/services/github.ts | 6 +++++- packages/github-app/src/issues.test.ts | 9 +++++++++ packages/github-app/src/issues.ts | 24 +++++++++++++++++++++--- runs/org-spec-audit.test.ts | 3 +++ runs/org-spec-audit.ts | 16 ++++++++++++++++ 6 files changed, 57 insertions(+), 6 deletions(-) diff --git a/packages/core/src/fakes/github-fake.ts b/packages/core/src/fakes/github-fake.ts index 64fc369..8bfc395 100644 --- a/packages/core/src/fakes/github-fake.ts +++ b/packages/core/src/fakes/github-fake.ts @@ -50,6 +50,7 @@ export type GithubFakeState = { state: "open" | "closed" | "all"; labels?: readonly string[]; updatedWithinDays?: number; + maxPages?: number; /** * Recorded, never simulated — the fake holds one page, so it can never * truncate. A dedup read's correctness depends on ASKING for `strict`, and @@ -168,9 +169,9 @@ export const makeGithubFake = ( const openedBranches = new Set(); const service: GithubService = { - issues: ({ repo, state: want = "open", labels, updatedWithinDays, strict }) => + issues: ({ repo, state: want = "open", labels, updatedWithinDays, maxPages, strict }) => Effect.sync(() => { - state.issuesCalls.push({ repo, state: want, labels, updatedWithinDays, strict }); + state.issuesCalls.push({ repo, state: want, labels, updatedWithinDays, maxPages, strict }); const need = labels === undefined ? undefined : new Set(labels); return state.issues.filter((i) => { if (i.repo !== repo) return false; diff --git a/packages/core/src/services/github.ts b/packages/core/src/services/github.ts index 7e3574d..f62253a 100644 --- a/packages/core/src/services/github.ts +++ b/packages/core/src/services/github.ts @@ -64,7 +64,11 @@ export type IssueRef = { /** The outcome of {@link GithubService.openIssue}. */ export type IssueCreated = { readonly number: number; - /** The issue's web URL — what a notice links, so it is never empty on success. */ + /** + * The issue's web URL. Never empty: a caller announces the issue by linking + * it, so a create that came back without one fails rather than publishing a + * link to nowhere. + */ readonly url: string; }; diff --git a/packages/github-app/src/issues.test.ts b/packages/github-app/src/issues.test.ts index c31b5cc..cd680b8 100644 --- a/packages/github-app/src/issues.test.ts +++ b/packages/github-app/src/issues.test.ts @@ -272,6 +272,15 @@ describe("createIssue", () => { ); }); + it("fails when the create returns no url — a notice would link nowhere", async () => { + server.use( + http.post("https://api.github.com/repos/:owner/:repo/issues", () => + HttpResponse.json({ number: 41 }, { status: 201 }), + ), + ); + await expect(createIssue({ ...base, title: "t", body: "b" })).rejects.toThrow(/no html_url/); + }); + it("surfaces a non-2xx", async () => { server.use( http.post("https://api.github.com/repos/:owner/:repo/issues", () => diff --git a/packages/github-app/src/issues.ts b/packages/github-app/src/issues.ts index 288d576..99bde2e 100644 --- a/packages/github-app/src/issues.ts +++ b/packages/github-app/src/issues.ts @@ -255,10 +255,17 @@ export const listIssues = async (opts: ListIssuesOptions): Promise { expect(call).toMatchObject({ repo: "owner/control", state: "all", strict: true }); expect(call!.labels).toEqual([LABEL]); expect(call!.updatedWithinDays).toBeUndefined(); + // And a ceiling far above the library default, because `strict` turns an + // outgrown ceiling into a permanent failure rather than a short list. + expect(call!.maxPages).toBeGreaterThanOrEqual(20); }).pipe(Effect.provide(layer)); }); diff --git a/runs/org-spec-audit.ts b/runs/org-spec-audit.ts index 9aa49c8..164399d 100644 --- a/runs/org-spec-audit.ts +++ b/runs/org-spec-audit.ts @@ -196,6 +196,21 @@ const LANE_LABEL_PREFIX_DEFAULT = "question:"; */ const MAX_NEW_QUESTIONS_DEFAULT = 5; +/** + * Page ceiling for the ledger read — 2,000 label-filtered issues. + * + * Well above the library's default of 5 pages, because this read is `strict` and + * a strict read that outgrows its ceiling fails EVERY tick until someone raises + * it. It costs nothing until the set is that large: pagination stops on the first + * short page, so today it is one request. + * + * Read the number as a canary rather than a limit. Two thousand questions asked + * and never closed is not a pagination problem — it is [§8]'s failure mode + * arrived, the loop having asked more than the team answers, and a run going red + * there is closer to correct than a run quietly deciding on the first 500. + */ +const LEDGER_MAX_PAGES = 20; + /** The `maintenance-key` namespace every question is suppressed by. */ const MAINTENANCE_SOURCE = "org-spec-audit"; @@ -515,6 +530,7 @@ export const orgSpecAudit = defineRun({ state: "all", labels: [questionsLabel], strict: true, + maxPages: LEDGER_MAX_PAGES, }), ); const filed = indexFiledQuestions(onFile);