I1: the eng batching counter reads records under "Section N — <review> review" headings

plan-eng-multi-finding-batching failed in 6 of 12 runs with reviewCount 0-1
(or a timeout with every ask counted as Step 0). In run 37158847998 the agent
asked 11 separate questions, each with a saved ledger record, but filed
records R2-R10 under '## Section 1 — Architecture review'. The counter strips
'Section 1' but not the dash separator, so it rejected every record outside
the Decision ledger and the floor never filled. Quoted, historical and
unrelated headings still do not count.
This commit is contained in:
garrytan committed 2026-10-03 23:37:05 +00:00
1 parent ba50f756f8
commit 4e7447e691
3 files changed
+82 -1

No files matched your search

+11
View File
@@ -484,3 +484,14 @@ await import(${JSON.stringify(path.join(ROOT, 'test/skill-e2e-plan-eng-multi-fin
fs.rmSync(temp, { recursive: true, force: true });
}
});
test('a record under a "Section N — <review> review" heading counts (run 37158847998 read 1 of 11 separate asks)', () => {
const run = JSON.parse(fs.readFileSync(path.join(import.meta.dir, 'fixtures/eng-batching-section-heading-37158847998.json'), 'utf8'));
const [d1, d2] = run.calls as NativePlanQuestionCall[];
const counted = (plan: string) => factory(() => plan).isReviewAUQ(nativePlanCallFingerprint(structuredClone(d2!), 0, true), [d1!]);
expect(run.planAtD2).toContain('## Section 1 — Architecture review');
expect(counted(run.planAtD2)).toBe(true);
for (const heading of ['## Section 1 — Appendix', '## Quoted example — Architecture review', '## Section 1 — Architecture review (historical)']) {
expect(counted(run.planAtD2.replace('## Section 1 — Architecture review', heading)), heading).toBe(false);
}
});
@@ -0,0 +1,70 @@
{
"source": "https://github.com/garrytan/gstack/actions/runs/37158847998 (paid-slice-1, plan-eng-multi-finding-batching, reviewCount=1). The native D1/D2 calls are the retained observation.json calls; the saved plan is reconstructed from the PTY render of its Write/Edit diffs (R1 and R2 records verbatim; R1 tail and section prose shortened) because the plan file itself was not retained.",
"calls": [
{
"sessionId": "4ed970bb-01eb-489a-a6c1-698160fe2666",
"toolUseId": "toolu_01WmzT44t4AJMDLMchxzCFs2",
"questions": [
{
"question": "D1 — Use the job library's retry hook or build a custom inline scheduler?\nProject/branch/task: main branch, plan \"Add background job retry framework\" (PLAN.md), adding retries to 5 workers.\nELI10: When a job fails, something has to remember how many times it failed, wait the right amount of time, and run it again, even if the worker process crashes in between. The job library you already use does all of that and lets you plug in your own formula for the wait time. The plan proposes rebuilding that machinery by hand inside each of the 5 workers. The stakes: hand-rolled retry state is where lost jobs and double-runs come from after a deploy or crash.\nStakes if we pick wrong: a worker restart mid-backoff silently drops or double-runs jobs, and five copies of the bug have to be found and fixed separately.\nRecommendation: A because the plan's own words (\"same shape as the library version\") say the curve is the only thing you want to own, and the hook gives you exactly that with none of the persistence risk.\nCompleteness: A=9/10, B=5/10, C=investigate only, no score\nPros / cons:\nA) Library hook + one custom backoff function (recommended)\n ✅ Attempt count, delay scheduling and terminal failure state stay crash-safe in the library's store, not in worker memory\n ✅ Full control of the curve: one project-owned function returns the delay per attempt (human: ~1 day / CC: ~20 min)\n ❌ If this library's hook turns out not to expose attempt number or a custom function, you fall back to B or C after discovering it\nB) Custom inline scheduler in each worker, as planned\n ✅ Zero dependence on library hook semantics; the team reads every line of the retry path\n ✅ Can diverge per worker later without touching shared config\n ❌ Re-implements attempt persistence and rescheduling five times (human: ~1 week / CC: ~1 hour) and owns every crash and double-run edge case forever\nC) Bounded spike first: verify the hook, then decide\n ✅ Removes the one unknown (hook capability for this specific library) before committing\n ✅ Costs under an hour and leaves every other decision untouched\n ❌ Delays the decision; R1 stays pending and no retry code should start until it closes\nNet: You are trading one hour of verification risk (A) against a week of hand-rolled durability code and five copies of its bugs (B).",
"header": "Retry mechanism",
"multiSelect": false,
"options": [
{
"label": "A) Library hook + custom backoff fn (recommended)",
"description": "Register one project-owned backoff function (and one attempt-log listener) with the existing job library's retry hook; each of the 5 workers opts in via config. Attempt state, scheduling and terminal failure live in the library. Full control of the curve. Completeness 9/10. human: ~1 day / CC: ~20 min. Risk: hook capability unverified for this library."
},
{
"label": "B) Custom inline scheduler ×5",
"description": "Keep the plan as written: each worker computes delay, logs the attempt and re-dispatches itself. Owns attempt persistence and crash safety in project code, five times. Completeness 5/10. human: ~1 week / CC: ~1 hour plus ongoing maintenance."
},
{
"label": "C) Spike the hook first",
"description": "Bounded investigation (≤1 hour human / ≤10 min CC): confirm the library's retry hook accepts a custom delay function and exposes the attempt number. R1 stays pending; no retry implementation starts. All other decisions unchanged."
}
]
}
],
"answered": true,
"failed": false,
"answers": {
"D1 — Use the job library's retry hook or build a custom inline scheduler?\nProject/branch/task: main branch, plan \"Add background job retry framework\" (PLAN.md), adding retries to 5 workers.\nELI10: When a job fails, something has to remember how many times it failed, wait the right amount of time, and run it again, even if the worker process crashes in between. The job library you already use does all of that and lets you plug in your own formula for the wait time. The plan proposes rebuilding that machinery by hand inside each of the 5 workers. The stakes: hand-rolled retry state is where lost jobs and double-runs come from after a deploy or crash.\nStakes if we pick wrong: a worker restart mid-backoff silently drops or double-runs jobs, and five copies of the bug have to be found and fixed separately.\nRecommendation: A because the plan's own words (\"same shape as the library version\") say the curve is the only thing you want to own, and the hook gives you exactly that with none of the persistence risk.\nCompleteness: A=9/10, B=5/10, C=investigate only, no score\nPros / cons:\nA) Library hook + one custom backoff function (recommended)\n ✅ Attempt count, delay scheduling and terminal failure state stay crash-safe in the library's store, not in worker memory\n ✅ Full control of the curve: one project-owned function returns the delay per attempt (human: ~1 day / CC: ~20 min)\n ❌ If this library's hook turns out not to expose attempt number or a custom function, you fall back to B or C after discovering it\nB) Custom inline scheduler in each worker, as planned\n ✅ Zero dependence on library hook semantics; the team reads every line of the retry path\n ✅ Can diverge per worker later without touching shared config\n ❌ Re-implements attempt persistence and rescheduling five times (human: ~1 week / CC: ~1 hour) and owns every crash and double-run edge case forever\nC) Bounded spike first: verify the hook, then decide\n ✅ Removes the one unknown (hook capability for this specific library) before committing\n ✅ Costs under an hour and leaves every other decision untouched\n ❌ Delays the decision; R1 stays pending and no retry code should start until it closes\nNet: You are trading one hour of verification risk (A) against a week of hand-rolled durability code and five copies of its bugs (B).": "A) Library hook + custom backoff fn (recommended)"
},
"unansweredQuestionIndices": [],
"answeredAt": "2026-10-03T22:38:43.760Z"
},
{
"sessionId": "4ed970bb-01eb-489a-a6c1-698160fe2666",
"toolUseId": "toolu_01AwVJeKpu15hVeZXdJpZyrE",
"questions": [
{
"question": "D2 — Webhook retries: send a stable idempotency key on every attempt?\nProject/branch/task: main branch, plan \"Add background job retry framework\" (PLAN.md), `processWebhookJob()` now retries under the library hook (D1).\nELI10: Today a webhook is sent once; if it fails, it is gone. With retries, a webhook that actually arrived but whose \"200 OK\" got lost on the way back will be sent again, and the customer gets it twice. The standard fix is to stamp every attempt with the same unique id so the receiver can recognize the repeat and ignore it. Without that stamp, duplicate orders, double emails and double charges on the customer side become your support tickets.\nStakes if we pick wrong: customers process the same event twice with no way to tell it was a repeat, and you cannot fix it after the fact because the attempts carry nothing in common.\nRecommendation: A because it keeps the retry benefit and gives every receiver a dedupe handle for roughly ten minutes of CC work.\nCompleteness: A=9/10, B=4/10, C=6/10\nPros / cons:\nA) Stable idempotency key per job on every attempt (recommended)\n ✅ Receivers can dedupe deterministically; the key is the job id (or existing event id) so no new storage is needed\n ✅ Documented at-least-once contract that matches what Stripe, GitHub and every major webhook sender ship (human: ~half day / CC: ~10 min)\n ❌ Receivers that ignore the header still see duplicates; the contract shifts from \"never twice\" to \"detectably twice\"\nB) Retry without a key, accept duplicates\n ✅ Smallest code change; nothing new on the wire\n ✅ Fine if every receiver is already idempotent by payload content\n ❌ Repeat deliveries are indistinguishable from new events; duplicate side effects at customers are permanent\nC) Exclude the webhook job from retries\n ✅ Preserves the exact current at-most-once behavior; the regression test stays trivial\n ✅ No wire or documentation change for customers\n ❌ Webhook failures stay unrecovered, which is the problem the plan set out to fix\nNet: A trades a header and a line of docs for a dedupe guarantee; B keeps the duplicates; C keeps the drops.",
"header": "Webhook idempotency",
"multiSelect": false,
"options": [
{
"label": "A) Stable idempotency key per attempt (recommended)",
"description": "Every attempt of a webhook job sends the same per-job key (job id or existing event id) as a header and body field; delivery contract documented as at-least-once with dedupe key. Completeness 9/10. human: ~half day / CC: ~10 min. Other decisions unchanged."
},
{
"label": "B) Retry without a key",
"description": "Webhook job retries under the library hook with no idempotency key; at-least-once with undetectable duplicates, documented. Completeness 4/10. human: ~1 hour / CC: ~2 min."
},
{
"label": "C) Exclude webhook job from retries",
"description": "`processWebhookJob()` keeps at-most-once; the other 4 workers retry under D1. Webhook failures stay unrecovered. Completeness 6/10. R10 becomes moot for the webhook job only."
}
]
}
],
"answered": true,
"failed": false,
"answers": {
"D2 — Webhook retries: send a stable idempotency key on every attempt?\nProject/branch/task: main branch, plan \"Add background job retry framework\" (PLAN.md), `processWebhookJob()` now retries under the library hook (D1).\nELI10: Today a webhook is sent once; if it fails, it is gone. With retries, a webhook that actually arrived but whose \"200 OK\" got lost on the way back will be sent again, and the customer gets it twice. The standard fix is to stamp every attempt with the same unique id so the receiver can recognize the repeat and ignore it. Without that stamp, duplicate orders, double emails and double charges on the customer side become your support tickets.\nStakes if we pick wrong: customers process the same event twice with no way to tell it was a repeat, and you cannot fix it after the fact because the attempts carry nothing in common.\nRecommendation: A because it keeps the retry benefit and gives every receiver a dedupe handle for roughly ten minutes of CC work.\nCompleteness: A=9/10, B=4/10, C=6/10\nPros / cons:\nA) Stable idempotency key per job on every attempt (recommended)\n ✅ Receivers can dedupe deterministically; the key is the job id (or existing event id) so no new storage is needed\n ✅ Documented at-least-once contract that matches what Stripe, GitHub and every major webhook sender ship (human: ~half day / CC: ~10 min)\n ❌ Receivers that ignore the header still see duplicates; the contract shifts from \"never twice\" to \"detectably twice\"\nB) Retry without a key, accept duplicates\n ✅ Smallest code change; nothing new on the wire\n ✅ Fine if every receiver is already idempotent by payload content\n ❌ Repeat deliveries are indistinguishable from new events; duplicate side effects at customers are permanent\nC) Exclude the webhook job from retries\n ✅ Preserves the exact current at-most-once behavior; the regression test stays trivial\n ✅ No wire or documentation change for customers\n ❌ Webhook failures stay unrecovered, which is the problem the plan set out to fix\nNet: A trades a header and a line of docs for a dedupe guarantee; B keeps the duplicates; C keeps the drops.": "A) Stable idempotency key per attempt (recommended)"
},
"unansweredQuestionIndices": [],
"answeredAt": "2026-10-03T22:40:17.387Z"
}
],
"planAtD2": "# Plan\n\n## Decision ledger\n\n### R1: Retry mechanism — library hook or custom inline scheduler\nFinding: #1, P1, confidence 8/10, PLAN.md:4-6, plan-eng-review (native)\nPlan baseline: custom exponential-backoff scheduler inline in each of the 5 workers (original proposal, PLAN.md:4-6); no prior approval.\nRuntime evidence: unknown. The job library is named but not identified in the repo; hook capability is documented for BullMQ/Sidekiq/Celery/Oban (WebSearch, 2026-10-03), unverified for this project's library.\nComparison grid:\n\n| Choice | Current | A | B | C |\n|---|---|---|---|---|\n| R1 retry mechanism | custom inline scheduler ×5 (PLAN.md:4-6), pending | library retry hook + one project-owned backoff function | custom inline scheduler ×5 as planned | bounded spike (≤1 hour human / ≤10 min CC): confirm the library hook accepts a custom delay function and exposes attempt number; R1 stays pending |\n| Backoff curve shape | exponential, parameters unspecified (PLAN.md:4) | exponential, parameters pending (R3-R5) | exponential, parameters pending (R3-R5) | unchanged, pending |\n| R2 webhook delivery semantics | pending | pending | pending | pending |\n| R3 jitter / R4 delay cap / R5 exhaustion | pending | pending | pending | pending |\n| R6 shared envelope extraction | pending | pending (mostly moot under A) | pending | pending |\n| R7 regression contract / R8 graph cache | pending | pending | pending | pending |\n\nQuestion D1:\nD1 — Use the job library's retry hook or build a custom inline scheduler?\nProject/branch/task: main branch, plan \"Add background job retry framework\" (PLAN.md), adding retries to 5 workers.\nELI10: When a job fails, something has to remember how many times it failed, wait the right amount of time, and run it again, even if the worker process crashes in between. The job library you already use does all of that and lets you plug in your own formula for the wait time. The plan proposes rebuilding that machinery by hand inside each of the 5 workers. The stakes: hand-rolled retry state is where lost jobs and double-runs come from after a deploy or crash.\nStakes if we pick wrong: a worker restart mid-backoff silently drops or double-runs jobs, and five copies of the bug have to be found and fixed separately.\nRecommendation: A because the plan's own words (\"same shape as the library version\") say the curve is the only thing you want to own, and the hook gives you exactly that with none of the persistence risk.\nCompleteness: A=9/10, B=5/10, C=investigate only, no score\nPros / cons:\nA) Library hook + one custom backoff function (recommended)\n ✅ Attempt count, delay scheduling and terminal failure state stay crash-safe in the library's store, not in worker memory\n ✅ Full control of the curve: one project-owned function returns the delay per attempt (human: ~1 day / CC: ~20 min)\n ❌ If this library's hook turns out not to expose attempt number or a custom function, you fall back to B or C after discovering it\nB) Custom inline scheduler in each worker, as planned\n ✅ Zero dependence on library hook semantics; the team reads every line of the retry path\n ✅ Can diverge per worker later without touching shared config\n ❌ Re-implements attempt persistence and rescheduling five times (human: ~1 week / CC: ~1 hour) and owns every crash and double-run edge case forever\nC) Bounded spike first: verify the hook, then decide\n ✅ Removes the one unknown (hook capability for this specific library) before committing\n ✅ Costs under an hour and leaves every other decision untouched\n ❌ Delays the decision; R1 stays pending and no retry code should start until it closes\nNet: You are trading one hour of verification risk (A) against a week of hand-rolled durability code and five copies of its bugs (B).\nHeader: Retry mechanism\nOptions:\nA) Library hook + custom backoff fn (recommended)\nRegister one project-owned backoff function (and one attempt-log listener) with the existing job library's retry hook; each of the 5 workers opts in via config. Attempt state, scheduling and terminal failure live in the library. Full control of the curve. Completeness 9/10. human: ~1 day / CC: ~20 min. Risk: hook capability unverified for this library.\nB) Custom inline scheduler ×5\nKeep the plan as written: each worker computes delay, logs the attempt and re-dispatches itself. Owns attempt persistence and crash safety in project code, five times. Completeness 5/10. human: ~1 week / CC: ~1 hour plus ongoing maintenance.\nC) Spike the hook first\nState: approved\nActual answer: A) Library hook + custom backoff fn (recommended) (D1, user answer)\nAccepted scope: library hook\nHistory: none\n\n## Section 1 — Architecture review\n\nFindings (grounded in PLAN.md).\n\n### R2: Idempotency key on retried webhook deliveries\nFinding: #2, P1, confidence 8/10, PLAN.md:16-18, plan-eng-review (native)\nPlan baseline: original proposal: rewrite `processWebhookJob()`Line truncated
}
+1 -1
View File
@@ -202,7 +202,7 @@ function recordedBatchingIssue(call: NativePlanQuestionCall, savedPlan: string):
const recordSection = (at: number, depth: number) => {
const owner = tokens.slice(0, at).filter(token => token.type === 'heading' && token.depth < depth).at(-1);
if (!owner || owner.type !== 'heading') return false;
const name = clean(owner.text).replace(/^(?:Section\s+)?[1-9]\d*[.:]?\s*/i, '');
const name = clean(owner.text).replace(/^(?:Section\s+)?[1-9]\d*(?:[.:]|\s+[—–-])?\s*/i, '');
return /^Decision ledger$/i.test(name) || /^(?:Architecture|Code quality|Tests?|Testing|Performance) review(?:\s*[—–:-]\s+[A-Za-z0-9][A-Za-z0-9 ,/()&-]*)?$/i.test(name);
};
const words = (s: string) => (clean(s).toLowerCase().replace(/\(recommended\)/g, '').match(/[a-z][a-z0-9_]*/g) ?? [])