From b3a26090e666b9c4ac3d59d643be9106e236fd0a Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 1 Oct 2026 17:08:34 +0000 Subject: [PATCH] =?UTF-8?q?test(plan-eng=20batching):=20count=20saved=20de?= =?UTF-8?q?cisions=20whose=20label=20drops=20the=20(recommended)=20marker?= =?UTF-8?q?=20or=20whose=20report=20is=20titled=20'Eng=20Review=20Report?= =?UTF-8?q?=20=E2=80=94=20'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/eng-seeded-coverage.test.ts | 33 +++++++++ ...eng-batching-report-title-36798539821.json | 69 +++++++++++++++++++ test/helpers/eng-seeded-coverage.ts | 8 ++- 3 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/eng-batching-report-title-36798539821.json diff --git a/test/eng-seeded-coverage.test.ts b/test/eng-seeded-coverage.test.ts index 59df5a208..cfc74af68 100644 --- a/test/eng-seeded-coverage.test.ts +++ b/test/eng-seeded-coverage.test.ts @@ -192,3 +192,36 @@ describe('batching replay of a 2.1.284 rerun (bullet target, unnamed plan)', () expect(count(plan)).toEqual([]); }); }); + +describe('saved ledger from run 36798539821: report title and (recommended) marker', () => { + const reportTitleCapture: { calls: NativePlanQuestionCall[]; plans: string[] } = JSON.parse( + fs.readFileSync(path.join(import.meta.dir, 'fixtures/eng-batching-report-title-36798539821.json'), 'utf8')); + const [d1, d3] = reportTitleCapture.calls; + const [d1Plan, d3Plan] = reportTitleCapture.plans; + const countReportTitle = (call: NativePlanQuestionCall, plan: string, prior: NativePlanQuestionCall[] = []) => + createEngBatchingIssueCounter(() => plan, engSetupAUQ).isReviewAUQ(nativePlanCallFingerprint(structuredClone(call), 0, true), prior); + + test('a saved option label without the native (recommended) marker still owns the decision', () => { + expect(d1!.questions[0]!.options[0]!.label).toBe('Library hooks + custom backoff (recommended)'); + expect(d1Plan).toContain('\nA) Library hooks + custom backoff\n'); + expect(countReportTitle(d1!, d1Plan!)).toBe(true); + }); + + test('an unsourced brief inherits PLAN.md from an "Eng Review Report — " title', () => { + expect(d3Plan!.split('\n')[0]).toBe('# Eng Review Report — Add background job retry framework'); + expect(d3!.questions[0]!.question.split('\n')[1]).not.toMatch(/\.md\b/); + expect(countReportTitle(d3!, d3Plan!, [d1!])).toBe(true); + }); + + test('rejects a saved label that changes the choice, not just the marker', () => { + expect(countReportTitle(d1!, d1Plan!.replace('\nA) Library hooks + custom backoff\n', '\nA) Library hooks without custom backoff\n'))).toBe(false); + }); + + test('rejects a report title that names a different plan', () => { + expect(countReportTitle(d3!, d3Plan!.replace('# Eng Review Report — Add background job retry framework', '# Eng Review Report — Rewrite the billing service'), [d1!])).toBe(false); + }); + + test('rejects a report title with an unrelated prefix', () => { + expect(countReportTitle(d3!, d3Plan!.replace('# Eng Review Report — ', '# Copied Review Notes — '), [d1!])).toBe(false); + }); +}); diff --git a/test/fixtures/eng-batching-report-title-36798539821.json b/test/fixtures/eng-batching-report-title-36798539821.json new file mode 100644 index 000000000..5e2dcce85 --- /dev/null +++ b/test/fixtures/eng-batching-report-title-36798539821.json @@ -0,0 +1,69 @@ +{ + "source": "Periodic Evals run 36798539821, eval-slices (1), /plan-eng-review multi-finding batching; native calls D1 and D3 with the saved report as it stood 3s after each answer", + "calls": [ + { + "sessionId": "b3ebef1e-8b0f-4613-ab80-392f4e780930", + "toolUseId": "toolu_01SKGoBCLaoNJhSFiGFmqMYV", + "questions": [ + { + "question": "D1 — Retry engine: library hooks or hand-rolled?\nProject/branch/task: main — plan \"Add background job retry framework\", reviewing PLAN.md Architecture section.\nELI10: Your job library already knows how to retry a failed job later; the plan wants to rebuild that part by hand inside every worker, just so the wait-time curve is ours. Almost every job library lets you plug in your own curve through its retry hook, so you get the curve you want without also owning scheduling, attempt counting, persistence across process restarts, and dead-lettering. The stakes are a second retry system nobody else on the team knows, living in five copies.\nStakes if we pick wrong: a hand-rolled scheduler loses in-flight retries on deploy or crash, drifts from the library's attempt bookkeeping, and every retry bug is ours to find at 3am. Users see jobs that silently never run again.\nRecommendation: A because the plan admits the library already has the shape (PLAN.md:8); supplying a backoff function keeps full control of the curve at a fraction of the surface area. [Layer 1: reuse the installed dependency.]\nCompleteness: A=9/10, B=6/10, C=n/a (investigation, differs in kind)\nPros / cons:\nA) Library hooks + custom backoff (recommended) (human: ~1 day / CC: ~20 min)\n ✅ Full control of the delay curve via one backoff function; the library still persists attempts and survives restarts\n ✅ Dead-letter, max-attempts and retry visibility come for free from the library's existing dashboard and tooling\n ❌ If the library's hook cannot express the curve (verify first), you are back to B for that one worker\nB) Custom inline scheduler (human: ~3-4 days / CC: ~1 hr, plus ongoing maintenance)\n ✅ Zero dependence on the library's retry semantics; curve and scheduling are entirely ours\n ✅ Works even if the library's hook turns out to be too rigid\n ❌ Re-implements persistence, attempt tracking and dead-lettering; in-flight retries die with the process unless you also build durability\nC) Investigate hook API first (CC: ≤ 30 min)\n ✅ Replaces an assumption with a verified fact before committing either way\n ✅ Cheap; the answer feeds directly into A or B\n ❌ Decides nothing today; the mechanism stays pending and blocks the Code Quality question that depends on it\nNet: you are trading a few lines of backoff function (A) against owning a second retry system (B); C buys certainty for half an hour.", + "header": "Retry engine", + "multiSelect": false, + "options": [ + { + "label": "Library hooks + custom backoff (recommended)", + "description": "Use the existing job library's retry hook and supply one custom backoff function that returns the delay for attempt N. The library keeps scheduling, attempt persistence, max-attempts and dead-letter. Full control of the curve. Human: ~1 day / CC: ~20 min. Risk: low; verify the hook accepts a function before coding." + }, + { + "label": "Custom inline scheduler", + "description": "Hand-roll the exponential-backoff scheduler inside each worker as PLAN.md:7-9 proposes. We own scheduling, attempt counting, persistence across restarts and dead-lettering. Human: ~3-4 days / CC: ~1 hr plus ongoing maintenance. Risk: medium-high; in-flight retries are lost on crash or deploy unless durability is also built." + }, + { + "label": "Investigate hook API first", + "description": "Bounded investigation only: read the library's retry/backoff hook API and report whether a custom curve function is supported. CC: ≤ 30 min. Approves no implementation; the retry mechanism stays pending and R2-R5 stay pending." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Retry engine: library hooks or hand-rolled?\nProject/branch/task: main — plan \"Add background job retry framework\", reviewing PLAN.md Architecture section.\nELI10: Your job library already knows how to retry a failed job later; the plan wants to rebuild that part by hand inside every worker, just so the wait-time curve is ours. Almost every job library lets you plug in your own curve through its retry hook, so you get the curve you want without also owning scheduling, attempt counting, persistence across process restarts, and dead-lettering. The stakes are a second retry system nobody else on the team knows, living in five copies.\nStakes if we pick wrong: a hand-rolled scheduler loses in-flight retries on deploy or crash, drifts from the library's attempt bookkeeping, and every retry bug is ours to find at 3am. Users see jobs that silently never run again.\nRecommendation: A because the plan admits the library already has the shape (PLAN.md:8); supplying a backoff function keeps full control of the curve at a fraction of the surface area. [Layer 1: reuse the installed dependency.]\nCompleteness: A=9/10, B=6/10, C=n/a (investigation, differs in kind)\nPros / cons:\nA) Library hooks + custom backoff (recommended) (human: ~1 day / CC: ~20 min)\n ✅ Full control of the delay curve via one backoff function; the library still persists attempts and survives restarts\n ✅ Dead-letter, max-attempts and retry visibility come for free from the library's existing dashboard and tooling\n ❌ If the library's hook cannot express the curve (verify first), you are back to B for that one worker\nB) Custom inline scheduler (human: ~3-4 days / CC: ~1 hr, plus ongoing maintenance)\n ✅ Zero dependence on the library's retry semantics; curve and scheduling are entirely ours\n ✅ Works even if the library's hook turns out to be too rigid\n ❌ Re-implements persistence, attempt tracking and dead-lettering; in-flight retries die with the process unless you also build durability\nC) Investigate hook API first (CC: ≤ 30 min)\n ✅ Replaces an assumption with a verified fact before committing either way\n ✅ Cheap; the answer feeds directly into A or B\n ❌ Decides nothing today; the mechanism stays pending and blocks the Code Quality question that depends on it\nNet: you are trading a few lines of backoff function (A) against owning a second retry system (B); C buys certainty for half an hour.": "Library hooks + custom backoff (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-10-01T00:59:59.441Z" + }, + { + "sessionId": "b3ebef1e-8b0f-4613-ab80-392f4e780930", + "toolUseId": "toolu_012FpcaAT9TcStMcPRVJno8y", + "questions": [ + { + "question": "D3 — Backoff jitter: randomize retry delays?\nProject/branch/task: main — plan \"Add background job retry framework\", custom backoff function approved in D1.\nELI10: When a downstream service goes down, every job that hit it fails at the same moment. Without jitter they all come back at the same moment too, hammering the service exactly when it is trying to recover, and failing together again. Jitter spreads retries randomly across the window so the herd becomes a trickle. The plan says \"full control over the curve\" but never mentions it.\nStakes if we pick wrong: a recovering dependency gets knocked over by our own synchronized retry wave; the outage lasts longer and the fix looks like \"the retries made it worse\".\nRecommendation: A because full jitter is one line inside the backoff function and it is the standard remedy for synchronized retry storms.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Full jitter (recommended) (human: ~1 hr / CC: ~3 min)\n ✅ Five workers retrying the same dead dependency spread their load across the window instead of in lockstep\n ✅ One line in the backoff function; the library still owns scheduling so nothing else changes\n ❌ Retry timing becomes non-deterministic, so tests must seed or inject the random source\nB) No jitter (human: ~0 / CC: ~0)\n ✅ Deterministic delays are trivial to assert in tests and to reason about in logs\n ✅ Nothing to build\n ❌ All jobs that failed together retry together; thundering herd against a recovering dependency\nNet: one injectable random call versus a synchronized retry wave during every downstream outage.", + "header": "Jitter", + "multiSelect": false, + "options": [ + { + "label": "Full jitter (recommended)", + "description": "Inside the custom backoff function, return random(0, exponentialDelay(attempt)). Inject the random source so tests can seed it. Human: ~1 hr / CC: ~3 min. Risk: low." + }, + { + "label": "No jitter", + "description": "Return exponentialDelay(attempt) exactly. Deterministic. Nothing to build. Risk: synchronized retries against a recovering dependency." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 — Backoff jitter: randomize retry delays?\nProject/branch/task: main — plan \"Add background job retry framework\", custom backoff function approved in D1.\nELI10: When a downstream service goes down, every job that hit it fails at the same moment. Without jitter they all come back at the same moment too, hammering the service exactly when it is trying to recover, and failing together again. Jitter spreads retries randomly across the window so the herd becomes a trickle. The plan says \"full control over the curve\" but never mentions it.\nStakes if we pick wrong: a recovering dependency gets knocked over by our own synchronized retry wave; the outage lasts longer and the fix looks like \"the retries made it worse\".\nRecommendation: A because full jitter is one line inside the backoff function and it is the standard remedy for synchronized retry storms.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Full jitter (recommended) (human: ~1 hr / CC: ~3 min)\n ✅ Five workers retrying the same dead dependency spread their load across the window instead of in lockstep\n ✅ One line in the backoff function; the library still owns scheduling so nothing else changes\n ❌ Retry timing becomes non-deterministic, so tests must seed or inject the random source\nB) No jitter (human: ~0 / CC: ~0)\n ✅ Deterministic delays are trivial to assert in tests and to reason about in logs\n ✅ Nothing to build\n ❌ All jobs that failed together retry together; thundering herd against a recovering dependency\nNet: one injectable random call versus a synchronized retry wave during every downstream outage.": "Full jitter (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-10-01T01:02:45.480Z" + } + ], + "plans": [ + "# Eng Review Report — Add background job retry framework\n\nReview target (fixed): `PLAN.md` in `/home/runner/.cache/gstack-paid-shard-i9xSoz/tmp/gstack-plan-count-QVv99m` (branch `main`, commit 9c0d5e5)\nReviewer: /plan-eng-review (native Claude), session 636-1790816247-91f1d704\nReport file: this file (user-requested destination)\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## Decision ledger\n\n### R1: Retry engine — library retry hooks with a custom backoff function vs a hand-rolled inline scheduler\nFinding: SC1, P1, confidence 8/10, PLAN.md:7-9, reviewer: Claude (native /plan-eng-review)\nPlan baseline: original proposal — \"roll a 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. The repo contains no worker code, no job library dependency and no tests (git ls-files: CLAUDE.md, PLAN.md only). Library identity and its hook API are unverified. The plan's own line \"Same shape as the library version\" (PLAN.md:8) is the only evidence that the library already provides the shape.\nComparison grid:\n\n| Choice | Current | A) Library hooks + custom backoff | B) Custom inline scheduler | C) Investigate hook API first |\n|---|---|---|---|---|\n| R1 retry mechanism | custom inline scheduler in each worker (proposed, unapproved) | library retry hook supplying one custom backoff function (curve fully ours); library owns scheduling, attempt persistence, dead-letter | hand-rolled scheduler inline per worker as planned; we own scheduling, persistence, attempt tracking, dead-letter | bounded: read the library's retry/backoff hook API (CC ≤ 30 min), report whether a custom curve is supported; mechanism stays pending |\n| R2 shared retry envelope helper | duplicated in 5 worker files (proposed) | pending | pending | pending |\n| R3 webhook delivery regression contract | none planned (PLAN.md:17-19) | pending | pending | pending |\n| R4 payload / dependency-graph caching on retry | not planned (PLAN.md:22-24) | pending | pending | pending |\n| R5 jitter, max attempts, dead-letter policy | unspecified | pending | pending | pending |\n\nQuestion D1:\nD1 — Retry engine: library hooks or hand-rolled?\nProject/branch/task: main — plan \"Add background job retry framework\", reviewing PLAN.md Architecture section.\nELI10: Your job library already knows how to retry a failed job later; the plan wants to rebuild that part by hand inside every worker, just so the wait-time curve is ours. Almost every job library lets you plug in your own curve through its retry hook, so you get the curve you want without also owning scheduling, attempt counting, persistence across process restarts, and dead-lettering. The stakes are a second retry system nobody else on the team knows, living in five copies.\nStakes if we pick wrong: a hand-rolled scheduler loses in-flight retries on deploy or crash, drifts from the library's attempt bookkeeping, and every retry bug is ours to find at 3am. Users see jobs that silently never run again.\nRecommendation: A because the plan admits the library already has the shape (PLAN.md:8); supplying a backoff function keeps full control of the curve at a fraction of the surface area. [Layer 1: reuse the installed dependency.]\nCompleteness: A=9/10, B=6/10, C=n/a (investigation, differs in kind)\nPros / cons:\nA) Library hooks + custom backoff (recommended) (human: ~1 day / CC: ~20 min)\n ✅ Full control of the delay curve via one backoff function; the library still persists attempts and survives restarts\n ✅ Dead-letter, max-attempts and retry visibility come for free from the library's existing dashboard and tooling\n ❌ If the library's hook cannot express the curve (verify first), you are back to B for that one worker\nB) Custom inline scheduler (human: ~3-4 days / CC: ~1 hr, plus ongoing maintenance)\n ✅ Zero dependence on the library's retry semantics; curve and scheduling are entirely ours\n ✅ Works even if the library's hook turns out to be too rigid\n ❌ Re-implements persistence, attempt tracking and dead-lettering; in-flight retries die with the process unless you also build durability\nC) Investigate hook API first (CC: ≤ 30 min)\n ✅ Replaces an assumption with a verified fact before committing either way\n ✅ Cheap; the answer feeds directly into A or B\n ❌ Decides nothing today; the mechanism stays pending and blocks the Code Quality question that depends on it\nNet: you are trading a few lines of backoff function (A) against owning a second retry system (B); C buys certainty for half an hour.\nHeader: Retry engine\nOptions:\nA) Library hooks + custom backoff\nUse the existing job library's retry hook and supply one custom backoff function that returns the delay for attempt N. The library keeps scheduling, attempt persistence, max-attempts and dead-letter. Full control of the curve. Human: ~1 day / CC: ~20 min. Risk: low; verify the hook accepts a function before coding.\nB) Custom inline scheduler\nHand-roll the exponential-backoff scheduler inside each worker as PLAN.md:7-9 proposes. We own scheduling, attempt counting, persistence across restarts and dead-lettering. Human: ~3-4 days / CC: ~1 hr plus ongoing maintenance. Risk: medium-high; in-flight retries are lost on crash or deploy unless durability is also built.\nC) Investigate hook API first\nBounded investigation only: read the library's retry/backoff hook API and report whether a custom curve function is supported. CC: ≤ 30 min. Approves no implementation; the retry mechanism stays pending and R2-R5 stay pending.\n\nState: pending\nActual answer: unanswered\nAccepted scope: none\nHistory: none\n", + "# Eng Review Report — Add background job retry framework\n\nReview target (fixed): `PLAN.md` in `/home/runner/.cache/gstack-paid-shard-i9xSoz/tmp/gstack-plan-count-QVv99m` (branch `main`, commit 9c0d5e5)\nReviewer: /plan-eng-review (native Claude), session 636-1790816247-91f1d704\nReport file: this file (user-requested destination)\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## Working plan (current, amended only by approved decisions)\n\n### Architecture (R1 approved, D1 → A)\nUse the existing job library's retry hook. Supply one custom backoff function\n`backoff(attempt) -> delayMs` that owns the curve. The library keeps scheduling,\nattempt persistence, max-attempts and dead-letter. Verify the hook accepts a\nfunction before coding. No hand-rolled inline scheduler.\n\n### Code quality (pending R2)\nAs originally proposed: envelope duplicated across 5 worker files.\n\n### Tests (pending R3)\nAs originally proposed: `processWebhookJob()` rewritten, no regression test planned.\n\n### Performance (pending R4)\nAs originally proposed: full payload re-fetch and graph recompute on every retry.\n\n## Scope Challenge record\nComplexity gate: 5-6 proposed files, 0 new classes → B skipped. Findings: SC1 (P1, 8/10, PLAN.md:7-9) resolved by D1 → A.\nScope Challenge result: scope accepted as-is (mechanism changed, feature set unchanged; not a scope reduction).\n\n## Decision ledger\n\n### R1: Retry engine — library retry hooks with a custom backoff function vs a hand-rolled inline scheduler\nFinding: SC1, P1, confidence 8/10, PLAN.md:7-9, reviewer: Claude (native /plan-eng-review)\nPlan baseline: original proposal — \"roll a 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. The repo contains no worker code, no job library dependency and no tests (git ls-files: CLAUDE.md, PLAN.md only). Library identity and its hook API are unverified. The plan's own line \"Same shape as the library version\" (PLAN.md:8) is the only evidence that the library already provides the shape.\nComparison grid:\n\n| Choice | Current | A) Library hooks + custom backoff | B) Custom inline scheduler | C) Investigate hook API first |\n|---|---|---|---|---|\n| R1 retry mechanism | custom inline scheduler in each worker (proposed, unapproved) | library retry hook supplying one custom backoff function (curve fully ours); library owns scheduling, attempt persistence, dead-letter | hand-rolled scheduler inline per worker as planned; we own scheduling, persistence, attempt tracking, dead-letter | bounded: read the library's retry/backoff hook API (CC ≤ 30 min), report whether a custom curve is supported; mechanism stays pending |\n| R2 shared retry envelope helper | duplicated in 5 worker files (proposed) | pending | pending | pending |\n| R3 webhook delivery regression contract | none planned (PLAN.md:17-19) | pending | pending | pending |\n| R4 payload / dependency-graph caching on retry | not planned (PLAN.md:22-24) | pending | pending | pending |\n| R5 jitter, max attempts, dead-letter policy | unspecified | pending | pending | pending |\n\nQuestion D1:\nD1 — Retry engine: library hooks or hand-rolled?\nProject/branch/task: main — plan \"Add background job retry framework\", reviewing PLAN.md Architecture section.\nELI10: Your job library already knows how to retry a failed job later; the plan wants to rebuild that part by hand inside every worker, just so the wait-time curve is ours. Almost every job library lets you plug in your own curve through its retry hook, so you get the curve you want without also owning scheduling, attempt counting, persistence across process restarts, and dead-lettering. The stakes are a second retry system nobody else on the team knows, living in five copies.\nStakes if we pick wrong: a hand-rolled scheduler loses in-flight retries on deploy or crash, drifts from the library's attempt bookkeeping, and every retry bug is ours to find at 3am. Users see jobs that silently never run again.\nRecommendation: A because the plan admits the library already has the shape (PLAN.md:8); supplying a backoff function keeps full control of the curve at a fraction of the surface area. [Layer 1: reuse the installed dependency.]\nCompleteness: A=9/10, B=6/10, C=n/a (investigation, differs in kind)\nPros / cons:\nA) Library hooks + custom backoff (recommended) (human: ~1 day / CC: ~20 min)\n ✅ Full control of the delay curve via one backoff function; the library still persists attempts and survives restarts\n ✅ Dead-letter, max-attempts and retry visibility come for free from the library's existing dashboard and tooling\n ❌ If the library's hook cannot express the curve (verify first), you are back to B for that one worker\nB) Custom inline scheduler (human: ~3-4 days / CC: ~1 hr, plus ongoing maintenance)\n ✅ Zero dependence on the library's retry semantics; curve and scheduling are entirely ours\n ✅ Works even if the library's hook turns out to be too rigid\n ❌ Re-implements persistence, attempt tracking and dead-lettering; in-flight retries die with the process unless you also build durability\nC) Investigate hook API first (CC: ≤ 30 min)\n ✅ Replaces an assumption with a verified fact before committing either way\n ✅ Cheap; the answer feeds directly into A or B\n ❌ Decides nothing today; the mechanism stays pending and blocks the Code Quality question that depends on it\nNet: you are trading a few lines of backoff function (A) against owning a second retry system (B); C buys certainty for half an hour.\nHeader: Retry engine\nOptions:\nA) Library hooks + custom backoff\nUse the existing job library's retry hook and supply one custom backoff function that returns the delay for attempt N. The library keeps scheduling, attempt persistence, max-attempts and dead-letter. Full control of the curve. Human: ~1 day / CC: ~20 min. Risk: low; verify the hook accepts a function before coding.\nB) Custom inline scheduler\nHand-roll the exponential-backoff scheduler inside each worker as PLAN.md:7-9 proposes. We own scheduling, attempt counting, persistence across restarts and dead-lettering. Human: ~3-4 days / CC: ~1 hr plus ongoing maintenance. Risk: medium-high; in-flight retries are lost on crash or deploy unless durability is also built.\nC) Investigate hook API first\nBounded investigation only: read the library's retry/backoff hook API and report whether a custom curve function is supported. CC: ≤ 30 min. Approves no implementation; the retry mechanism stays pending and R2-R5 stay pending.\n\nState: approved\nActual answer: A) Library hooks + custom backoff (D1 answer, user-selected)\nAccepted scope: Use the existing job library's retry hook and supply one custom backoff function returning the delay for attempt N. The library keeps scheduling, attempt persistence, max-attempts and dead-letter. Verify the hook accepts a function before coding. Drop the hand-rolled inline scheduler from PLAN.md:7-9. Human: ~1 day / CC: ~20 min. R2-R5 remain pending.\nHistory: original proposal (hand-rolled inline scheduler, PLAN.md:7-9) superseded by D1 answer A.\n\n### R5: Webhook delivery semantics under retry — at-least-once with idempotency key and sent-marker, strict at-most-once, or at-least-once without dedupe\nFinding: A1, P1, confidence 8/10, PLAN.md:17-18, reviewer: Claude (native /plan-eng-review), Section 1 Architecture\nPlan baseline: original proposal — `processWebhookJob()` is rewritten under the retry framework; the plan names a \"prior at-most-once delivery guarantee\" (PLAN.md:18) but states no target guarantee. Nothing approved for R5. R1 is approved (library hook + custom backoff).\nRuntime evidence: unknown. `processWebhookJob()` source is not in the repo; whether it already sends an idempotency header or persists a delivered marker is unverified.\nComparison grid:\n\n| Choice | Current | A) At-least-once + idempotency key + sent-marker | B) Strict at-most-once (retry pre-send only) | C) At-least-once, no dedupe |\n|---|---|---|---|---|\n| R1 retry mechanism | approved: library hook + custom backoff (D1 → A) | fixed | fixed | fixed |\n| R5 delivery semantics | unstated; retry framework implies at-least-once by default | retry all failures incl. timeouts; stable idempotency header per event; persist delivered marker on 2xx before job returns; retry path skips send if marker set | retry only failures before the HTTP request leaves (payload fetch, graph build, serialization); post-send failures log + dead-letter, never resend | retry all failures incl. timeouts; no idempotency header; no marker; receivers may get duplicates |\n| R2 shared retry envelope helper | duplicated in 5 worker files (proposed) | pending | pending | pending |\n| R3 webhook delivery regression contract | none planned (PLAN.md:17-19) | pending (asserts chosen R5 semantics once approved) | pending | pending |\n| R4 payload / graph caching on retry | not planned | pending | pending | pending |\n| R6 jitter | unspecified | pending | pending | pending |\n| R7 delay cap | unspecified | pending | pending | pending |\n| R8 max attempts + dead-letter | unspecified | pending | pending | pending |\n\nQuestion D2:\nD2 — Webhook retries: what delivery guarantee do receivers get?\nProject/branch/task: main — plan \"Add background job retry framework\", `processWebhookJob()` rewrite (PLAN.md:17-18).\nELI10: Today a webhook is sent once; if it fails, it is gone. Once you add retries, a request that timed out after the receiver already processed it will be sent again, so receivers can see the same event twice. You have to choose: keep \"never twice\" (and accept \"sometimes never\"), or move to \"always at least once\" and give receivers a stable key so they can ignore the duplicate. The plan is silent, which means the choice gets made by accident in code.\nStakes if we pick wrong: duplicate webhooks double-charge or double-notify downstream customers; or, in the other direction, transient network blips silently drop events customers were promised.\nRecommendation: A because retries exist to survive timeouts, and a stable idempotency key plus a persisted delivered marker is the industry-standard way to make at-least-once safe for receivers.\nCompleteness: A=9/10, B=7/10, C=4/10\nPros / cons:\nA) At-least-once + idempotency key + sent-marker (recommended) (human: ~2 days / CC: ~40 min)\n ✅ Transient failures and timeouts are retried, so customers receive events they were promised\n ✅ Stable idempotency header lets receivers dedupe; delivered marker stops our own re-send after a late 2xx\n ❌ Needs a small schema change (delivered marker) and a documented header contract for receivers\nB) Strict at-most-once (retry pre-send only) (human: ~1 day / CC: ~20 min)\n ✅ Preserves today's guarantee exactly; receivers see no behavior change and need no dedupe work\n ✅ Simplest to test: a retry can never follow a sent request\n ❌ Network timeouts and 5xx from the receiver are never retried; those events are lost and only visible in logs or dead-letter\nC) At-least-once, no dedupe (human: ~0.5 day / CC: ~10 min)\n ✅ Least code: just let the retry hook wrap the whole job\n ✅ Events are never silently lost on transient faults\n ❌ Receivers get duplicates with no key to dedupe on; double side-effects downstream are our fault and invisible to us\nNet: A costs a marker column and a header contract; B keeps today's promise but keeps today's silent drops; C ships fastest and pushes duplicate bugs onto every receiver.\nHeader: Webhook semantics\nOptions:\nA) At-least-once + idempotency key + sent-marker\nRetry every failure including timeouts. Each delivery carries a stable idempotency header derived from the event/job id (not the attempt). Persist a delivered marker on 2xx before the job returns; the retry path checks the marker and skips the send if set. Document the header for receivers. Human: ~2 days / CC: ~40 min. Risk: low-medium (one schema change).\nB) Strict at-most-once (retry pre-send only)\nPreserve today's guarantee. Retry only failures that occur before the HTTP request leaves (payload fetch, graph build, serialization). Any failure after send is logged and dead-lettered, never resent. Human: ~1 day / CC: ~20 min. Risk: low; transient receiver faults drop events as they do today.\nC) At-least-once, no dedupe\nWrap the whole job in the retry hook with no idempotency header and no delivered marker. Receivers may receive duplicates and cannot dedupe them. Human: ~0.5 day / CC: ~10 min. Risk: high for downstream side-effects.\n\nState: approved\nActual answer: A) At-least-once + idempotency key + sent-marker (D2 answer, user-selected)\nAccepted scope: `processWebhookJob()` retries every failure including timeouts. Each delivery carries a stable idempotency header derived from the event/job id, independent of attempt number. A delivered marker is persisted on 2xx before the job returns; the retry path checks the marker and skips the send when set. The header contract is documented for receivers. Includes the schema change for the marker and the tests/docs that establish this behavior. Human: ~2 days / CC: ~40 min. R2, R3, R4, R6, R7, R8 remain pending; R3 will assert these semantics.\nHistory: unstated semantics in PLAN.md:17-18 resolved by D2 answer A.\n\n### R6: Backoff jitter — on or off\nFinding: A2 (part 1 of 3), P2, confidence 7/10, PLAN.md:7-9, reviewer: Claude (native /plan-eng-review), Section 1 Architecture\nPlan baseline: original proposal specifies \"exponential-backoff\" with \"full control over the curve\" (PLAN.md:7-9); jitter unspecified. R1 approved (library hook + custom backoff function). Nothing approved for R6.\nRuntime evidence: unknown; no worker code in repo. Whether the library's default backoff already applies jitter is unverified.\nComparison grid:\n\n| Choice | Current | A) Full jitter | B) No jitter |\n|---|---|---|---|\n| R1 retry mechanism | approved (D1 → A) | fixed | fixed |\n| R5 delivery semantics | approved (D2 → A) | fixed | fixed |\n| R6 jitter | unspecified | delay = random(0, exponentialDelay(attempt)) inside the custom backoff function | delay = exponentialDelay(attempt) exactly, deterministic |\n| R7 delay cap | unspecified | pending | pending |\n| R8 max attempts + dead-letter | unspecified | pending | pending |\n| R2 shared helper / R3 regression / R4 caching | pending | pending | pending |\n\nQuestion D3:\nD3 — Backoff jitter: randomize retry delays?\nProject/branch/task: main — plan \"Add background job retry framework\", custom backoff function approved in D1.\nELI10: When a downstream service goes down, every job that hit it fails at the same moment. Without jitter they all come back at the same moment too, hammering the service exactly when it is trying to recover, and failing together again. Jitter spreads retries randomly across the window so the herd becomes a trickle. The plan says \"full control over the curve\" but never mentions it.\nStakes if we pick wrong: a recovering dependency gets knocked over by our own synchronized retry wave; the outage lasts longer and the fix looks like \"the retries made it worse\".\nRecommendation: A because full jitter is one line inside the backoff function and it is the standard remedy for synchronized retry storms.\nCompleteness: A=9/10, B=6/10\nPros / cons:\nA) Full jitter (recommended) (human: ~1 hr / CC: ~3 min)\n ✅ Five workers retrying the same dead dependency spread their load across the window instead of in lockstep\n ✅ One line in the backoff function; the library still owns scheduling so nothing else changes\n ❌ Retry timing becomes non-deterministic, so tests must seed or inject the random source\nB) No jitter (human: ~0 / CC: ~0)\n ✅ Deterministic delays are trivial to assert in tests and to reason about in logs\n ✅ Nothing to build\n ❌ All jobs that failed together retry together; thundering herd against a recovering dependency\nNet: one injectable random call versus a synchronized retry wave during every downstream outage.\nHeader: Jitter\nOptions:\nA) Full jitter\nInside the custom backoff function, return random(0, exponentialDelay(attempt)). Inject the random source so tests can seed it. Human: ~1 hr / CC: ~3 min. Risk: low.\nB) No jitter\nReturn exponentialDelay(attempt) exactly. Deterministic. Nothing to build. Risk: synchronized retries against a recovering dependency.\n\nState: pending\nActual answer: unanswered\nAccepted scope: none\nHistory: none\n" + ] +} diff --git a/test/helpers/eng-seeded-coverage.ts b/test/helpers/eng-seeded-coverage.ts index eac6f6c02..12924bf7e 100644 --- a/test/helpers/eng-seeded-coverage.ts +++ b/test/helpers/eng-seeded-coverage.ts @@ -163,7 +163,7 @@ 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*[:—–-]\s*/i, '') + const targetName = (s: string) => clean(s).replace(/^Eng(?:ineering)? review(?: report)?\s*[:—–-]\s*/i, '') .replace(/^Plan\s*[:—–-]\s*/i, '').toLowerCase(); 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]!)); @@ -186,7 +186,7 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string): const namedSource = !rawSourceNames.length && named.length <= 1 && titles.length >= 1 && titles[0]!.type === 'heading' && currentHeading(tokens.indexOf(titles[0]!)) && targetFiles.length === 1 && targetFiles[0]!.split('/').at(-1) === 'PLAN.md' && - (named.length === 0 || /^Eng(?:ineering)? review\s*[:—–-]\s*\S/i.test(clean(titles[0]!.text)) && + (named.length === 0 || /^Eng(?:ineering)? review(?: report)?\s*[:—–-]\s*\S/i.test(clean(titles[0]!.text)) && titles.every(title => title.type === 'heading' && targetName(title.text) === named[0])); if (!directSource && !namedSource) return; const withdrawn = (value: string, owners: string) => new RegExp( @@ -324,6 +324,8 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string): const explicitSelectors = nativeOptions.flatMap(option => option.selector ? [option.selector] : []); if (new Set(explicitSelectors).size !== explicitSelectors.length || nativeOptions.some(option => option.selector && !selectors.includes(option.selector) || !option.label || /^[A-D][).:]\s+/.test(option.label))) continue; + // The preamble's `(recommended)` suffix marks the recommendation; it is not part of the choice. + const unmarked = (label: string) => clean(label).replace(/\s*\(recommended\)$/i, ''); const readOptions = (lines: string[]) => { const records: Array<{ selector: string; label: string; description: string[] }> = []; for (const line of lines) { @@ -335,7 +337,7 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string): if (records.length !== nativeOptions.length || new Set(records.map(record => record.selector)).size !== records.length || records.some(record => !selectors.includes(record.selector))) return undefined; const matches = records.map(record => nativeOptions.flatMap((native, at) => - (!native.selector || native.selector === record.selector) && clean(record.label) === native.label && + (!native.selector || native.selector === record.selector) && unmarked(record.label) === unmarked(native.label) && clean(record.description.join('\n')) === clean(native.description) ? [at] : [])); return matches.every(match => match.length === 1) && new Set(matches.flat()).size === records.length ? records : undefined; };