From 8bd53faa6cbcf8e440ada78c41303682bb090cd8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 29 Sep 2026 19:15:06 +0000 Subject: [PATCH] test(eng-batching): bind unsourced native briefs through the report's target Run 36606688266 asked ten separate native review questions (D1-D9 bound to ledger records R1-R9) and failed reviewCount=0 < FLOOR=3: its briefs named the plan by title instead of citing PLAN.md, its report declared 'Review target (fixed): PLAN.md' under '# Engineering review: ', and it kept an unfenced copy of the plan's own H1. The named-source route now accepts those spellings and non-inline ledger briefs. The same replay rejects a foreign, mixed, duplicate or missing target, another plan's title or copied H1, a brief naming another plan or file, a mismatched saved brief, and re-asks. The run-36597762183 capture still counts 3. --- test/eng-seeded-coverage.test.ts | 86 ++++- ...-batching-unsourced-brief-36606688266.json | 311 ++++++++++++++++++ test/helpers/eng-seeded-coverage.ts | 23 +- 3 files changed, 409 insertions(+), 11 deletions(-) create mode 100644 test/fixtures/eng-batching-unsourced-brief-36606688266.json diff --git a/test/eng-seeded-coverage.test.ts b/test/eng-seeded-coverage.test.ts index 66a2d07cc..262cd5e3c 100644 --- a/test/eng-seeded-coverage.test.ts +++ b/test/eng-seeded-coverage.test.ts @@ -1,7 +1,11 @@ import { describe, expect, test } from 'bun:test'; import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; -import { isEngBatchingIssueAUQ } from './helpers/eng-seeded-coverage'; -import { nativePlanCallFingerprint } from './helpers/claude-pty-runner'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { createEngBatchingIssueCounter, isEngBatchingIssueAUQ } from './helpers/eng-seeded-coverage'; +import { engSetupAUQ, hasCompletePlanReport, nativePlanCallFingerprint } from './helpers/claude-pty-runner'; +import batchingCapture from './fixtures/eng-batching-unsourced-brief-36606688266.json'; function question(call: NativePlanQuestionCall, text: string) { const answer = call.answers![call.questions[0]!.question]!; @@ -78,3 +82,81 @@ describe('batching caller counts completed issue decisions across setup boundari expect(check(quoted)).toBe(true); }); }); + +describe('batching replay of run 36606688266 (unsourced native briefs)', () => { + // Run 36606688266 asked one native question per finding (D1-D9 bound to + // ledger records R1-R9, D10 a TODO follow-up) but cited no PLAN.md line in the + // native brief, so the old detector counted zero review decisions. + const FLOOR = 3; + const calls = batchingCapture.calls as unknown as NativePlanQuestionCall[]; + + function count(plan: string, edit: (calls: NativePlanQuestionCall[]) => void = () => {}) { + const copy = structuredClone(calls); + edit(copy); + const counter = createEngBatchingIssueCounter(() => plan, engSetupAUQ); + const counted = copy.filter((call, index) => counter.isReviewAUQ(nativePlanCallFingerprint(call, 0, true), copy.slice(0, index))); + return { counted: counted.length, issues: counter.trace.map(entry => entry.issue) }; + } + + test('the recorded failing verdict is the detector, not the review', () => { + expect(batchingCapture.recordedOutcome).toEqual({ outcome: 'completion_summary', step0Count: 10, reviewCount: 0 }); + expect(calls.every(call => call.answered && call.questions.length === 1)).toBe(true); + }); + + test('each ledger-bound native decision counts once without a native source citation', () => { + const { counted, issues } = count(batchingCapture.plan); + expect(issues).toEqual(['R1', 'R2', 'R3', 'R4', 'R5', 'R6', 'R7', 'R8', 'R9'].map(id => `record:${id}`)); + expect(counted).toBeGreaterThanOrEqual(FLOOR); + }); + + test('a re-asked decision cannot inflate the count', () => { + const { counted } = count(batchingCapture.plan, all => { + const again = structuredClone(all[0]!); + again.toolUseId += '-again'; + all.splice(1, 0, again); + }); + expect(counted).toBe(9); + }); + + const target = 'Review target (fixed): `PLAN.md`'; + for (const [name, plan] of [ + ['a foreign target', batchingCapture.plan.replace(target, 'Review target (fixed): `OTHER.md`')], + ['a mixed target', batchingCapture.plan.replace(target, 'Review target (fixed): `OTHER.md` and `PLAN.md`')], + ['two target declarations', batchingCapture.plan.replace(target, `${target}\nReview target (fixed): \`PLAN.md\``)], + ['no target declaration', batchingCapture.plan.replace(target, 'Report scope: the fixture repo')], + ['a report title for another plan', batchingCapture.plan.replace('# Engineering review: Add background job retry framework', '# Engineering review: Replace all customer data')], + ['an archived report title', batchingCapture.plan.replace('# Engineering review:', '# Archived engineering review:')], + ['a copied H1 naming another plan', batchingCapture.plan.replace('# Plan: Add background job retry framework', '# Plan: Replace all customer data')], + ] as const) test(`the unsourced route rejects ${name}`, () => { + expect(count(plan).counted).toBe(0); + }); + + test('the unsourced route rejects a native brief naming another plan or file', () => { + const rename = (from: string, to: string) => (all: NativePlanQuestionCall[]) => { + for (const call of all) call.questions[0]!.question = call.questions[0]!.question.replace(from, to); + }; + expect(count(batchingCapture.plan, rename('plan "Add background job retry framework"', 'plan "Replace all customer data"')).counted).toBe(0); + expect(count(batchingCapture.plan, rename('plan "Add background job retry framework"', 'plan "Add background job retry framework", OTHER.md')).counted).toBe(0); + expect(count(batchingCapture.plan, rename('plan "Add background job retry framework"', 'the plan')).counted).toBe(0); + }); + + test('a saved record whose brief title differs from the native question does not bind it', () => { + const plan = batchingCapture.plan.replace(/^Question D1:\n.*$/m, 'Question D1:\nD1 — Some other decision?'); + expect(count(plan).issues).not.toContain('record:R1'); + }); + + test('the completed report is the early outcome point; a partial report is not', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'eng-batching-report-')); + try { + const report = path.join(dir, 'report.md'); + fs.writeFileSync(report, batchingCapture.plan); + expect(hasCompletePlanReport(report, 0, Date.now() + 1_000)).toBe(true); + fs.writeFileSync(report, batchingCapture.plan.slice(0, batchingCapture.plan.indexOf('## Completion summary'))); + expect(hasCompletePlanReport(report, 0, Date.now() + 1_000)).toBe(false); + fs.writeFileSync(report, batchingCapture.plan.replace('## GSTACK REVIEW REPORT', '```\n## GSTACK REVIEW REPORT') + '\n```\n'); + expect(hasCompletePlanReport(report, 0, Date.now() + 1_000)).toBe(false); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/test/fixtures/eng-batching-unsourced-brief-36606688266.json b/test/fixtures/eng-batching-unsourced-brief-36606688266.json new file mode 100644 index 000000000..661f0fc2e --- /dev/null +++ b/test/fixtures/eng-batching-unsourced-brief-36606688266.json @@ -0,0 +1,311 @@ +{ + "source": "run 36606688266 plan-eng-multi-finding-batching observation.json (transcript.calls) and the saved report replayed from its Write/Edit inputs", + "recordedOutcome": { + "outcome": "completion_summary", + "step0Count": 10, + "reviewCount": 0 + }, + "calls": [ + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_011vfjpt7sgeSRMw1zhHATN8", + "questions": [ + { + "question": "D1 — Use the job library's retry hooks or roll a custom inline scheduler?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: The job library you already run has a retry system built in. The plan wants to rebuild the same thing by hand inside each of the 5 workers, only so the delay curve can be tuned. Retry systems are easy to get subtly wrong: losing the attempt count when a worker restarts, retrying forever, or retrying twice at once. The library has already paid for those bugs; a hand-rolled copy pays for them again in production.\nStakes if we pick wrong: a hand-rolled scheduler that drops attempt state on restart or double-schedules turns one failed job into duplicate side effects or an infinite retry loop, with no dead-letter to catch it.\nRecommendation: A because the plan admits the shapes are identical, and the curve is pluggable in the library through a strategy function, so A delivers the same control with far less new code. (human: ~1 day / CC: ~20 min for A; human: ~1 week / CC: ~2 h for B plus ongoing ownership)\nCompleteness: A=9/10, B=5/10\nPros / cons:\nA) Library hooks + custom curve (recommended)\n ✅ Attempt counting, persistence across restarts and terminal handling come from tested library code, not new code\n ✅ The custom curve still lives in one strategy function, so \"full control over the curve\" is preserved\n ❌ Requires confirming the library's hook accepts a custom delay function; if it does not, we fall back to B for the curve only\nB) Custom inline scheduler\n ✅ Zero dependency on the library's retry semantics or its upgrade cadence\n ✅ Any curve shape, any bookkeeping, no hook constraints\n ❌ Rebuilds attempt state, restart persistence, concurrency guards and dead-lettering by hand, and those are the parts that fail at 3am\nNet: you are trading a one-line strategy function against owning a second retry engine forever.", + "header": "Retry engine", + "multiSelect": false, + "options": [ + { + "label": "Library hooks + custom curve (recommended)", + "description": "Register the exponential-backoff curve as one custom backoff strategy function with the job library's built-in retry hooks. Attempt counting, persistence across worker restarts, and terminal/dead-letter handling come from the library. Verify the hook accepts a delay function first; if it does not, fall back to a custom curve only for delay computation while keeping library scheduling. R2-R5 stay pending. Completeness 9/10. human: ~1 day / CC: ~20 min." + }, + { + "label": "Custom inline scheduler", + "description": "Keep the plan as written: a custom exponential-backoff scheduler inline in each worker, bypassing the library's retry hooks. Attempt state, restart persistence, concurrency guards and terminal handling must be designed and tested by hand (tracked under R3). R2-R5 stay pending. Completeness 5/10. human: ~1 week / CC: ~2 h plus ongoing ownership." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Use the job library's retry hooks or roll a custom inline scheduler?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: The job library you already run has a retry system built in. The plan wants to rebuild the same thing by hand inside each of the 5 workers, only so the delay curve can be tuned. Retry systems are easy to get subtly wrong: losing the attempt count when a worker restarts, retrying forever, or retrying twice at once. The library has already paid for those bugs; a hand-rolled copy pays for them again in production.\nStakes if we pick wrong: a hand-rolled scheduler that drops attempt state on restart or double-schedules turns one failed job into duplicate side effects or an infinite retry loop, with no dead-letter to catch it.\nRecommendation: A because the plan admits the shapes are identical, and the curve is pluggable in the library through a strategy function, so A delivers the same control with far less new code. (human: ~1 day / CC: ~20 min for A; human: ~1 week / CC: ~2 h for B plus ongoing ownership)\nCompleteness: A=9/10, B=5/10\nPros / cons:\nA) Library hooks + custom curve (recommended)\n ✅ Attempt counting, persistence across restarts and terminal handling come from tested library code, not new code\n ✅ The custom curve still lives in one strategy function, so \"full control over the curve\" is preserved\n ❌ Requires confirming the library's hook accepts a custom delay function; if it does not, we fall back to B for the curve only\nB) Custom inline scheduler\n ✅ Zero dependency on the library's retry semantics or its upgrade cadence\n ✅ Any curve shape, any bookkeeping, no hook constraints\n ❌ Rebuilds attempt state, restart persistence, concurrency guards and dead-lettering by hand, and those are the parts that fail at 3am\nNet: you are trading a one-line strategy function against owning a second retry engine forever.": "Library hooks + custom curve (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:46:48.252Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_0182pLbYfj4VpsRE57iBzMYQ", + "questions": [ + { + "question": "D2 — What delivery guarantee does `processWebhookJob()` keep once it retries?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Today a webhook is sent once; if it fails, it is gone (at-most-once). The moment you retry, a request that timed out after the customer already received it gets sent again, so the customer sees the same event twice. You have to pick: either only retry when you are sure the request never left, or retry freely but stamp every attempt with the same id so the customer can ignore repeats. The plan does neither and just retries.\nStakes if we pick wrong: customers process duplicate events (double orders, double emails) with no way to detect them, or you ship a retry feature that almost never fires because most webhook failures are timeouts.\nRecommendation: B because it is the standard webhook contract (retry on timeout/5xx, stable event id per attempt) and is the only option where retrying actually improves delivery while giving receivers a way to dedupe. This is a receiver-visible contract change; A is the right pick if you cannot communicate it to receivers.\nCompleteness: A=7/10, B=9/10, C=3/10\nPros / cons:\nA) Keep at-most-once\n ✅ No change to what receivers see; the existing guarantee and its regression test stay valid as-is\n ✅ Smallest blast radius: no new headers, no receiver communication needed\n ❌ Retries only fire on connect/DNS/pre-send errors; timeouts and 5xx go straight to terminal, so most real failures are still not retried\nB) At-least-once + idempotency key (recommended)\n ✅ Timeouts and 5xx are retried, so delivery reliability actually improves for receivers\n ✅ Same delivery id on every attempt lets receivers dedupe; this is the contract Stripe/GitHub-style webhooks use\n ❌ Receiver-visible contract change: duplicates become possible and receivers must be told to dedupe on the id\nC) Plain retry (plan as written)\n ✅ Least code: no header, no classification, just retry on any failure\n ✅ Ships fastest\n ❌ Duplicates reach receivers with no way to tell them apart; silent double side effects\nNet: A keeps the promise but retries little; B changes the promise but makes retries worth having; C breaks the promise silently.", + "header": "Webhook semantics", + "multiSelect": false, + "options": [ + { + "label": "Keep at-most-once", + "description": "Preserve at-most-once for `processWebhookJob()`. Retry only failures where the request provably never left the process (connection refused, DNS failure, errors before send). Timeouts, 5xx and any post-send ambiguity are terminal and go to whatever terminal handling R3 decides. No new headers; receiver contract unchanged. R3, R4, R5, R7 stay pending. Completeness 7/10. human: ~half day / CC: ~10 min." + }, + { + "label": "At-least-once + idempotency key (recommended)", + "description": "Retry timeouts and 5xx as well as pre-send failures. Every attempt carries the same stable delivery id (e.g. an `Idempotency-Key`/event-id header derived from the job id, generated once, persisted with the job). 4xx other than 408/429 is non-retryable for webhooks. Document to receivers that duplicates are possible and must be deduped on the id. R3, R4, R5, R7 stay pending. Completeness 9/10. human: ~1 day / CC: ~20 min." + }, + { + "label": "Plain retry (plan as written)", + "description": "Retry `processWebhookJob()` on any failure with no delivery id and no classification. Duplicates reach receivers undetectably. R3, R4, R5, R7 stay pending. Completeness 3/10. human: ~1 h / CC: ~5 min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 — What delivery guarantee does `processWebhookJob()` keep once it retries?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Today a webhook is sent once; if it fails, it is gone (at-most-once). The moment you retry, a request that timed out after the customer already received it gets sent again, so the customer sees the same event twice. You have to pick: either only retry when you are sure the request never left, or retry freely but stamp every attempt with the same id so the customer can ignore repeats. The plan does neither and just retries.\nStakes if we pick wrong: customers process duplicate events (double orders, double emails) with no way to detect them, or you ship a retry feature that almost never fires because most webhook failures are timeouts.\nRecommendation: B because it is the standard webhook contract (retry on timeout/5xx, stable event id per attempt) and is the only option where retrying actually improves delivery while giving receivers a way to dedupe. This is a receiver-visible contract change; A is the right pick if you cannot communicate it to receivers.\nCompleteness: A=7/10, B=9/10, C=3/10\nPros / cons:\nA) Keep at-most-once\n ✅ No change to what receivers see; the existing guarantee and its regression test stay valid as-is\n ✅ Smallest blast radius: no new headers, no receiver communication needed\n ❌ Retries only fire on connect/DNS/pre-send errors; timeouts and 5xx go straight to terminal, so most real failures are still not retried\nB) At-least-once + idempotency key (recommended)\n ✅ Timeouts and 5xx are retried, so delivery reliability actually improves for receivers\n ✅ Same delivery id on every attempt lets receivers dedupe; this is the contract Stripe/GitHub-style webhooks use\n ❌ Receiver-visible contract change: duplicates become possible and receivers must be told to dedupe on the id\nC) Plain retry (plan as written)\n ✅ Least code: no header, no classification, just retry on any failure\n ✅ Ships fastest\n ❌ Duplicates reach receivers with no way to tell them apart; silent double side effects\nNet: A keeps the promise but retries little; B changes the promise but makes retries worth having; C breaks the promise silently.": "Keep at-most-once" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:48:33.604Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_012ueyTcioa6A2y9YE4gYPcR", + "questions": [ + { + "question": "D3 — How many times may a job retry, and where does it go when it gives up?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: A retry curve without a stopping point is a job that runs forever when the thing it depends on is down for good. You need a maximum number of tries, and you need a place for jobs that used up their tries (a dead-letter set) so someone can look at them and replay them later. Otherwise failed work quietly disappears or quietly never stops.\nStakes if we pick wrong: either a poisoned job hammers a downstream forever and starves healthy jobs, or failed webhooks and jobs vanish with only a log line nobody reads.\nRecommendation: A because the library already provides the failed set, so the dead-letter and alert cost a config line and one log call, and it is the only option where an operator can find and replay a lost job.\nCompleteness: A=9/10, B=6/10, C=2/10\nPros / cons:\nA) Bounded + dead-letter + alert (recommended)\n ✅ Exhausted jobs are inspectable and replayable from the library's failed set, with the last error attached\n ✅ One structured log line plus a metric on dead-letter entry makes a downstream outage visible within minutes\n ❌ Needs a per-worker ceiling value and a dead-letter retention/cleanup policy to be chosen and documented\nB) Bounded + log-and-drop\n ✅ Bounds the retry loop with the least configuration\n ✅ No dead-letter retention to manage\n ❌ A dropped job is gone; the only trace is a log line, so replay after an outage is impossible\nC) Unbounded (plan as written)\n ✅ No ceiling to tune; a job eventually succeeds if the dependency ever recovers\n ✅ Zero extra code\n ❌ Permanently failing jobs retry forever, consume worker capacity and never surface as a problem\nNet: you are choosing whether a job that cannot succeed becomes a visible artifact, a log line, or a permanent background load.", + "header": "Attempt ceiling", + "multiSelect": false, + "options": [ + { + "label": "Bounded + dead-letter + alert (recommended)", + "description": "Set a maximum attempt count per worker (default 5, overridable per worker, configured in the same place as the backoff strategy). On exhaustion or on a non-retryable error, the job lands in the library's dead-letter/failed set with its last error; emit one structured error log and a metric on entry. Webhook timeouts/5xx (terminal per R2) land here too. Document the retention/replay procedure. R4, R5 stay pending. Completeness 9/10. human: ~half day / CC: ~15 min." + }, + { + "label": "Bounded + log-and-drop", + "description": "Set the same per-worker maximum attempt count (default 5). On exhaustion, log the error and drop the job; no dead-letter set, no metric, no replay. R4, R5 stay pending. Completeness 6/10. human: ~1 h / CC: ~5 min." + }, + { + "label": "Unbounded (plan as written)", + "description": "No attempt ceiling; the exponential curve continues until the job succeeds. No terminal handling. Conflicts with R2, which needs a terminal destination for webhook timeouts. R4, R5 stay pending. Completeness 2/10. human: 0 / CC: 0." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 — How many times may a job retry, and where does it go when it gives up?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: A retry curve without a stopping point is a job that runs forever when the thing it depends on is down for good. You need a maximum number of tries, and you need a place for jobs that used up their tries (a dead-letter set) so someone can look at them and replay them later. Otherwise failed work quietly disappears or quietly never stops.\nStakes if we pick wrong: either a poisoned job hammers a downstream forever and starves healthy jobs, or failed webhooks and jobs vanish with only a log line nobody reads.\nRecommendation: A because the library already provides the failed set, so the dead-letter and alert cost a config line and one log call, and it is the only option where an operator can find and replay a lost job.\nCompleteness: A=9/10, B=6/10, C=2/10\nPros / cons:\nA) Bounded + dead-letter + alert (recommended)\n ✅ Exhausted jobs are inspectable and replayable from the library's failed set, with the last error attached\n ✅ One structured log line plus a metric on dead-letter entry makes a downstream outage visible within minutes\n ❌ Needs a per-worker ceiling value and a dead-letter retention/cleanup policy to be chosen and documented\nB) Bounded + log-and-drop\n ✅ Bounds the retry loop with the least configuration\n ✅ No dead-letter retention to manage\n ❌ A dropped job is gone; the only trace is a log line, so replay after an outage is impossible\nC) Unbounded (plan as written)\n ✅ No ceiling to tune; a job eventually succeeds if the dependency ever recovers\n ✅ Zero extra code\n ❌ Permanently failing jobs retry forever, consume worker capacity and never surface as a problem\nNet: you are choosing whether a job that cannot succeed becomes a visible artifact, a log line, or a permanent background load.": "Bounded + dead-letter + alert (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:50:10.303Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_016fgg5mWHBjFGWxQPRz9Whr", + "questions": [ + { + "question": "D4 — Add jitter to the backoff curve, or keep it deterministic?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: When a downstream service goes down, hundreds of jobs across all 5 workers fail at the same moment. With a pure exponential curve they all wake up at exactly the same moment too, and hit the recovering service as one wave, which can knock it over again. Jitter adds a random spread to each delay so the retries trickle back instead of stampeding.\nStakes if we pick wrong: a downstream that recovers from an outage gets re-flattened by your own synchronized retry wave, turning a 2-minute blip into a 20-minute incident.\nRecommendation: A because it is two lines inside the strategy function you already own and it is the standard mitigation for retry storms; deterministic curves are only useful in tests, which can seed or stub the random source.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Equal jitter (recommended)\n ✅ Retries after a shared outage spread across the window instead of returning as one synchronized burst\n ✅ Lives inside the single strategy function from D1, so every worker gets it with no per-worker code\n ❌ Curve tests need an injectable random source to stay deterministic\nB) No jitter (pure curve)\n ✅ Exact, predictable retry times that are easy to reason about and assert in tests\n ✅ Zero extra code beyond the curve itself\n ❌ All jobs that fail together retry together, so the retry framework itself becomes a traffic amplifier during outages\nNet: predictability in tests against stampede protection in production; the test cost is one injected random source.", + "header": "Jitter", + "multiSelect": false, + "options": [ + { + "label": "Equal jitter (recommended)", + "description": "Inside the single backoff strategy function, compute the exponential delay and return half of it plus a random amount up to the other half (equal jitter). The random source is injectable so tests can pin it. Required proof: unit test that returned delays fall within [curve/2, curve] for each attempt, and that a pinned random source gives a deterministic value. R5 stays pending. Completeness 9/10. human: ~1 h / CC: ~5 min." + }, + { + "label": "No jitter (pure curve)", + "description": "Return the exact exponential curve value with no random spread. Required proof: unit test of the exact value per attempt. R5 stays pending. Completeness 6/10. human: ~30 min / CC: ~3 min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 — Add jitter to the backoff curve, or keep it deterministic?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: When a downstream service goes down, hundreds of jobs across all 5 workers fail at the same moment. With a pure exponential curve they all wake up at exactly the same moment too, and hit the recovering service as one wave, which can knock it over again. Jitter adds a random spread to each delay so the retries trickle back instead of stampeding.\nStakes if we pick wrong: a downstream that recovers from an outage gets re-flattened by your own synchronized retry wave, turning a 2-minute blip into a 20-minute incident.\nRecommendation: A because it is two lines inside the strategy function you already own and it is the standard mitigation for retry storms; deterministic curves are only useful in tests, which can seed or stub the random source.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Equal jitter (recommended)\n ✅ Retries after a shared outage spread across the window instead of returning as one synchronized burst\n ✅ Lives inside the single strategy function from D1, so every worker gets it with no per-worker code\n ❌ Curve tests need an injectable random source to stay deterministic\nB) No jitter (pure curve)\n ✅ Exact, predictable retry times that are easy to reason about and assert in tests\n ✅ Zero extra code beyond the curve itself\n ❌ All jobs that fail together retry together, so the retry framework itself becomes a traffic amplifier during outages\nNet: predictability in tests against stampede protection in production; the test cost is one injected random source.": "Equal jitter (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:51:07.968Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_01AGgMxL2cDbt1vqtMqNz8dH", + "questions": [ + { + "question": "D5 — Should workers name errors that must not be retried, or retry every failure to the ceiling?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Some failures fix themselves if you wait (a database hiccup, a slow API). Others never will (a payload that fails validation, a revoked API key). Retrying the second kind five times with growing delays just wastes capacity and delays the moment someone notices. Letting each worker say \"these error types are permanent\" sends them straight to the dead-letter set on the first try.\nStakes if we pick wrong: a bad payload burns 5 attempts and up to the full backoff window before it surfaces, and during a bad deploy every job does this at once.\nRecommendation: A because it is a small per-worker list, the dead-letter path already exists from D3, and it turns a permanent failure into an immediate signal instead of a delayed one. Medium confidence on which types are permanent; verify against the actual error classes when implementing.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Explicit non-retryable list (recommended)\n ✅ Permanent failures reach the dead-letter set on attempt 1, so operators see bad payloads or revoked credentials within seconds\n ✅ Unknown errors still default to retry, so nothing transient is accidentally dropped\n ❌ Each worker needs a short, reviewed list of permanent error types, and a wrong entry makes a transient error permanent\nB) Retry everything to ceiling\n ✅ No classification to get wrong; behavior is identical for every worker\n ✅ Nothing to maintain when new error types appear\n ❌ Permanent failures consume the full attempt budget and backoff window before anyone can see them\nNet: a short reviewed list per worker against a guaranteed delay on every permanent failure.", + "header": "Error classes", + "multiSelect": false, + "options": [ + { + "label": "Explicit non-retryable list (recommended)", + "description": "Each of the 4 non-webhook workers declares its non-retryable error types (validation errors, auth/permission errors, malformed payload). Those bypass retry and land in the dead-letter set (R3) on attempt 1 with the error attached. Any error not on the list retries per R3/R4. Required proof: per worker, one test that a listed error goes to dead-letter without a retry, and one test that an unlisted error retries. Completeness 9/10. human: ~half day / CC: ~15 min." + }, + { + "label": "Retry everything to ceiling", + "description": "No classification. Every failure in the 4 non-webhook workers retries per R3/R4 until the ceiling, then lands in dead-letter. Required proof: covered by R3 tests. Completeness 6/10. human: 0 / CC: 0." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 — Should workers name errors that must not be retried, or retry every failure to the ceiling?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Some failures fix themselves if you wait (a database hiccup, a slow API). Others never will (a payload that fails validation, a revoked API key). Retrying the second kind five times with growing delays just wastes capacity and delays the moment someone notices. Letting each worker say \"these error types are permanent\" sends them straight to the dead-letter set on the first try.\nStakes if we pick wrong: a bad payload burns 5 attempts and up to the full backoff window before it surfaces, and during a bad deploy every job does this at once.\nRecommendation: A because it is a small per-worker list, the dead-letter path already exists from D3, and it turns a permanent failure into an immediate signal instead of a delayed one. Medium confidence on which types are permanent; verify against the actual error classes when implementing.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Explicit non-retryable list (recommended)\n ✅ Permanent failures reach the dead-letter set on attempt 1, so operators see bad payloads or revoked credentials within seconds\n ✅ Unknown errors still default to retry, so nothing transient is accidentally dropped\n ❌ Each worker needs a short, reviewed list of permanent error types, and a wrong entry makes a transient error permanent\nB) Retry everything to ceiling\n ✅ No classification to get wrong; behavior is identical for every worker\n ✅ Nothing to maintain when new error types appear\n ❌ Permanent failures consume the full attempt budget and backoff window before anyone can see them\nNet: a short reviewed list per worker against a guaranteed delay on every permanent failure.": "Explicit non-retryable list (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:52:12.006Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_016w6Sj4WK8TpN3bzensgKcm", + "questions": [ + { + "question": "D6 — Extract one shared retry policy module now, or keep 5 copy-pasted envelopes?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: You just decided the curve shape, jitter, the attempt ceiling, the dead-letter alert and how errors are classified. Each of those has to live somewhere. If the retry envelope stays copy-pasted in 5 workers, every one of those decisions is copied 5 times, and the first bug fix or tuning change has to be made in 5 places and tested 5 times. One small shared module applies each decision once and every worker gets it.\nStakes if we pick wrong: a curve or ceiling bug fixed in 4 of 5 workers, or a jitter change that lands in 3, and nobody notices until the fifth worker stampedes a downstream.\nRecommendation: A because the plan already states the 5 bodies are identical, the module is about 50 lines, the migration is mechanical per worker, and doing the refactor commit before the behavior commit keeps each step reviewable. (human: ~1 day / CC: ~30 min)\nCompleteness: A=9/10, B=6/10, C=3/10\nPros / cons:\nA) Extract now, migrate all 5 (recommended)\n ✅ Curve, jitter, ceiling, dead-letter alert and classifier shape are each implemented and tested exactly once\n ✅ Refactor commit lands before the behavior-change commit, so each is small and reviewable on its own\n ❌ A defect in the shared module affects all 5 workers at once; the contract tests are the guard\nB) Extract now, migrate webhook only\n ✅ Smallest first step; proves the module against the worker whose behavior is changing anyway\n ✅ Other 4 workers are untouched in this change, so their risk is zero for now\n ❌ Leaves 4 copies carrying the new policy by hand, so the duplication the plan already called out gets worse, not better\nC) Leave duplication\n ✅ No refactor risk in this change at all\n ✅ Matches the plan as written\n ❌ Every approved policy decision is copy-pasted 5 times and drifts from the first fix onward\nNet: one 50-line module now against 5 hand-maintained copies of every retry decision.", + "header": "Shared module", + "multiSelect": false, + "options": [ + { + "label": "Extract now, migrate all 5 (recommended)", + "description": "Create one `retryPolicy` module exporting backoffStrategy(attempt, rng) with equal jitter and a configurable max-delay clamp, DEFAULT_MAX_ATTEMPTS, onDeadLetter(job, err) emitting the structured log and metric, and isNonRetryable(err, list). Structured attempt log fields: job id, attempt, delay, error class, decision. Inline ASCII state diagram in the module header. All 5 workers register with the library through it, each passing its own non-retryable list. Land the refactor commit before the behavior-change commit. Required proof: shared-contract unit tests for each export plus one integration test per worker that the library invokes the shared policy on failure. Completeness 9/10. human: ~1 day / CC: ~30 min." + }, + { + "label": "Extract now, migrate webhook only", + "description": "Create the same `retryPolicy` module and migrate only the webhook worker in this change. The other 4 workers keep their copied envelopes and apply R3/R4/R5 by hand until a follow-up (TODO). Required proof: shared-contract unit tests plus one webhook integration test. Completeness 6/10. human: ~half day / CC: ~15 min." + }, + { + "label": "Leave duplication", + "description": "Keep 5 copy-pasted envelopes as the plan proposes. Apply R3/R4/R5 policy in each copy. No shared module, no shared tests; per-copy tests only. Completeness 3/10. human: ~1 day of copy-paste / CC: ~20 min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 — Extract one shared retry policy module now, or keep 5 copy-pasted envelopes?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: You just decided the curve shape, jitter, the attempt ceiling, the dead-letter alert and how errors are classified. Each of those has to live somewhere. If the retry envelope stays copy-pasted in 5 workers, every one of those decisions is copied 5 times, and the first bug fix or tuning change has to be made in 5 places and tested 5 times. One small shared module applies each decision once and every worker gets it.\nStakes if we pick wrong: a curve or ceiling bug fixed in 4 of 5 workers, or a jitter change that lands in 3, and nobody notices until the fifth worker stampedes a downstream.\nRecommendation: A because the plan already states the 5 bodies are identical, the module is about 50 lines, the migration is mechanical per worker, and doing the refactor commit before the behavior commit keeps each step reviewable. (human: ~1 day / CC: ~30 min)\nCompleteness: A=9/10, B=6/10, C=3/10\nPros / cons:\nA) Extract now, migrate all 5 (recommended)\n ✅ Curve, jitter, ceiling, dead-letter alert and classifier shape are each implemented and tested exactly once\n ✅ Refactor commit lands before the behavior-change commit, so each is small and reviewable on its own\n ❌ A defect in the shared module affects all 5 workers at once; the contract tests are the guard\nB) Extract now, migrate webhook only\n ✅ Smallest first step; proves the module against the worker whose behavior is changing anyway\n ✅ Other 4 workers are untouched in this change, so their risk is zero for now\n ❌ Leaves 4 copies carrying the new policy by hand, so the duplication the plan already called out gets worse, not better\nC) Leave duplication\n ✅ No refactor risk in this change at all\n ✅ Matches the plan as written\n ❌ Every approved policy decision is copy-pasted 5 times and drifts from the first fix onward\nNet: one 50-line module now against 5 hand-maintained copies of every retry decision.": "Extract now, migrate all 5 (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:53:59.810Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_01Pv8bKkKS2rSamqKz1rvNX1", + "questions": [ + { + "question": "D7 — How do we protect the existing `processWebhookJob()` behavior through the rewrite?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: You are rewriting the code that sends webhooks to customers, and there are no tests around it. The rewrite is supposed to keep everything the same except how failures are handled. Without tests written against the current code first, there is no way to know whether the new version still sends the same request, with the same headers and signature, exactly once. The cheapest insurance is to pin the current behavior in tests before changing a line, then keep them green.\nStakes if we pick wrong: a subtly different request body or signature ships to every webhook receiver at once, or a duplicate send slips through, and the first signal is a customer complaint.\nRecommendation: A because characterization tests are cheap with AI, they are the only way to detect an unintended difference in a rewrite, and they become the permanent contract suite for the webhook path. (human: ~1 day / CC: ~20 min)\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Characterize first, then rewrite (recommended)\n ✅ Pins request shape, headers, signature and success bookkeeping against the current code, so any unintended difference fails a test\n ✅ Intentional changes (pre-send retry, dead-letter) are written as explicit tests, so the diff between old and new behavior is documented\n ❌ Requires reading the current implementation carefully and a day of test writing before the rewrite starts\nB) At-most-once assertions only\n ✅ Covers the one guarantee the plan named as at risk\n ✅ Faster to write; no characterization pass\n ❌ Request shape, headers, signature and success bookkeeping can change silently and no test notices\nNet: pin the whole current contract for a day of work, or protect one guarantee and hope the rest survived.", + "header": "Webhook regression", + "multiSelect": false, + "options": [ + { + "label": "Characterize first, then rewrite (recommended)", + "description": "Before modifying `processWebhookJob()`, write characterization tests (e.g. `processWebhookJob.test`) against the current implementation asserting: exact request body, headers and signature for a fixed payload; exactly one send on success, on timeout and on 5xx; success bookkeeping. Keep them green through the rewrite. Then add intentional-difference tests: pre-send failure schedules a retry; timeout/5xx lands in dead-letter with no second send. Flag the suite CRITICAL in the plan. R8 stays pending. Completeness 9/10. human: ~1 day / CC: ~20 min." + }, + { + "label": "At-most-once assertions only", + "description": "After the rewrite, add tests asserting exactly one send on success, timeout and 5xx, and a retry on pre-send failure. No characterization of request shape, headers, signature or success bookkeeping. R8 stays pending. Completeness 6/10. human: ~2 h / CC: ~5 min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 — How do we protect the existing `processWebhookJob()` behavior through the rewrite?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: You are rewriting the code that sends webhooks to customers, and there are no tests around it. The rewrite is supposed to keep everything the same except how failures are handled. Without tests written against the current code first, there is no way to know whether the new version still sends the same request, with the same headers and signature, exactly once. The cheapest insurance is to pin the current behavior in tests before changing a line, then keep them green.\nStakes if we pick wrong: a subtly different request body or signature ships to every webhook receiver at once, or a duplicate send slips through, and the first signal is a customer complaint.\nRecommendation: A because characterization tests are cheap with AI, they are the only way to detect an unintended difference in a rewrite, and they become the permanent contract suite for the webhook path. (human: ~1 day / CC: ~20 min)\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Characterize first, then rewrite (recommended)\n ✅ Pins request shape, headers, signature and success bookkeeping against the current code, so any unintended difference fails a test\n ✅ Intentional changes (pre-send retry, dead-letter) are written as explicit tests, so the diff between old and new behavior is documented\n ❌ Requires reading the current implementation carefully and a day of test writing before the rewrite starts\nB) At-most-once assertions only\n ✅ Covers the one guarantee the plan named as at risk\n ✅ Faster to write; no characterization pass\n ❌ Request shape, headers, signature and success bookkeeping can change silently and no test notices\nNet: pin the whole current contract for a day of work, or protect one guarantee and hope the rest survived.": "Characterize first, then rewrite (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:55:40.441Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_01N7xKjvDw5oJhSmyLK3zRAR", + "questions": [ + { + "question": "D8 — Do the per-worker integration tests run the real job library, or a mocked hook?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: The whole point of D1 was that the library remembers attempt counts across worker restarts and keeps failed jobs somewhere you can find them. A test that fakes the library cannot check either of those; it only checks that your function was called. Running the real library against a throwaway test queue is slower but proves the parts you are relying on actually behave.\nStakes if we pick wrong: the retry count resets on every deploy or the failed set is empty when you need it, and every test was green because the mock said so.\nRecommendation: A because the library's persistence and failed set are load-bearing assumptions from D1 and D3, and a mock cannot verify either; the cost is a test backend fixture the library almost certainly already ships.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Real library backend in tests (recommended)\n ✅ Proves attempt count survives a worker restart and that exhausted jobs are actually in the failed set with their last error\n ✅ Catches library-version behavior changes and off-by-one attempt numbering that a stub would hide\n ❌ Slower suite and a test backend fixture to maintain (in-process queue or container)\nB) Mocked library hooks\n ✅ Fast, deterministic, no external fixture\n ✅ Enough to prove the worker wiring calls the shared policy\n ❌ Restart persistence and failed-set contents stay unverified, which are exactly the guarantees D1 and D3 depend on\nNet: a slower fixture that verifies the library promises you are betting on, or fast tests that trust them.", + "header": "Integration depth", + "multiSelect": false, + "options": [ + { + "label": "Real library backend in tests (recommended)", + "description": "Per-worker integration tests (5) run the actual job library against a test backend (in-process or containerized queue). Assertions: strategy invoked with real attempt numbers; attempt count survives a simulated worker restart mid-backoff; after the ceiling the job is in the failed set with its last error; a listed non-retryable error is in the failed set after attempt 1. Mark [E2E]. Completeness 9/10. human: ~1 day / CC: ~30 min." + }, + { + "label": "Mocked library hooks", + "description": "Per-worker tests stub the library retry hook and assert the shared policy is invoked with the expected arguments. No restart or failed-set verification. Completeness 6/10. human: ~2 h / CC: ~10 min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D8 — Do the per-worker integration tests run the real job library, or a mocked hook?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: The whole point of D1 was that the library remembers attempt counts across worker restarts and keeps failed jobs somewhere you can find them. A test that fakes the library cannot check either of those; it only checks that your function was called. Running the real library against a throwaway test queue is slower but proves the parts you are relying on actually behave.\nStakes if we pick wrong: the retry count resets on every deploy or the failed set is empty when you need it, and every test was green because the mock said so.\nRecommendation: A because the library's persistence and failed set are load-bearing assumptions from D1 and D3, and a mock cannot verify either; the cost is a test backend fixture the library almost certainly already ships.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Real library backend in tests (recommended)\n ✅ Proves attempt count survives a worker restart and that exhausted jobs are actually in the failed set with their last error\n ✅ Catches library-version behavior changes and off-by-one attempt numbering that a stub would hide\n ❌ Slower suite and a test backend fixture to maintain (in-process queue or container)\nB) Mocked library hooks\n ✅ Fast, deterministic, no external fixture\n ✅ Enough to prove the worker wiring calls the shared policy\n ❌ Restart persistence and failed-set contents stay unverified, which are exactly the guarantees D1 and D3 depend on\nNet: a slower fixture that verifies the library promises you are betting on, or fast tests that trust them.": "Real library backend in tests (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:56:39.620Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_01XEdmtg1U25aqBrEGFb8dfg", + "questions": [ + { + "question": "D9 — Cache the dependency graph across retries, or recompute it on every attempt?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Every time a job retries, the plan reads the whole payload from the database again and rebuilds the same dependency graph from it. The payload never changes between attempts, so the answer is always the same. With up to 5 attempts, that is up to 5 reads and 5 builds per failing job, and failing jobs pile up exactly when something is already down. Building once and storing the result with the job removes almost all of that.\nStakes if we pick wrong: a downstream outage turns into a database load spike from your own retries, or you spend effort caching something that turns out to be cheap.\nRecommendation: A because the graph is derived from an immutable payload, the library already stores job data per attempt, and the guard (payload hash check) makes the cache safe; it also removes the redundant DB fetch. Confidence is medium: if payloads are tiny and the graph build is microseconds, C is acceptable and this becomes a TODO.\nCompleteness: A=9/10, B=5/10, C=4/10\nPros / cons:\nA) Compute once, store on job (recommended)\n ✅ Retries read no extra payload and build no graph; outage-time DB load drops from 5N to about N\n ✅ Survives worker restarts and works across workers because the cache lives in the job data, not in a process\n ❌ Adds serialized graph size to each job record and needs a payload-hash guard to stay correct\nB) In-process memo\n ✅ Simple to add, no change to job data shape\n ✅ Helps when the same worker process picks up the retry\n ❌ Retries usually land on a different worker or after a restart, so the memo misses most of the time and still re-fetches the payload\nC) Leave as-is\n ✅ Zero new code and no cache correctness to reason about\n ✅ Bounded at 5 attempts by D3, so the waste is finite\n ❌ Every retry storm during an outage multiplies database reads by up to 5\nNet: one persisted derived value with a hash guard, or accept a 5x read multiplier exactly when the system is least healthy.", + "header": "Graph caching", + "multiSelect": false, + "options": [ + { + "label": "Compute once, store on job (recommended)", + "description": "On attempt 1, read the payload (from the library's job data if it carries it, else one DB fetch), build the dependency graph, and persist the serialized graph plus a payload hash in the job data. On later attempts, verify the hash and deserialize; on mismatch, rebuild. Required proof: unit test that attempt 2+ performs no DB fetch and no graph build when the hash matches; test that a hash mismatch triggers a rebuild. Completeness 9/10. human: ~half day / CC: ~15 min." + }, + { + "label": "In-process memo", + "description": "Memoize the built graph per job id in worker memory. Payload re-fetch unchanged. Required proof: test that a second attempt in the same process reuses the graph. Completeness 5/10. human: ~1 h / CC: ~5 min." + }, + { + "label": "Leave as-is", + "description": "Re-fetch the payload and rebuild the graph on every attempt, as the plan proposes. No new tests. Completeness 4/10. human: 0 / CC: 0." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D9 — Cache the dependency graph across retries, or recompute it on every attempt?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Every time a job retries, the plan reads the whole payload from the database again and rebuilds the same dependency graph from it. The payload never changes between attempts, so the answer is always the same. With up to 5 attempts, that is up to 5 reads and 5 builds per failing job, and failing jobs pile up exactly when something is already down. Building once and storing the result with the job removes almost all of that.\nStakes if we pick wrong: a downstream outage turns into a database load spike from your own retries, or you spend effort caching something that turns out to be cheap.\nRecommendation: A because the graph is derived from an immutable payload, the library already stores job data per attempt, and the guard (payload hash check) makes the cache safe; it also removes the redundant DB fetch. Confidence is medium: if payloads are tiny and the graph build is microseconds, C is acceptable and this becomes a TODO.\nCompleteness: A=9/10, B=5/10, C=4/10\nPros / cons:\nA) Compute once, store on job (recommended)\n ✅ Retries read no extra payload and build no graph; outage-time DB load drops from 5N to about N\n ✅ Survives worker restarts and works across workers because the cache lives in the job data, not in a process\n ❌ Adds serialized graph size to each job record and needs a payload-hash guard to stay correct\nB) In-process memo\n ✅ Simple to add, no change to job data shape\n ✅ Helps when the same worker process picks up the retry\n ❌ Retries usually land on a different worker or after a restart, so the memo misses most of the time and still re-fetches the payload\nC) Leave as-is\n ✅ Zero new code and no cache correctness to reason about\n ✅ Bounded at 5 attempts by D3, so the waste is finite\n ❌ Every retry storm during an outage multiplies database reads by up to 5\nNet: one persisted derived value with a hash guard, or accept a 5x read multiplier exactly when the system is least healthy.": "Compute once, store on job (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T17:58:34.047Z" + }, + { + "sessionId": "cd04b55a-5f1a-4672-b3ed-40ea1b1636bc", + "toolUseId": "toolu_012XHAmgKwXGVbhDkoCK1K1R", + "questions": [ + { + "question": "D10 — Record the webhook at-least-once upgrade as a TODO?\nProject/branch/task: gstack-plan-count-yWJb6k on main, retry framework plan; follow-up to D2.\nELI10: In D2 you chose to keep webhooks at \"send at most once\", so a slow or erroring receiver means that delivery is dropped into the failed set instead of retried. The fix (retry with a delivery id the receiver can dedupe) needs receiver work first. This question only decides whether we write that follow-up down in TODOS.md so it does not get lost.\nStakes if we pick wrong: skipped, the only trace is a code comment and a decision-log row; built now, this PR grows and contradicts the D2 call.\nRecommendation: A because the upgrade has an external prerequisite and a clear trigger, which is exactly what a TODO is for.\nNote: options differ in kind, not coverage — no completeness score.\nPros / cons:\nA) Add to TODOS.md (recommended)\n ✅ Creates TODOS.md with the What/Why/Context/Depends-on record, findable by /retro and future reviews\n ✅ Zero implementation cost now; the marker in code and the TODO entry point at each other\n ❌ One more file in the repo that someone has to keep honest as work lands\nB) Skip\n ✅ No new file; the decision log and gstack-shortcut marker already carry the trigger\n ✅ Avoids a TODO nobody may pick up if receivers never add dedupe\n ❌ The trigger lives only in a comment and a JSONL row, easy to miss when receivers do change\nC) Build it now in this PR\n ✅ Ships the stronger delivery guarantee in the same change as the retry framework\n ✅ Reuses the retryPolicy module while it is fresh\n ❌ Reverses D2 and depends on receiver dedupe that does not exist yet, so duplicates would reach receivers\nNet: a TODO entry now versus relying on a code marker alone; building now is off the table until receivers can dedupe.", + "header": "Webhook TODO", + "multiSelect": false, + "options": [ + { + "label": "Add to TODOS.md (recommended)", + "description": "Create TODOS.md at implementation time with the TODO record (What/Why/Context/Depends-on) under a `## Workers` section, P3, effort M. No product code change." + }, + { + "label": "Skip", + "description": "Do not create TODOS.md. The decision log entry and the gstack-shortcut marker remain the only trail." + }, + { + "label": "Build it now in this PR", + "description": "Extend the accepted scope to at-least-once delivery with delivery id and idempotency key; would reopen D2." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D10 — Record the webhook at-least-once upgrade as a TODO?\nProject/branch/task: gstack-plan-count-yWJb6k on main, retry framework plan; follow-up to D2.\nELI10: In D2 you chose to keep webhooks at \"send at most once\", so a slow or erroring receiver means that delivery is dropped into the failed set instead of retried. The fix (retry with a delivery id the receiver can dedupe) needs receiver work first. This question only decides whether we write that follow-up down in TODOS.md so it does not get lost.\nStakes if we pick wrong: skipped, the only trace is a code comment and a decision-log row; built now, this PR grows and contradicts the D2 call.\nRecommendation: A because the upgrade has an external prerequisite and a clear trigger, which is exactly what a TODO is for.\nNote: options differ in kind, not coverage — no completeness score.\nPros / cons:\nA) Add to TODOS.md (recommended)\n ✅ Creates TODOS.md with the What/Why/Context/Depends-on record, findable by /retro and future reviews\n ✅ Zero implementation cost now; the marker in code and the TODO entry point at each other\n ❌ One more file in the repo that someone has to keep honest as work lands\nB) Skip\n ✅ No new file; the decision log and gstack-shortcut marker already carry the trigger\n ✅ Avoids a TODO nobody may pick up if receivers never add dedupe\n ❌ The trigger lives only in a comment and a JSONL row, easy to miss when receivers do change\nC) Build it now in this PR\n ✅ Ships the stronger delivery guarantee in the same change as the retry framework\n ✅ Reuses the retryPolicy module while it is fresh\n ❌ Reverses D2 and depends on receiver dedupe that does not exist yet, so duplicates would reach receivers\nNet: a TODO entry now versus relying on a code marker alone; building now is off the table until receivers can dedupe.": "Add to TODOS.md (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-29T18:01:55.194Z" + } + ], + "plan": "# Engineering review: Add background job retry framework\n\nReview target (fixed): `PLAN.md` in `/home/runner/.cache/gstack-paid-shard-mmiSh1/tmp/gstack-plan-count-yWJb6k` (branch `main`, commit `0ad2479`).\nReport file: this file (path requested by the user).\nReviewer: /plan-eng-review, session `642-1790703811-6005ed0c`, 2026-09-29.\n\n## Original plan (unchanged copy)\n\n# Plan: Add background job retry framework\n\n## Architecture\nWe'll roll a custom exponential-backoff scheduler inline in each worker\nrather than use the existing job library's built-in retry hooks. Same\nshape as the library version, but we want full control over the curve.\n\n## Code quality\nThe retry envelope (compute delay, log attempt, dispatch) is duplicated\nacross 5 worker files with copy-pasted bodies. We will leave the\nduplication for now and refactor \"later.\"\n\n## Tests\nThe existing `processWebhookJob()` flow gets rewritten as part of this\nchange. No regression test for the prior at-most-once delivery guarantee\nis planned.\n\n## Performance\nOn every retry we re-fetch the full job payload from the database, then\niterate the payload to recompute the dependency graph. Could cache the\ngraph on the first attempt; not planned.\n\n## Scope Challenge record\n\nEvidence available: plan text only. The repo contains `PLAN.md` and `CLAUDE.md`; the 5 worker files, `processWebhookJob()`, the job library and its retry hooks are `not available` in this checkout. Findings quote plan lines and are calibrated as plan-text findings.\n\nComplexity count (estimates from plan text): ~5-6 changed files (5 worker files; `processWebhookJob()` may live in one of them), 0 new classes/services (scheduler is inline). Below the 8-file / 2-class gate, so the complexity selectors (B) are skipped.\n\nSearch check: Aside unavailable, host WebSearch used. Industry default [Layer 1]: library built-in retry, exponential backoff + jitter, bounded attempts, dead-letter, idempotent handlers.\n\n## Decision ledger\n\n### R1: Retry scheduler mechanism (library hooks vs custom inline scheduler)\nFinding: SC-1, P1, confidence 8/10, PLAN.md:7-9, reviewer: plan-eng-review (native)\nPlan baseline: original proposal, \"custom exponential-backoff scheduler inline in each worker rather than use the existing job library's built-in retry hooks\" (PLAN.md:7-9). Nothing approved yet.\nRuntime evidence: unknown. Job library and worker files not available in this checkout; plan text states the library has built-in retry hooks and the custom version is the \"same shape\".\nComparison grid:\n\n| Choice | Current | A) Library hooks + custom curve | B) Custom inline scheduler |\n|---|---|---|---|\n| R1 retry mechanism | custom inline scheduler (proposed) | library retry hooks, backoff supplied as one strategy function | custom scheduler inline per worker, as proposed |\n| Backoff curve ownership | \"full control\" wanted | full control via strategy function (verify hook accepts a function; else fall back to B) | full control |\n| Attempt count persistence / terminal handling | unspecified | inherited from library | must be hand-built (pending, R3) |\n| R2 webhook delivery semantics | pending | pending | pending |\n| R3 attempt bound + dead-letter | pending | pending | pending |\n| R4 jitter | pending | pending | pending |\n| R5 shared envelope | pending | pending | pending |\n\nQuestion D1:\nD1 — Use the job library's retry hooks or roll a custom inline scheduler?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: The job library you already run has a retry system built in. The plan wants to rebuild the same thing by hand inside each of the 5 workers, only so the delay curve can be tuned. Retry systems are easy to get subtly wrong: losing the attempt count when a worker restarts, retrying forever, or retrying twice at once. The library has already paid for those bugs; a hand-rolled copy pays for them again in production.\nStakes if we pick wrong: a hand-rolled scheduler that drops attempt state on restart or double-schedules turns one failed job into duplicate side effects or an infinite retry loop, with no dead-letter to catch it.\nRecommendation: A because the plan admits the shapes are identical, and the curve is pluggable in the library through a strategy function, so A delivers the same control with far less new code. (human: ~1 day / CC: ~20 min for A; human: ~1 week / CC: ~2 h for B plus ongoing ownership)\nCompleteness: A=9/10, B=5/10\nPros / cons:\nA) Library hooks + custom curve (recommended)\n ✅ Attempt counting, persistence across restarts and terminal handling come from tested library code, not new code\n ✅ The custom curve still lives in one strategy function, so \"full control over the curve\" is preserved\n ❌ Requires confirming the library's hook accepts a custom delay function; if it does not, we fall back to B for the curve only\nB) Custom inline scheduler\n ✅ Zero dependency on the library's retry semantics or its upgrade cadence\n ✅ Any curve shape, any bookkeeping, no hook constraints\n ❌ Rebuilds attempt state, restart persistence, concurrency guards and dead-lettering by hand, and those are the parts that fail at 3am\nNet: you are trading a one-line strategy function against owning a second retry engine forever.\nHeader: Retry engine\nOptions:\nA) Library hooks + custom curve (recommended)\nRegister the exponential-backoff curve as one custom backoff strategy function with the job library's built-in retry hooks. Attempt counting, persistence across worker restarts, and terminal/dead-letter handling come from the library. Verify the hook accepts a delay function first; if it does not, fall back to a custom curve only for delay computation while keeping library scheduling. R2-R5 stay pending. Completeness 9/10. human: ~1 day / CC: ~20 min.\nB) Custom inline scheduler\nKeep the plan as written: a custom exponential-backoff scheduler inline in each worker, bypassing the library's retry hooks. Attempt state, restart persistence, concurrency guards and terminal handling must be designed and tested by hand (tracked under R3). R2-R5 stay pending. Completeness 5/10. human: ~1 week / CC: ~2 h plus ongoing ownership.\n\nState: approved\nActual answer: A) Library hooks + custom curve (D1 answer, user selection)\nAccepted scope: Replace the custom inline scheduler with the job library's built-in retry hooks. The exponential-backoff curve is supplied as one custom backoff strategy function. Attempt counting, persistence across worker restarts and terminal/dead-letter handling come from the library. First implementation step: verify the hook accepts a delay function; if it does not, use a custom delay computation only, keeping library scheduling. Required proof: unit tests of the strategy function (curve values per attempt) and an integration test that the library invokes it on failure. R2-R5 remain pending.\nHistory: none\n\nScope Challenge result: scope accepted as-is (D1 changed mechanism, not feature scope). MODE = FULL_REVIEW.\n\n### R2: Webhook delivery semantics under retry\nFinding: ARCH-1, P1, confidence 8/10, PLAN.md:17-19, reviewer: plan-eng-review (native)\nPlan baseline: original proposal, `processWebhookJob()` is rewritten to retry; prior guarantee was at-most-once; no idempotency key or retry classification stated (PLAN.md:17-19). R1 approved: retries run through library hooks.\nRuntime evidence: unknown. `processWebhookJob()` and receiver contract not available in this checkout. Plan text asserts the prior guarantee was at-most-once.\nComparison grid:\n\n| Choice | Current | A) Keep at-most-once | B) At-least-once + idempotency key | C) Plain retry (plan as written) |\n|---|---|---|---|---|\n| R2 webhook delivery semantics | at-most-once today; plan retries without stating semantics | at-most-once preserved: retry only when the request provably never left (connect/DNS/pre-send errors); timeouts and 5xx are terminal | at-least-once: retry timeouts/5xx too; every attempt carries the same stable delivery id header so receivers can dedupe | at-least-once with duplicates indistinguishable to receivers |\n| Receiver-visible contract | no duplicates | no duplicates (unchanged) | duplicates possible, always carrying the same id (contract change, communicate to receivers) | duplicates possible, not deduplicable |\n| R1 library hooks | approved | approved, unchanged | approved, unchanged | approved, unchanged |\n| R3 attempt bound + dead-letter | pending | pending | pending | pending |\n| R4 jitter | pending | pending | pending | pending |\n| R5 error classification (other workers) | pending | pending (webhook classification fixed by this row) | pending (webhook classification fixed by this row) | pending |\n| R7 regression contract | pending | pending | pending | pending |\n\nQuestion D2:\nD2 — What delivery guarantee does `processWebhookJob()` keep once it retries?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Today a webhook is sent once; if it fails, it is gone (at-most-once). The moment you retry, a request that timed out after the customer already received it gets sent again, so the customer sees the same event twice. You have to pick: either only retry when you are sure the request never left, or retry freely but stamp every attempt with the same id so the customer can ignore repeats. The plan does neither and just retries.\nStakes if we pick wrong: customers process duplicate events (double orders, double emails) with no way to detect them, or you ship a retry feature that almost never fires because most webhook failures are timeouts.\nRecommendation: B because it is the standard webhook contract (retry on timeout/5xx, stable event id per attempt) and is the only option where retrying actually improves delivery while giving receivers a way to dedupe. This is a receiver-visible contract change; A is the right pick if you cannot communicate it to receivers.\nCompleteness: A=7/10, B=9/10, C=3/10\nPros / cons:\nA) Keep at-most-once\n ✅ No change to what receivers see; the existing guarantee and its regression test stay valid as-is\n ✅ Smallest blast radius: no new headers, no receiver communication needed\n ❌ Retries only fire on connect/DNS/pre-send errors; timeouts and 5xx go straight to terminal, so most real failures are still not retried\nB) At-least-once + idempotency key (recommended)\n ✅ Timeouts and 5xx are retried, so delivery reliability actually improves for receivers\n ✅ Same delivery id on every attempt lets receivers dedupe; this is the contract Stripe/GitHub-style webhooks use\n ❌ Receiver-visible contract change: duplicates become possible and receivers must be told to dedupe on the id\nC) Plain retry (plan as written)\n ✅ Least code: no header, no classification, just retry on any failure\n ✅ Ships fastest\n ❌ Duplicates reach receivers with no way to tell them apart; silent double side effects\nNet: A keeps the promise but retries little; B changes the promise but makes retries worth having; C breaks the promise silently.\nHeader: Webhook semantics\nOptions:\nA) Keep at-most-once\nPreserve at-most-once for `processWebhookJob()`. Retry only failures where the request provably never left the process (connection refused, DNS failure, errors before send). Timeouts, 5xx and any post-send ambiguity are terminal and go to whatever terminal handling R3 decides. No new headers; receiver contract unchanged. R3, R4, R5, R7 stay pending. Completeness 7/10. human: ~half day / CC: ~10 min.\nB) At-least-once + idempotency key (recommended)\nRetry timeouts and 5xx as well as pre-send failures. Every attempt carries the same stable delivery id (e.g. an `Idempotency-Key`/event-id header derived from the job id, generated once, persisted with the job). 4xx other than 408/429 is non-retryable for webhooks. Document to receivers that duplicates are possible and must be deduped on the id. R3, R4, R5, R7 stay pending. Completeness 9/10. human: ~1 day / CC: ~20 min.\nC) Plain retry (plan as written)\nRetry `processWebhookJob()` on any failure with no delivery id and no classification. Duplicates reach receivers undetectably. R3, R4, R5, R7 stay pending. Completeness 3/10. human: ~1 h / CC: ~5 min.\n\nState: approved\nActual answer: A) Keep at-most-once (D2 answer, user selection)\nAccepted scope: `processWebhookJob()` preserves at-most-once delivery. Retry fires only for failures where the request provably never left the process (connection refused, DNS failure, errors raised before send). Timeouts, 5xx responses and any post-send ambiguity are terminal and route to the terminal handling decided in R3. No new headers; receiver contract unchanged. Accepted shortcut (Completeness 7/10): ceiling is that timeouts/5xx are never retried; upgrade trigger is when receivers can dedupe on a stable delivery id, at which point revisit toward at-least-once + idempotency key. Required proof: regression test that a timeout/5xx produces exactly one send and no retry; test that a pre-send failure retries. R3, R4, R5, R7 remain pending. Decision log id: f9e8dfdf-4e90-4883-9064-014d784b9405.\nHistory: none\n\n### R3: Attempt ceiling and terminal handling (dead-letter)\nFinding: ARCH-2, P1, confidence 8/10, PLAN.md:7-9, reviewer: plan-eng-review (native)\nPlan baseline: original proposal names an exponential-backoff curve with no maximum attempts and no behavior on exhaustion (PLAN.md:7-9). R1 approved: library hooks. R2 approved: webhook timeouts/5xx are terminal and route to this row's handling.\nRuntime evidence: unknown. Library's dead-letter/failed-set feature not verifiable in this checkout.\nComparison grid:\n\n| Choice | Current | A) Bounded + dead-letter + alert | B) Bounded + log-and-drop | C) Unbounded (plan as written) |\n|---|---|---|---|---|\n| R3 attempt ceiling | none stated | max attempts per worker, default 5, configured in one place | max attempts per worker, default 5 | no ceiling |\n| R3 terminal disposition | none stated | exhausted and non-retryable jobs land in the library's dead-letter/failed set with last error; one structured error log + metric on entry | error log only, job dropped | never terminal (retries forever) |\n| R1 library hooks | approved | approved, unchanged | approved, unchanged | approved, unchanged |\n| R2 webhook at-most-once | approved | approved; webhook timeouts/5xx land in dead-letter | approved; webhook timeouts/5xx logged and dropped | approved (conflict: terminal has no destination) |\n| R4 jitter | pending | pending | pending | pending |\n| R5 error classification | pending | pending | pending | pending |\n\nQuestion D3:\nD3 — How many times may a job retry, and where does it go when it gives up?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: A retry curve without a stopping point is a job that runs forever when the thing it depends on is down for good. You need a maximum number of tries, and you need a place for jobs that used up their tries (a dead-letter set) so someone can look at them and replay them later. Otherwise failed work quietly disappears or quietly never stops.\nStakes if we pick wrong: either a poisoned job hammers a downstream forever and starves healthy jobs, or failed webhooks and jobs vanish with only a log line nobody reads.\nRecommendation: A because the library already provides the failed set, so the dead-letter and alert cost a config line and one log call, and it is the only option where an operator can find and replay a lost job.\nCompleteness: A=9/10, B=6/10, C=2/10\nPros / cons:\nA) Bounded + dead-letter + alert (recommended)\n ✅ Exhausted jobs are inspectable and replayable from the library's failed set, with the last error attached\n ✅ One structured log line plus a metric on dead-letter entry makes a downstream outage visible within minutes\n ❌ Needs a per-worker ceiling value and a dead-letter retention/cleanup policy to be chosen and documented\nB) Bounded + log-and-drop\n ✅ Bounds the retry loop with the least configuration\n ✅ No dead-letter retention to manage\n ❌ A dropped job is gone; the only trace is a log line, so replay after an outage is impossible\nC) Unbounded (plan as written)\n ✅ No ceiling to tune; a job eventually succeeds if the dependency ever recovers\n ✅ Zero extra code\n ❌ Permanently failing jobs retry forever, consume worker capacity and never surface as a problem\nNet: you are choosing whether a job that cannot succeed becomes a visible artifact, a log line, or a permanent background load.\nHeader: Attempt ceiling\nOptions:\nA) Bounded + dead-letter + alert (recommended)\nSet a maximum attempt count per worker (default 5, overridable per worker, configured in the same place as the backoff strategy). On exhaustion or on a non-retryable error, the job lands in the library's dead-letter/failed set with its last error; emit one structured error log and a metric on entry. Webhook timeouts/5xx (terminal per R2) land here too. Document the retention/replay procedure. R4, R5 stay pending. Completeness 9/10. human: ~half day / CC: ~15 min.\nB) Bounded + log-and-drop\nSet the same per-worker maximum attempt count (default 5). On exhaustion, log the error and drop the job; no dead-letter set, no metric, no replay. R4, R5 stay pending. Completeness 6/10. human: ~1 h / CC: ~5 min.\nC) Unbounded (plan as written)\nNo attempt ceiling; the exponential curve continues until the job succeeds. No terminal handling. Conflicts with R2, which needs a terminal destination for webhook timeouts. R4, R5 stay pending. Completeness 2/10. human: 0 / CC: 0.\n\nState: approved\nActual answer: A) Bounded + dead-letter + alert (D3 answer, user selection)\nAccepted scope: Maximum attempt count per worker, default 5, overridable per worker, configured in the same place as the backoff strategy. On exhaustion or on a non-retryable error the job lands in the library's dead-letter/failed set with its last error; one structured error log and one metric are emitted on entry. Webhook timeouts/5xx (terminal per R2) land here too. Retention/replay procedure documented. Required proof: test that attempt N+1 is never scheduled after the ceiling; test that an exhausted job appears in the failed set with its last error and that the log/metric fire once; test that a webhook timeout lands in the failed set on attempt 1. R4, R5 remain pending.\nHistory: none\n\n### R4: Jitter on the backoff curve\nFinding: ARCH-3, P2, confidence 7/10, PLAN.md:7, reviewer: plan-eng-review (native)\nPlan baseline: original proposal, \"custom exponential-backoff scheduler\" with \"full control over the curve\" (PLAN.md:7-9); no jitter mentioned. R1 approved: curve lives in one strategy function.\nRuntime evidence: unknown. No curve code available.\nComparison grid:\n\n| Choice | Current | A) Equal jitter | B) No jitter (pure curve) |\n|---|---|---|---|\n| R4 jitter | unspecified (pure `base * 2^attempt` implied) | delay = half of the curve value plus a random amount up to the other half, so retries spread across the window | delay = exact curve value; all jobs failing at time T retry at T+delay together |\n| Curve ownership (R1) | approved: one strategy function | unchanged; jitter applied inside the same function | unchanged |\n| Delay ceiling | unspecified | unspecified (implicitly bounded by R3 max attempts) | unspecified |\n| R3 attempt ceiling | approved | approved, unchanged | approved, unchanged |\n| R5 error classification | pending | pending | pending |\n\nQuestion D4:\nD4 — Add jitter to the backoff curve, or keep it deterministic?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: When a downstream service goes down, hundreds of jobs across all 5 workers fail at the same moment. With a pure exponential curve they all wake up at exactly the same moment too, and hit the recovering service as one wave, which can knock it over again. Jitter adds a random spread to each delay so the retries trickle back instead of stampeding.\nStakes if we pick wrong: a downstream that recovers from an outage gets re-flattened by your own synchronized retry wave, turning a 2-minute blip into a 20-minute incident.\nRecommendation: A because it is two lines inside the strategy function you already own and it is the standard mitigation for retry storms; deterministic curves are only useful in tests, which can seed or stub the random source.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Equal jitter (recommended)\n ✅ Retries after a shared outage spread across the window instead of returning as one synchronized burst\n ✅ Lives inside the single strategy function from D1, so every worker gets it with no per-worker code\n ❌ Curve tests need an injectable random source to stay deterministic\nB) No jitter (pure curve)\n ✅ Exact, predictable retry times that are easy to reason about and assert in tests\n ✅ Zero extra code beyond the curve itself\n ❌ All jobs that fail together retry together, so the retry framework itself becomes a traffic amplifier during outages\nNet: predictability in tests against stampede protection in production; the test cost is one injected random source.\nHeader: Jitter\nOptions:\nA) Equal jitter (recommended)\nInside the single backoff strategy function, compute the exponential delay and return half of it plus a random amount up to the other half (equal jitter). The random source is injectable so tests can pin it. Required proof: unit test that returned delays fall within [curve/2, curve] for each attempt, and that a pinned random source gives a deterministic value. R5 stays pending. Completeness 9/10. human: ~1 h / CC: ~5 min.\nB) No jitter (pure curve)\nReturn the exact exponential curve value with no random spread. Required proof: unit test of the exact value per attempt. R5 stays pending. Completeness 6/10. human: ~30 min / CC: ~3 min.\n\nState: approved\nActual answer: A) Equal jitter (D4 answer, user selection)\nAccepted scope: The single backoff strategy function computes the exponential delay and returns half of it plus a random amount up to the other half (equal jitter). Random source is injectable. Required proof: unit test that returned delays fall within [curve/2, curve] for each attempt; unit test that a pinned random source yields a deterministic value. R5 remains pending.\nHistory: none\n\n### R5: Retryable vs non-retryable error classification (non-webhook workers)\nFinding: ARCH-4, P2, confidence 6/10 (medium: actual error types not available), PLAN.md:7-9, reviewer: plan-eng-review (native)\nPlan baseline: original proposal retries on failure with no classification (PLAN.md:7-9). R2 fixed the webhook worker's classification (pre-send only). R3 approved: non-retryable errors route to dead-letter.\nRuntime evidence: unknown. Worker error types not available in this checkout.\nComparison grid:\n\n| Choice | Current | A) Explicit non-retryable list | B) Retry everything to ceiling |\n|---|---|---|---|\n| R5 classification | none; every failure retries | each worker declares its non-retryable error types (validation, auth/permission, malformed payload); those go straight to dead-letter; unknown errors retry | every error retries until the R3 ceiling, then dead-letter |\n| R2 webhook classification | approved (pre-send only) | unchanged | unchanged |\n| R3 ceiling + dead-letter | approved | unchanged; non-retryable short-circuits to dead-letter on attempt 1 | unchanged |\n| R4 jitter | approved | unchanged | unchanged |\n\nQuestion D5:\nD5 — Should workers name errors that must not be retried, or retry every failure to the ceiling?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Some failures fix themselves if you wait (a database hiccup, a slow API). Others never will (a payload that fails validation, a revoked API key). Retrying the second kind five times with growing delays just wastes capacity and delays the moment someone notices. Letting each worker say \"these error types are permanent\" sends them straight to the dead-letter set on the first try.\nStakes if we pick wrong: a bad payload burns 5 attempts and up to the full backoff window before it surfaces, and during a bad deploy every job does this at once.\nRecommendation: A because it is a small per-worker list, the dead-letter path already exists from D3, and it turns a permanent failure into an immediate signal instead of a delayed one. Medium confidence on which types are permanent; verify against the actual error classes when implementing.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Explicit non-retryable list (recommended)\n ✅ Permanent failures reach the dead-letter set on attempt 1, so operators see bad payloads or revoked credentials within seconds\n ✅ Unknown errors still default to retry, so nothing transient is accidentally dropped\n ❌ Each worker needs a short, reviewed list of permanent error types, and a wrong entry makes a transient error permanent\nB) Retry everything to ceiling\n ✅ No classification to get wrong; behavior is identical for every worker\n ✅ Nothing to maintain when new error types appear\n ❌ Permanent failures consume the full attempt budget and backoff window before anyone can see them\nNet: a short reviewed list per worker against a guaranteed delay on every permanent failure.\nHeader: Error classes\nOptions:\nA) Explicit non-retryable list (recommended)\nEach of the 4 non-webhook workers declares its non-retryable error types (validation errors, auth/permission errors, malformed payload). Those bypass retry and land in the dead-letter set (R3) on attempt 1 with the error attached. Any error not on the list retries per R3/R4. Required proof: per worker, one test that a listed error goes to dead-letter without a retry, and one test that an unlisted error retries. Completeness 9/10. human: ~half day / CC: ~15 min.\nB) Retry everything to ceiling\nNo classification. Every failure in the 4 non-webhook workers retries per R3/R4 until the ceiling, then lands in dead-letter. Required proof: covered by R3 tests. Completeness 6/10. human: 0 / CC: 0.\n\nState: approved\nActual answer: A) Explicit non-retryable list (D5 answer, user selection)\nAccepted scope: Each of the 4 non-webhook workers declares its non-retryable error types (validation, auth/permission, malformed payload; verify against actual error classes at implementation). Listed errors bypass retry and land in the dead-letter set (R3) on attempt 1 with the error attached. Unlisted errors retry per R3/R4. Required proof: per worker, one test that a listed error goes to dead-letter without a retry and one test that an unlisted error retries.\nHistory: none\n\nSection 1 dispositions: ARCH-1 (R2) accepted as at-most-once preserved; ARCH-2 (R3) accepted; ARCH-3 (R4) accepted; ARCH-4 (R5) accepted. Suppressed: webhook signature timestamp on retried attempts (confidence 4).\n\n### R6: Shared retry policy module vs duplicated envelope\nFinding: CQ-1, P1, confidence 9/10, PLAN.md:12-14, reviewer: plan-eng-review (native). Also carries CQ-2 (structured attempt log contract, P2, 7/10) and CQ-3 (delay clamp edge case, P2, 7/10) as contract details of the shared module.\nPlan baseline: original proposal, \"duplicated across 5 worker files with copy-pasted bodies. We will leave the duplication for now and refactor later\" (PLAN.md:12-14). R1, R3, R4, R5 approved: curve, ceiling, dead-letter, classification are now policy that each worker must apply.\nRuntime evidence: unknown. Worker files not available; plan asserts identical bodies.\nShared-code rubric: 5 proposed callers (plan assumption); identical behavior stated by plan; helper = one `retryPolicy` module (backoffStrategy, DEFAULT_MAX_ATTEMPTS, onDeadLetter, isNonRetryable); est. implementation removed 100-150, added 55-75, saved 45-95; tests add 80-120 so total diff may grow; blast radius all 5 workers, mitigated by contract tests and library scheduling.\nComparison grid:\n\n| Choice | Current | A) Extract now, migrate all 5 | B) Extract now, migrate webhook only | C) Leave duplication |\n|---|---|---|---|---|\n| R6 shared module | none; 5 copies | one `retryPolicy` module; all 5 workers register through it in this change, refactor commit before behavior commit | one `retryPolicy` module; webhook worker migrated now, other 4 keep copies until a follow-up | 5 copies of curve/ceiling/dead-letter/classifier config |\n| Structured attempt log (CQ-2) | unspecified | in shared module: job id, attempt, delay, error class, decision | in shared module, webhook only | per copy, unspecified |\n| Delay clamp (CQ-3) | unspecified | in shared strategy: clamp at configurable max delay | in shared strategy, webhook only | per copy, unspecified |\n| Inline ASCII state diagram | none | in shared module header | in shared module header | none |\n| R1/R3/R4/R5 approved policy | approved | applied once | applied once for webhook, 4 copies otherwise | applied 5 times |\n\nQuestion D6:\nD6 — Extract one shared retry policy module now, or keep 5 copy-pasted envelopes?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: You just decided the curve shape, jitter, the attempt ceiling, the dead-letter alert and how errors are classified. Each of those has to live somewhere. If the retry envelope stays copy-pasted in 5 workers, every one of those decisions is copied 5 times, and the first bug fix or tuning change has to be made in 5 places and tested 5 times. One small shared module applies each decision once and every worker gets it.\nStakes if we pick wrong: a curve or ceiling bug fixed in 4 of 5 workers, or a jitter change that lands in 3, and nobody notices until the fifth worker stampedes a downstream.\nRecommendation: A because the plan already states the 5 bodies are identical, the module is about 50 lines, the migration is mechanical per worker, and doing the refactor commit before the behavior commit keeps each step reviewable. (human: ~1 day / CC: ~30 min)\nCompleteness: A=9/10, B=6/10, C=3/10\nPros / cons:\nA) Extract now, migrate all 5 (recommended)\n ✅ Curve, jitter, ceiling, dead-letter alert and classifier shape are each implemented and tested exactly once\n ✅ Refactor commit lands before the behavior-change commit, so each is small and reviewable on its own\n ❌ A defect in the shared module affects all 5 workers at once; the contract tests are the guard\nB) Extract now, migrate webhook only\n ✅ Smallest first step; proves the module against the worker whose behavior is changing anyway\n ✅ Other 4 workers are untouched in this change, so their risk is zero for now\n ❌ Leaves 4 copies carrying the new policy by hand, so the duplication the plan already called out gets worse, not better\nC) Leave duplication\n ✅ No refactor risk in this change at all\n ✅ Matches the plan as written\n ❌ Every approved policy decision is copy-pasted 5 times and drifts from the first fix onward\nNet: one 50-line module now against 5 hand-maintained copies of every retry decision.\nHeader: Shared module\nOptions:\nA) Extract now, migrate all 5 (recommended)\nCreate one `retryPolicy` module exporting backoffStrategy(attempt, rng) with equal jitter and a configurable max-delay clamp, DEFAULT_MAX_ATTEMPTS, onDeadLetter(job, err) emitting the structured log and metric, and isNonRetryable(err, list). Structured attempt log fields: job id, attempt, delay, error class, decision. Inline ASCII state diagram in the module header. All 5 workers register with the library through it, each passing its own non-retryable list. Land the refactor commit before the behavior-change commit. Required proof: shared-contract unit tests for each export plus one integration test per worker that the library invokes the shared policy on failure. Completeness 9/10. human: ~1 day / CC: ~30 min.\nB) Extract now, migrate webhook only\nCreate the same `retryPolicy` module and migrate only the webhook worker in this change. The other 4 workers keep their copied envelopes and apply R3/R4/R5 by hand until a follow-up (TODO). Required proof: shared-contract unit tests plus one webhook integration test. Completeness 6/10. human: ~half day / CC: ~15 min.\nC) Leave duplication\nKeep 5 copy-pasted envelopes as the plan proposes. Apply R3/R4/R5 policy in each copy. No shared module, no shared tests; per-copy tests only. Completeness 3/10. human: ~1 day of copy-paste / CC: ~20 min.\n\nState: approved\nActual answer: A) Extract now, migrate all 5 (D6 answer, user selection)\nAccepted scope: One `retryPolicy` module exporting backoffStrategy(attempt, rng) (exponential, equal jitter per R4, configurable max-delay clamp), DEFAULT_MAX_ATTEMPTS (5, per R3), onDeadLetter(job, err) (structured log + metric per R3), isNonRetryable(err, list) (per R5). Structured attempt log fields: job id, attempt, delay, error class, decision. Inline ASCII state diagram in the module header. All 5 workers register with the library through it, each passing its own non-retryable list. Refactor commit lands before the behavior-change commit. Required proof: shared-contract unit tests for each export (including clamp at max delay for large attempt numbers) plus one integration test per worker that the library invokes the shared policy on failure.\nHistory: none\n\nSection 2 dispositions: CQ-1 (R6) accepted; CQ-2 and CQ-3 accepted as part of R6's module contract; diagram requirement accepted as part of R6.\n\n### R7: Regression contract for the `processWebhookJob()` rewrite\nFinding: TEST-1, P1 CRITICAL, confidence 9/10, PLAN.md:17-19, reviewer: plan-eng-review (native). REGRESSION RULE.\nPlan baseline: original proposal, \"`processWebhookJob()` flow gets rewritten as part of this change. No regression test for the prior at-most-once delivery guarantee is planned\" (PLAN.md:17-19). R2 approved at-most-once preserved with partial required proof (one send on timeout/5xx; pre-send failure retries). No approved contract covers the rest of the existing behavior.\nRuntime evidence: unknown. `processWebhookJob()` and any existing tests not available in this checkout; this repo has 0 test files.\nBehavior to preserve: exactly one HTTP send per job on success, timeout and 5xx; request body, headers and signature shape; success bookkeeping (delivered mark). Intentional changes: pre-send failures retry (R2); terminal failures land in dead-letter instead of prior handling (R3).\nComparison grid:\n\n| Choice | Current | A) Characterize first, then rewrite | B) At-most-once assertions only |\n|---|---|---|---|\n| R7 regression coverage | none planned | before touching the code: characterization tests pinning request shape, headers, signature, success bookkeeping and one-send-on-timeout/5xx against the CURRENT implementation; they stay green through the rewrite; then add the R2/R3 intentional-difference assertions | after the rewrite: tests asserting exactly one send on success/timeout/5xx and retry on pre-send failure only |\n| Request shape / headers / signature | unprotected | protected | unprotected |\n| Success bookkeeping | unprotected | protected | unprotected |\n| One send on timeout/5xx (R2 proof) | approved proof | included | included |\n| Pre-send retry / dead-letter (R2, R3 proof) | approved proof | included as explicit intentional-difference tests | included |\n| R8 integration depth | pending | pending | pending |\n\nQuestion D7:\nD7 — How do we protect the existing `processWebhookJob()` behavior through the rewrite?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: You are rewriting the code that sends webhooks to customers, and there are no tests around it. The rewrite is supposed to keep everything the same except how failures are handled. Without tests written against the current code first, there is no way to know whether the new version still sends the same request, with the same headers and signature, exactly once. The cheapest insurance is to pin the current behavior in tests before changing a line, then keep them green.\nStakes if we pick wrong: a subtly different request body or signature ships to every webhook receiver at once, or a duplicate send slips through, and the first signal is a customer complaint.\nRecommendation: A because characterization tests are cheap with AI, they are the only way to detect an unintended difference in a rewrite, and they become the permanent contract suite for the webhook path. (human: ~1 day / CC: ~20 min)\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Characterize first, then rewrite (recommended)\n ✅ Pins request shape, headers, signature and success bookkeeping against the current code, so any unintended difference fails a test\n ✅ Intentional changes (pre-send retry, dead-letter) are written as explicit tests, so the diff between old and new behavior is documented\n ❌ Requires reading the current implementation carefully and a day of test writing before the rewrite starts\nB) At-most-once assertions only\n ✅ Covers the one guarantee the plan named as at risk\n ✅ Faster to write; no characterization pass\n ❌ Request shape, headers, signature and success bookkeeping can change silently and no test notices\nNet: pin the whole current contract for a day of work, or protect one guarantee and hope the rest survived.\nHeader: Webhook regression\nOptions:\nA) Characterize first, then rewrite (recommended)\nBefore modifying `processWebhookJob()`, write characterization tests (e.g. `processWebhookJob.test`) against the current implementation asserting: exact request body, headers and signature for a fixed payload; exactly one send on success, on timeout and on 5xx; success bookkeeping. Keep them green through the rewrite. Then add intentional-difference tests: pre-send failure schedules a retry; timeout/5xx lands in dead-letter with no second send. Flag the suite CRITICAL in the plan. R8 stays pending. Completeness 9/10. human: ~1 day / CC: ~20 min.\nB) At-most-once assertions only\nAfter the rewrite, add tests asserting exactly one send on success, timeout and 5xx, and a retry on pre-send failure. No characterization of request shape, headers, signature or success bookkeeping. R8 stays pending. Completeness 6/10. human: ~2 h / CC: ~5 min.\n\nState: approved\nActual answer: A) Characterize first, then rewrite (D7 answer, user selection)\nAccepted scope: CRITICAL regression suite. Before modifying `processWebhookJob()`, characterization tests against the current implementation assert: exact request body, headers and signature for a fixed payload; exactly one send on success, on timeout and on 5xx; success bookkeeping. They stay green through the rewrite. Then intentional-difference tests: pre-send failure schedules a retry; timeout/5xx lands in dead-letter with no second send. Sequencing: this suite is the first implementation task and gates the webhook rewrite. R8 remains pending.\nHistory: none\n\n### R8: Integration depth for library -> retryPolicy -> dead-letter\nFinding: TEST-2, P2, confidence 7/10, PLAN.md:7-9 (library hooks, per R1), reviewer: plan-eng-review (native)\nPlan baseline: no integration tests proposed. R1/R3/R6 approved \"one integration test per worker that the library invokes the shared policy on failure\" without fixing whether the library runs for real or is mocked.\nRuntime evidence: unknown. Library test harness not available.\nComparison grid:\n\n| Choice | Current | A) Real library backend in tests | B) Mocked library hooks |\n|---|---|---|---|\n| R8 integration depth | unspecified | per-worker integration tests run the actual job library against a test backend (in-process or containerized queue); assert attempt count survives a simulated worker restart, failed set contains the job with last error, strategy invoked with real attempt numbers | per-worker tests stub the library's retry hook and assert the policy is called; no restart or failed-set verification |\n| Restart persistence (R1 claim) | unverified | verified | unverified |\n| Failed-set contents (R3) | unverified | verified | asserted against a stub |\n| Approved unit tests (R4-R7) | approved | unchanged | unchanged |\n\nQuestion D8:\nD8 — Do the per-worker integration tests run the real job library, or a mocked hook?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: The whole point of D1 was that the library remembers attempt counts across worker restarts and keeps failed jobs somewhere you can find them. A test that fakes the library cannot check either of those; it only checks that your function was called. Running the real library against a throwaway test queue is slower but proves the parts you are relying on actually behave.\nStakes if we pick wrong: the retry count resets on every deploy or the failed set is empty when you need it, and every test was green because the mock said so.\nRecommendation: A because the library's persistence and failed set are load-bearing assumptions from D1 and D3, and a mock cannot verify either; the cost is a test backend fixture the library almost certainly already ships.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Real library backend in tests (recommended)\n ✅ Proves attempt count survives a worker restart and that exhausted jobs are actually in the failed set with their last error\n ✅ Catches library-version behavior changes and off-by-one attempt numbering that a stub would hide\n ❌ Slower suite and a test backend fixture to maintain (in-process queue or container)\nB) Mocked library hooks\n ✅ Fast, deterministic, no external fixture\n ✅ Enough to prove the worker wiring calls the shared policy\n ❌ Restart persistence and failed-set contents stay unverified, which are exactly the guarantees D1 and D3 depend on\nNet: a slower fixture that verifies the library promises you are betting on, or fast tests that trust them.\nHeader: Integration depth\nOptions:\nA) Real library backend in tests (recommended)\nPer-worker integration tests (5) run the actual job library against a test backend (in-process or containerized queue). Assertions: strategy invoked with real attempt numbers; attempt count survives a simulated worker restart mid-backoff; after the ceiling the job is in the failed set with its last error; a listed non-retryable error is in the failed set after attempt 1. Mark [E2E]. Completeness 9/10. human: ~1 day / CC: ~30 min.\nB) Mocked library hooks\nPer-worker tests stub the library retry hook and assert the shared policy is invoked with the expected arguments. No restart or failed-set verification. Completeness 6/10. human: ~2 h / CC: ~10 min.\n\nState: approved\nActual answer: A) Real library backend in tests (D8 answer, user selection)\nAccepted scope: Five per-worker integration tests [E2E] run the actual job library against a test backend (in-process or containerized queue). Assertions: strategy invoked with real attempt numbers; attempt count survives a simulated worker restart mid-backoff; after the ceiling the job is in the failed set with its last error; a listed non-retryable error is in the failed set after attempt 1.\nHistory: none\n\nSection 3 dispositions: TEST-1 (R7) accepted, CRITICAL; TEST-2 (R8) accepted. Gaps identified: 26 (all paths, no existing coverage detectable). LLM/eval scope: none.\nTest Plan Artifact: ~/.gstack/projects/gstack-plan-count-yWJb6k/runner-main-eng-review-test-plan-20260929-175707.md\n\n### R9: Per-retry payload re-fetch and dependency-graph recompute\nFinding: PERF-1, P2, confidence 7/10, PLAN.md:22-24, reviewer: plan-eng-review (native)\nPlan baseline: original proposal, \"On every retry we re-fetch the full job payload from the database, then iterate the payload to recompute the dependency graph. Could cache the graph on the first attempt; not planned\" (PLAN.md:22-24). R1 approved: library schedules retries and carries job data. R3 approved: ceiling 5.\nRuntime evidence: unknown. Payload size, graph cost and whether the library passes job data to the handler are not verifiable here.\nComparison grid:\n\n| Choice | Current | A) Compute once, store on job | B) In-process memo | C) Leave as-is |\n|---|---|---|---|---|\n| R9 payload read per attempt | DB fetch every attempt | read payload from the library's job data if present; DB fetch only on attempt 1 otherwise | DB fetch every attempt | DB fetch every attempt |\n| R9 graph build per attempt | recompute every attempt | build on attempt 1, persist serialized graph in job data; later attempts deserialize; guard: only valid because graph is a pure function of the immutable payload, assert payload hash matches | build once per worker process, cache keyed by job id; lost on restart and not shared across workers | recompute every attempt |\n| Reads under outage (N jobs x 5 attempts) | 5N reads + 5N builds | ~N reads + N builds | 5N reads, ~N-5N builds | 5N reads + 5N builds |\n| R1/R3 approved | approved | unchanged | unchanged | unchanged |\n\nQuestion D9:\nD9 — Cache the dependency graph across retries, or recompute it on every attempt?\nProject/branch/task: `main` of the plan fixture repo, plan \"Add background job retry framework\".\nELI10: Every time a job retries, the plan reads the whole payload from the database again and rebuilds the same dependency graph from it. The payload never changes between attempts, so the answer is always the same. With up to 5 attempts, that is up to 5 reads and 5 builds per failing job, and failing jobs pile up exactly when something is already down. Building once and storing the result with the job removes almost all of that.\nStakes if we pick wrong: a downstream outage turns into a database load spike from your own retries, or you spend effort caching something that turns out to be cheap.\nRecommendation: A because the graph is derived from an immutable payload, the library already stores job data per attempt, and the guard (payload hash check) makes the cache safe; it also removes the redundant DB fetch. Confidence is medium: if payloads are tiny and the graph build is microseconds, C is acceptable and this becomes a TODO.\nCompleteness: A=9/10, B=5/10, C=4/10\nPros / cons:\nA) Compute once, store on job (recommended)\n ✅ Retries read no extra payload and build no graph; outage-time DB load drops from 5N to about N\n ✅ Survives worker restarts and works across workers because the cache lives in the job data, not in a process\n ❌ Adds serialized graph size to each job record and needs a payload-hash guard to stay correct\nB) In-process memo\n ✅ Simple to add, no change to job data shape\n ✅ Helps when the same worker process picks up the retry\n ❌ Retries usually land on a different worker or after a restart, so the memo misses most of the time and still re-fetches the payload\nC) Leave as-is\n ✅ Zero new code and no cache correctness to reason about\n ✅ Bounded at 5 attempts by D3, so the waste is finite\n ❌ Every retry storm during an outage multiplies database reads by up to 5\nNet: one persisted derived value with a hash guard, or accept a 5x read multiplier exactly when the system is least healthy.\nHeader: Graph caching\nOptions:\nA) Compute once, store on job (recommended)\nOn attempt 1, read the payload (from the library's job data if it carries it, else one DB fetch), build the dependency graph, and persist the serialized graph plus a payload hash in the job data. On later attempts, verify the hash and deserialize; on mismatch, rebuild. Required proof: unit test that attempt 2+ performs no DB fetch and no graph build when the hash matches; test that a hash mismatch triggers a rebuild. Completeness 9/10. human: ~half day / CC: ~15 min.\nB) In-process memo\nMemoize the built graph per job id in worker memory. Payload re-fetch unchanged. Required proof: test that a second attempt in the same process reuses the graph. Completeness 5/10. human: ~1 h / CC: ~5 min.\nC) Leave as-is\nRe-fetch the payload and rebuild the graph on every attempt, as the plan proposes. No new tests. Completeness 4/10. human: 0 / CC: 0.\n\nState: approved\nActual answer: A) Compute once, store on job (D9 answer, user selection)\nAccepted scope: On attempt 1, read the payload (from the library's job data if it carries it, else one DB fetch), build the dependency graph, persist the serialized graph plus a payload hash in the job data. On later attempts, verify the hash and deserialize; on mismatch, rebuild. Required proof: unit test that attempt 2+ performs no DB fetch and no graph build when the hash matches; test that a hash mismatch triggers a rebuild.\nHistory: none\n\nSection 4 dispositions: PERF-1 (R9) accepted. Suppressed: job-record growth from serialized graph for very large payloads (confidence 4).\n\nOutside Voice: CODEX_MODE disabled (codex_reviews=disabled). No outside invocation, no native replacement. outside_status: disabled. Disabled record logged via gstack-review-log (skill codex-plan-review, status skipped).\n\n### R10 — TODO proposal: webhook at-least-once upgrade (TODO-1)\n\nFinding: D2 kept `processWebhookJob()` at at-most-once, so timeouts and 5xx responses are terminal and land in the failed set on attempt 1. That was logged as an accepted shortcut (decision id f9e8dfdf-4e90-4883-9064-014d784b9405) with upgrade trigger \"receivers can dedupe on a stable delivery id\". No TODOS.md exists in the repo.\nPlan baseline: none (the plan does not mention delivery semantics beyond the rewrite).\nRuntime evidence: none available; webhook receiver code is outside this repo.\nTODO record:\n What: Move webhook delivery to at-least-once with a stable delivery id and idempotency key once receivers can dedupe.\n Why: Under at-most-once, every receiver timeout or 5xx is a lost delivery that only a manual replay recovers. At-least-once turns those into automatic retries.\n Pros: closes the biggest remaining reliability hole; reuses the retryPolicy module and failed-set tooling from this change; the characterization suite from D7 already covers the send path.\n Cons: needs receiver-side dedupe first (external dependency); duplicate deliveries during the transition if a receiver lags; signature scheme may need a delivery-id header.\n Context: after this plan lands, timeouts/5xx go to the failed set with `gstack-shortcut(dec-f9e8dfdf)` marking the cut in `processWebhookJob()`. Start there: add a `delivery_id` to the payload, publish a dedupe contract to receivers, then flip timeouts/5xx from terminal to retryable in the webhook worker's non-retryable list.\n Depends on / blocked by: receivers exposing dedupe on delivery id; this plan's T4 (webhook rewrite) merged.\n Effort: M. Priority: P3.\nComparison grid:\n A) Add to TODOS.md: keeps the upgrade trigger visible outside the code comment; zero build cost now. Completeness: kind choice.\n B) Skip: nothing recorded beyond the decision log entry and the code marker. Completeness: kind choice.\n C) Build now: extends this PR to at-least-once, contradicting D2's answer. Completeness: kind choice.\nQuestion D10 (full text):\nD10 — Record the webhook at-least-once upgrade as a TODO?\nProject/branch/task: gstack-plan-count-yWJb6k on main, retry framework plan; follow-up to D2.\nELI10: In D2 you chose to keep webhooks at \"send at most once\", so a slow or erroring receiver means that delivery is dropped into the failed set instead of retried. The fix (retry with a delivery id the receiver can dedupe) needs receiver work first. This question only decides whether we write that follow-up down in TODOS.md so it does not get lost.\nStakes if we pick wrong: skipped, the only trace is a code comment and a decision-log row; built now, this PR grows and contradicts the D2 call.\nRecommendation: A because the upgrade has an external prerequisite and a clear trigger, which is exactly what a TODO is for.\nNote: options differ in kind, not coverage — no completeness score.\nPros / cons:\nA) Add to TODOS.md (recommended)\n ✅ Creates TODOS.md with the What/Why/Context/Depends-on record above, findable by /retro and future reviews\n ✅ Zero implementation cost now; the marker in code and the TODO entry point at each other\n ❌ One more file in the repo that someone has to keep honest as work lands\nB) Skip\n ✅ No new file; the decision log and gstack-shortcut marker already carry the trigger\n ✅ Avoids a TODO nobody may pick up if receivers never add dedupe\n ❌ The trigger lives only in a comment and a JSONL row, easy to miss when receivers do change\nC) Build it now in this PR\n ✅ Ships the stronger delivery guarantee in the same change as the retry framework\n ✅ Reuses the retryPolicy module while it is fresh\n ❌ Reverses D2 and depends on receiver dedupe that does not exist yet, so duplicates would reach receivers\nNet: a TODO entry now versus relying on a code marker alone; building now is off the table until receivers can dedupe.\nHeader: Webhook TODO\nOptions:\nA) Add to TODOS.md (recommended)\nCreate TODOS.md at implementation time with the TODO record above under a `## Workers` section (P3, effort M). No product code change.\nB) Skip\nDo not create TODOS.md. The decision log entry and the gstack-shortcut marker remain the only trail.\nC) Build it now in this PR\nExtend the accepted scope to at-least-once delivery with delivery id and idempotency key; would reopen D2.\n\nState: approved\nActual answer: A) Add to TODOS.md (D10 answer, user selection)\nAccepted scope: At implementation time, create TODOS.md with a `## Workers` section holding the TODO-1 record above (What/Why/Context/Effort M/Priority P3/Depends on). No product code change beyond the file. This is a documentation task (T9 below).\nHistory: none\n\nTODOS.md updates: 1 item proposed, 1 accepted (TODO-1).\n\nApproval readiness: PASS. Checked R1 (D1=A), R2 (D2=A, accepted shortcut dec-f9e8dfdf-4e90-4883-9064-014d784b9405), R3 (D3=A), R4 (D4=A), R5 (D5=A), R6 (D6=A), R7 (D7=A, CRITICAL regression contract carried forward verbatim into T1), R8 (D8=A), R9 (D9=A), R10 (D10=A). Every accepted remedy cites its own actual user answer. No deferrals. No pending records.\n\n## Working plan (revised): Add background job retry framework\n\n### Context\nThe five background workers have no shared retry behavior today. The original plan proposed an inline exponential-backoff scheduler copied into each worker, bypassing the job library's retry hooks, with no bound on attempts, no dead-letter path, no regression coverage for the webhook worker's at-most-once guarantee, and a full payload re-fetch plus dependency-graph rebuild on every attempt. This review replaced each of those with a decision the user approved (D1 to D10). The revised plan below is the only approved version; the original is preserved above under \"Original plan (unchanged copy)\".\n\n### Architecture (D1, D3, D4, D5)\n- Use the job library's built-in retry hooks. The custom curve lives in one backoff strategy function passed to the library. First implementation step: confirm the hook accepts a delay function; if it only accepts a fixed table, keep the library's attempt bookkeeping and supply the computed delay per attempt.\n- Backoff: exponential base curve with equal jitter, delay drawn from `[curve/2, curve]`, RNG injectable for tests, configurable max-delay clamp.\n- Attempts: `DEFAULT_MAX_ATTEMPTS = 5`, per-worker override, configured alongside the strategy. Attempt N+1 is never scheduled.\n- Exhaustion or a listed non-retryable error sends the job to the library's failed/dead-letter set with the last error attached, emits one structured log line and one metric. Retention and replay are documented (T8).\n- Each of the four non-webhook workers declares its own non-retryable error list (validation, auth/permission, malformed payload; confirm the concrete classes at implementation). Listed errors dead-letter on attempt 1; everything else retries.\n\n### Webhook delivery (D2, accepted shortcut dec-f9e8dfdf-4e90-4883-9064-014d784b9405)\n- `processWebhookJob()` keeps at-most-once. Only pre-send failures retry (connection refused, DNS failure, errors raised before bytes leave). Receiver timeouts and 5xx responses are terminal and go to the failed set on attempt 1.\n- Ceiling: timeouts and 5xx are never retried. Upgrade trigger: receivers can dedupe on a stable delivery id, then move to at-least-once with an idempotency key (TODO-1).\n- Mark the cut in code at the classification point: `gstack-shortcut(dec-f9e8dfdf): timeouts/5xx never retried, upgrade when receivers can dedupe on a stable delivery id`.\n\n### Code quality (D6)\n- One `retryPolicy` module exporting `backoffStrategy(attempt, rng)`, `DEFAULT_MAX_ATTEMPTS`, `onDeadLetter(job, err)`, `isNonRetryable(err, list)`.\n- Structured attempt log fields: job id, attempt, delay, error class, decision (retry | dead-letter | success).\n- ASCII state diagram in the module header (copied below under Diagrams).\n- All five workers register through this module with their own non-retryable list. The refactor commit lands before any behavior commit.\n\n### Tests (D7, D8)\n- CRITICAL and first: a characterization suite for `processWebhookJob()` pinning exact request body, headers and signature for a fixed payload; exactly one send on success, on timeout and on 5xx; and success bookkeeping. It must be green before the rewrite starts and stay green through it. Then intentional-difference tests: pre-send failure schedules a retry; timeout/5xx land in the failed set with no second send.\n- Shared-contract unit tests per `retryPolicy` export, including the clamp and the jitter bounds with a pinned RNG.\n- Five per-worker [E2E] integration tests against the real job library on a test backend: strategy invoked with real attempt numbers; attempt count survives a simulated restart mid-backoff; exhausted job in the failed set with last error; listed non-retryable error in the failed set after attempt 1.\n- Test Plan artifact: `~/.gstack/projects/gstack-plan-count-yWJb6k/runner-main-eng-review-test-plan-20260929-175707.md` (unchanged by later decisions).\n\n### Performance (D9)\n- On attempt 1 read the payload (from the library's job data if present, else one DB fetch), build the dependency graph, persist the serialized graph plus a payload hash in the job data. Later attempts verify the hash and deserialize; a mismatch triggers a rebuild.\n\n### Follow-ups (D10)\n- Create `TODOS.md` with TODO-1 (webhook at-least-once upgrade, P3, effort M) under `## Workers`.\n\n## NOT in scope\n- At-least-once webhook delivery with idempotency keys: deferred to TODO-1 because receivers cannot dedupe yet (D2, D10).\n- A custom scheduler outside the job library: rejected in D1; the library owns attempt bookkeeping.\n- Retry budgets or circuit breakers across workers during a downstream outage: not raised by the plan; jitter plus a 5-attempt bound is the accepted mitigation (D3, D4).\n- In-process graph memoization: rejected in D9 in favor of persisting the graph on the job.\n- Per-attempt webhook signature timestamp handling: suppressed at confidence 4; revisit if the signature scheme includes a timestamp that receivers validate.\n\n## What already exists\n- The job library's retry hooks, attempt counter and failed/dead-letter set: reused, not rebuilt (D1, D3). The plan's \"same shape as the library version\" line was the tell that rebuilding added nothing.\n- The existing `processWebhookJob()` send path, headers and signature code: preserved behind the characterization suite (D7); the rewrite changes the retry envelope around it, not the request it produces.\n- The current dependency-graph builder: reused once per job on attempt 1 (D9); only the caching wrapper is new.\n- Shared-code rubric for the `retryPolicy` extraction (D6): callers = 5 workers; reuse-before-extract = no existing shared retry helper found in the plan or the (unavailable) worker files, so extraction is the reuse; helper size = four small exports; line accounting = removes five copies of the envelope, adds one module (net negative); blast radius = all five workers, mitigated by the refactor-first commit and one integration test per worker (D8).\n\n## Diagrams\n\nRetry flow through the library hooks:\n\n```\nenqueue ──▶ worker handler ──▶ success ──▶ done (log decision=success)\n │\n ▼ throws err\n isNonRetryable(err, list)? ──yes──▶ onDeadLetter(job, err) ──▶ failed set\n │ no (1 log line + 1 metric)\n ▼\n attempt < maxAttempts? ──no──▶ onDeadLetter(job, err) ──▶ failed set\n │ yes\n ▼\n delay = backoffStrategy(attempt, rng) [curve/2, curve], clamped\n │\n ▼\n library schedules attempt+1 ──▶ (restart-safe: count lives in the library)\n```\n\n`retryPolicy` state diagram (also goes in the module header):\n\n```\n ┌──────────┐ ok ┌─────────┐\n ──────▶ │ ATTEMPT n│ ────▶ │ SUCCESS │\n └──────────┘ └─────────┘\n │ err\n ▼\n ┌──────────────┐ listed ┌─────────────┐\n │ classify err │ ─────▶ │ DEAD_LETTER │ ◀──┐\n └──────────────┘ └─────────────┘ │\n │ retryable │ n == max\n ▼ │\n ┌──────────────┐ ──────────────────────────┘\n │ n < max ? │\n └──────────────┘\n │ yes\n ▼\n ┌──────────────┐ library timer ┌────────────┐\n │ BACKOFF(n) │ ──────────────▶ │ ATTEMPT n+1│\n └──────────────┘ └────────────┘\n```\n\nWebhook worker classification (at-most-once):\n\n```\nsend attempt\n ├─ pre-send failure (ECONNREFUSED, DNS, serialization) ──▶ retryable ──▶ BACKOFF\n ├─ timeout after bytes sent ──▶ terminal ──▶ DEAD_LETTER (gstack-shortcut dec-f9e8dfdf)\n ├─ 5xx ──▶ terminal ──▶ DEAD_LETTER (gstack-shortcut dec-f9e8dfdf)\n └─ 2xx ──▶ SUCCESS\n```\n\nGraph cache on job data (D9):\n\n```\nattempt 1: payload ──▶ build graph ──▶ job.data = {graph, payloadHash}\nattempt n: job.data.payloadHash == hash(payload)? ──yes──▶ deserialize graph\n └─no───▶ rebuild + overwrite\n```\n\nFiles needing inline diagrams: the `retryPolicy` module header (state diagram above); the webhook worker's classification block (the at-most-once branch table above).\n\n## Failure modes\n\n| Path | Realistic production failure | Test coverage | Error handling | User-visible? | Gap |\n|------|------------------------------|---------------|----------------|---------------|-----|\n| Library hook + strategy | Hook ignores the returned delay and uses its default table | Integration test asserts strategy invoked with real attempt numbers (T6) | Startup assertion that the hook accepted a function (T2) | Ops see wrong delays in attempt logs | covered |\n| Backoff + jitter | RNG returns out-of-range value, delay negative or above clamp | Unit tests for bounds and clamp (T2) | Clamp in `backoffStrategy` | None | covered |\n| Attempt bound | Restart mid-backoff resets the count and retries forever | Restart test (T6) | Count lives in the library, not the process | Ops see repeated attempts | covered |\n| Dead-letter | Exhausted job dropped without log or metric | Unit test log+metric fire once (T2), integration test entry in failed set (T6) | `onDeadLetter` always called on both exits | Ops alert fires | covered |\n| Non-retryable list | Validation error retried 5 times, wasting the window | Per-worker listed/unlisted tests (T5) | `isNonRetryable` short-circuit | None | covered |\n| Webhook at-most-once | Rewrite silently double-sends on timeout; receivers see duplicates | Characterization suite (T1) pins one send on timeout/5xx | Timeout/5xx classified terminal | Receivers, not our users | **critical gap in the original plan**, closed by T1 |\n| Graph cache | Payload changed after attempt 1, stale graph used | Hash-mismatch rebuild test (T7) | Hash guard | Silent if guard missing | covered |\n\nCritical gaps flagged: 1 (webhook at-most-once regression; the original plan had no test, no handling, and the failure would have been silent). Closed by T1, which gates T4.\n\n## Worktree parallelization strategy\n\nDependency table:\n\n| Step | Modules touched | Depends on |\n|------|----------------|------------|\n| S1 Webhook characterization suite (T1) | webhook worker tests | — |\n| S2 retryPolicy module + unit tests (T2) | new retryPolicy module, its tests | — |\n| S3 Migrate 4 non-webhook workers + non-retryable lists (T3, T5) | 4 worker modules, their tests | S2 |\n| S4 Webhook rewrite on retryPolicy (T4) | webhook worker | S1, S2 |\n| S5 Per-worker integration tests (T6) | integration test suite, test backend config | S3, S4 |\n| S6 Graph cache on job data (T7) | dependency-graph builder, the worker that owns it | — (S3 if that worker is one of the four) |\n| S7 Dead-letter runbook + TODOS.md (T8, T9) | docs | — |\n\nParallel lanes:\n- Lane A: S2 → S3 → S5 (shared retryPolicy module and four workers)\n- Lane B: S1 → [wait for S2] → S4 (webhook worker only)\n- Lane C: S6 (graph builder; independent unless it lives in one of the four migrated workers)\n- Lane D: S7 (docs only)\n\nExecution order: launch A, B, C, D together. B blocks at S4 until A finishes S2. Merge A and B, then run S5 against both. Merge C and D whenever green.\n\nConflict flags: the webhook worker is touched only by Lane B; the four other workers only by Lane A. If the graph builder sits inside one of the four workers, sequence Lane C after S3 instead of running it in parallel. Lane D touches no code.\n\n## Implementation Tasks\nSynthesized from this review's findings. Each task derives from a specific\nfinding above. Run with Claude Code or Codex; checkbox as you ship.\n\n- [ ] **T1 (P1, human: ~1 day / CC: ~20 min)** — webhook worker tests — Write the `processWebhookJob()` characterization suite before touching the function (CRITICAL)\n - Surfaced by: Test review — TEST-1 (R7): no regression test for the at-most-once guarantee\n - Files: webhook worker test module (paths not present in this checkout)\n - Verify: suite green on current code; asserts exact body/headers/signature, one send on success, timeout and 5xx, success bookkeeping\n- [ ] **T2 (P1, human: ~1 day / CC: ~20 min)** — retryPolicy module — Create `retryPolicy` exporting `backoffStrategy`, `DEFAULT_MAX_ATTEMPTS`, `onDeadLetter`, `isNonRetryable`, with state diagram in the header\n - Surfaced by: Architecture — ARCH-1 (R1), ARCH-2 (R3), ARCH-3 (R4); Code quality — CQ-1 (R6)\n - Files: new retryPolicy module + unit tests; confirm the library hook accepts a delay function first\n - Verify: unit tests for jitter bounds `[curve/2, curve]` with pinned RNG, clamp, log+metric fire once, `isNonRetryable` on listed/unlisted errors\n- [ ] **T3 (P1, human: ~1.5 days / CC: ~30 min)** — 4 non-webhook workers — Register each worker through `retryPolicy` and delete the copy-pasted envelopes, refactor commit before any behavior change\n - Surfaced by: Code quality — CQ-1, CQ-2, CQ-3 (R6)\n - Files: the four non-webhook worker modules\n - Verify: existing worker tests green after the refactor commit; no inline delay computation remains (grep for the old envelope)\n- [ ] **T4 (P1, human: ~1 day / CC: ~20 min)** — webhook worker — Rewrite `processWebhookJob()` on `retryPolicy` keeping at-most-once; add the `gstack-shortcut(dec-f9e8dfdf): timeouts/5xx never retried, upgrade when receivers can dedupe on a stable delivery id` marker at the classification point\n - Surfaced by: Architecture — ARCH-1 (R2, accepted shortcut); Test review — TEST-1 (R7) intentional-difference tests\n - Files: webhook worker module + its tests\n - Verify: T1 suite still green; new tests show pre-send failure schedules a retry, timeout/5xx land in failed set with no second send\n- [ ] **T5 (P1, human: ~half day / CC: ~15 min)** — 4 non-webhook workers — Declare each worker's non-retryable error list (validation, auth/permission, malformed payload; confirm classes) and pass it at registration\n - Surfaced by: Architecture — ARCH-4 (R5)\n - Files: the four non-webhook worker modules + tests\n - Verify: per worker, listed error dead-letters on attempt 1 with no retry; unlisted error retries\n- [ ] **T6 (P1, human: ~2 days / CC: ~40 min)** — integration test suite — Add five per-worker [E2E] tests against the real job library on a test backend\n - Surfaced by: Test review — TEST-2 (R8)\n - Files: integration test suite, test backend configuration\n - Verify: strategy invoked with real attempt numbers; attempt count survives simulated restart mid-backoff; exhausted job in failed set with last error; listed non-retryable in failed set after attempt 1\n- [ ] **T7 (P2, human: ~half day / CC: ~15 min)** — dependency-graph builder — Build the graph once on attempt 1 and persist it with a payload hash in job data; verify hash and deserialize on later attempts\n - Surfaced by: Performance — PERF-1 (R9)\n - Files: graph builder and the worker that owns it\n - Verify: attempt 2+ performs no DB fetch and no build when the hash matches; mismatch triggers a rebuild\n- [ ] **T8 (P2, human: ~2 h / CC: ~5 min)** — docs — Document failed-set retention and the replay procedure\n - Surfaced by: Architecture — ARCH-2 (R3): retention/replay documented\n - Files: ops/runbook doc next to the workers\n - Verify: an operator can replay one job from the failed set following the doc alone\n- [ ] **T9 (P3, human: ~15 min / CC: ~2 min)** — docs — Create `TODOS.md` with TODO-1 (webhook at-least-once upgrade) under `## Workers`\n - Surfaced by: TODOS.md updates — R10 (D10=A)\n - Files: TODOS.md\n - Verify: entry has What/Why/Context/Effort M/Priority P3/Depends on\n\nEffort assumption: tests at ~50x, module extraction at ~30x, docs at ~20x human-to-CC ratio; paths are unavailable in this checkout, so estimates assume five workers of ordinary size.\n\n## Unresolved decisions that may bite you later\nNone. D1 through D10 all answered.\n\n## Completion summary\n- Step 0: Scope Challenge — scope accepted as-is (D1 changed mechanism, not feature scope)\n- Architecture Review: 4 issues found\n- Code Quality Review: 3 issues found\n- Test Review: diagram produced, 26 gaps identified\n- Performance Review: 1 issue found\n- NOT in scope: written\n- What already exists: written\n- TODOS.md updates: 1 item proposed to user (accepted)\n- Failure modes: 1 critical gap flagged (closed by T1)\n- Unresolved decisions: 0 in this review\n- Outside voice: codex, disabled (codex_reviews disabled; recorded as outside_status disabled, no native replacement)\n- Parallelization: 4 lanes, 3 parallel / 1 sequential (Lane B waits on Lane A's S2 before the webhook rewrite)\n- Lake Score: 0/9 = 10/10 choices / answered coverage choices (D10 was a kind choice, excluded)\n- issues_found for the log: 4 + 3 + 1 + 26 = 34 (Scope Challenge SC-1 and Outside Voice reported separately)\n\n## Suppressed findings\n- Webhook signature timestamp on retried attempts: if the signature covers a timestamp, a retried pre-send failure re-signs with a new time; receivers with tight windows may reject. Confidence 4/10; the signature scheme is not visible in this checkout.\n- Job-record growth from the serialized graph for very large payloads (D9). Confidence 4/10; payload sizes unknown.\n- Retry-storm memory pressure from many simultaneous backoff timers. Confidence 4/10; the library owns timers under D1, so this is likely moot.\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 0 | — | — |\n| Outside Review | codex via `/plan-eng-review` Outside Voice | Independent 2nd opinion | 1 | DISABLED | none (codex_reviews disabled, phase plan-review) |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 1 | ISSUES OPEN (this run) | 34 issues, 1 critical gaps |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | — |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | — |\n\n**OUTSIDE COVERAGE:** codex, phase plan-review, disabled (codex_reviews disabled, logged 2026-09-29T18:01:01Z, source none, host claude), no findings. Native review does not substitute for outside coverage.\n\n**VERDICT:** No reviews CLEAR. Eng Review ISSUES OPEN: 34 findings mapped to 9 implementation tasks, 0 unresolved decisions, 1 critical gap closed by T1. eng review required.\n\nNO UNRESOLVED DECISIONS\n" +} diff --git a/test/helpers/eng-seeded-coverage.ts b/test/helpers/eng-seeded-coverage.ts index 7ccde9d7f..658dee8a2 100644 --- a/test/helpers/eng-seeded-coverage.ts +++ b/test/helpers/eng-seeded-coverage.ts @@ -117,6 +117,9 @@ export function isEngBatchingIssueAUQ(fp: AskUserQuestionFingerprint, priorCalls return !priorCalls.some(prior => batchingIssueNumber(prior) === issue); } +// The report's one target declaration, in the skill's own spellings. +const TARGET_FIELD = /^(?:Reviewed |Review )?[Tt]arget(?: \(fixed\))?:/; + /** A native brief can use its D number and topic while its stable R identity * lives in the required saved ledger. Count that owned choice, not a title * spelling. This does not approve the row or validate the implementation. */ @@ -160,10 +163,10 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string): const rawSourceNames = [...(lines[1] ?? '').matchAll(/\b[\w./-]+\.md\b/g)]; const directSource = sourceNames.length > 0 && sourceNames.every(name => name === 'PLAN.md') && new Set([...metadata.matchAll(/\bPLAN\.md:([1-9]\d*(?:[-–][1-9]\d*)?)\b/g)].map(match => match[1])).size <= 1; - const targetName = (s: string) => clean(s).replace(/^Eng(?:ineering)? review:\s*/i, '') + const targetName = (s: string) => clean(s).replace(/^Eng(?:ineering)? review\s*[:—–-]\s*/i, '') .replace(/^Plan\s*[:—–-]\s*/i, '').toLowerCase(); - const named = [...(lines[1] ?? '').matchAll(/"(Plan:\s*[^"\n]+)"|“(Plan:\s*[^”\n]+)”/g)] - .map(match => targetName(match[1] ?? match[2]!)); + const named = [...(lines[1] ?? '').matchAll(/"(Plan:\s*[^"\n]+)"|“(Plan:\s*[^”\n]+)”|\b[Pp]lan\s+"([^"\n]+)"|\b[Pp]lan\s+“([^”\n]+)”/g)] + .map(match => targetName(match[1] ?? match[2] ?? match[3] ?? match[4]!)); const titles = tokens.slice(0, start).filter(token => token.type === 'heading' && token.depth === 1); const targetFields = tokens.slice(0, start).flatMap((token, at) => { if (token.type !== 'paragraph' || !currentHeading(at)) return []; @@ -171,14 +174,16 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string): const quotedContext = /\b(?:quoted|copied|historical|example|hypothetical|archived)\b[^\n]*:\s*$/i; if (previous?.type === 'paragraph' && quotedContext.test(previous.raw)) return []; const parts = token.raw.split('\n'); - return parts.filter((line, i) => /^Reviewed target:/.test(line) && + return parts.filter((line, i) => TARGET_FIELD.test(line) && !parts.slice(0, i).some(part => quotedContext.test(part))); }); - const namedSource = !rawSourceNames.length && named.length === 1 && titles.length === 1 && + // The report title owns the target; an unfenced copy of the reviewed plan + // may add its own H1 only when it names that same plan. + const namedSource = !rawSourceNames.length && named.length === 1 && titles.length >= 1 && titles[0]!.type === 'heading' && currentHeading(tokens.indexOf(titles[0]!)) && - /^Eng(?:ineering)? review:\s*Plan\s*[:—–-]/i.test(clean(titles[0]!.text)) && - targetName(titles[0]!.text) === named[0] && targetFields.length === 1 && - /^Reviewed target:\s*`?PLAN\.md`?(?:\s|$)/.test(targetFields[0]!) && + /^Eng(?:ineering)? review\s*[:—–-]\s*\S/i.test(clean(titles[0]!.text)) && + titles.every(title => title.type === 'heading' && targetName(title.text) === named[0]) && targetFields.length === 1 && + new RegExp(`${TARGET_FIELD.source}\\s*\`?PLAN\\.md\`?(?:\\s|$)`).test(targetFields[0]!) && [...targetFields[0]!.matchAll(/\b[\w./-]+\.md\b/g)].length === 1; if (!directSource && !namedSource) return; const withdrawn = (value: string, owners: string) => new RegExp( @@ -265,7 +270,7 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string): if (questions.length !== 1) continue; const inlineBrief = fields[questions[0]!]!.slice(marker.length).trim(); const inline = Boolean(inlineBrief); - if (!inline && (namedSource || clean(fields[questions[0]! + 1] ?? '') !== clean(title))) continue; + if (!inline && clean(fields[questions[0]! + 1] ?? '') !== clean(title)) continue; const sources = [...finding[0]!.matchAll(/\b([\w./-]+\.md)(?::([1-9]\d*(?:[-–][1-9]\d*)?))?\b/g)]; if (sources.length !== 1 || sources[0]![1] !== 'PLAN.md' || !inline && !sources[0]![2] || source && sources[0]![2] !== source) continue;