From bb8be93c525be399d97081da6eecd094a074b999 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 11:19:12 -0700 Subject: [PATCH] test(engine): audit the auto-heal review-lane call sites (a DELIBERATE-LITERAL note that is not true) (#2802) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What One new source-level audit test, 3 cases. **No production file is touched.** Fourth instance of the optional-role-parameter class (#2795, #2798, #2799) — and the only one so far where the source **annotation asserts the opposite of the fact**. `packages/engine/src/__tests__/auto-heal-review-lane-callsite-audit.test.ts` ## The finding `project-engine.ts`'s `hasAutoHealableVerificationBufferFailure` takes the review-lane answer as an optional parameter defaulting to `task.column === "in-review"`. Its `DELIBERATE-LITERAL` note says: > "Both call sites pass the resolved answer; the default exists so an unconverted caller keeps exactly today's behaviour rather than silently changing meaning." **There are three call sites, not two:** | site | passes the resolved lane? | |---|---| | `canMergeTask:2657` (threads its own param) | ✅ | | ← `canMergeTask:2903` | ✅ `t.column === reviewLane` | | ← `canMergeTask:3334` | ✅ `task.column === mergeLoopReviewLane` | | **merge loop `:3655`** — direct call | ❌ **nothing** | The note counts the two gating callers and misses the healing one. Note which half is converted: **the sites deciding whether a card MAY merge resolve the lane; the site that would RECOVER a stuck card does not.** The consequence is in the same comment: on a renamed board *"a task whose merge verification died on a buffer-overflow error was never auto-healed — it sat retry-exhausted until a human reset it. The failure is invisible because 'no auto-heal' looks identical to 'nothing to heal'."* ## Why this is a source audit and not an E2E The predicate and its caller are both **private methods** of `ProjectEngine`. The three sibling files in this series each carry a live behavioural differential because their predicates are exported; this one cannot, and inventing a mock `ProjectEngine` to assert a private method would prove only that the mock behaves as written. Stated plainly rather than substituted for — the finding is a call-site fact, and a call-site fact is what is asserted. The third case deliberately pins the **false note itself**, so the audit fails when someone corrects the sentence — forcing them to also decide what to do about the third site rather than fixing the prose and leaving the gap. ## A self-correction, forced by the mutation run The first version filtered call sites on whether the argument text contained `isReviewColumn` / `ReviewLane`. Converting the unconverted site to pass a plain `true` left the count at one and **the suite stayed green** — the "alarm in both directions" the header claims did not exist. Now it counts **arguments** (depth-aware, so nested calls and object literals do not confuse it), which is the property actually being asserted and cannot be spelled around. Re-verified: | state | result | |---|---| | main | 3/3 pass | | site 3655 converted to pass a third argument | **fails** | Recorded in an FNXC note next to the helper, because the first version is the exact mistake this series exists to catch. ## Not done, and why **No fix.** Passing the resolved lane at `:3655` means resolving the task's review column inside the merge loop; whether that resolution belongs there or should be hoisted alongside `mergeLoopReviewLane` (already computed nearby, which is what makes the omission look accidental rather than considered) is a decision for the file's owner. ## Verification - new suite — **3/3 passed**, mutation-verified in both directions - `pnpm lint` — clean - Unit lane, no PostgreSQL required; adds no gate surface. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...to-heal-review-lane-callsite-audit.test.ts | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 packages/engine/src/__tests__/auto-heal-review-lane-callsite-audit.test.ts diff --git a/packages/engine/src/__tests__/auto-heal-review-lane-callsite-audit.test.ts b/packages/engine/src/__tests__/auto-heal-review-lane-callsite-audit.test.ts new file mode 100644 index 0000000000..03c024dc79 --- /dev/null +++ b/packages/engine/src/__tests__/auto-heal-review-lane-callsite-audit.test.ts @@ -0,0 +1,105 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-08:45 (call-site audit — a DELIBERATE-LITERAL note that is not true): + +Fourth instance of the optional-role-parameter class (#2795, #2798, #2799), and the only one so far +where the source ANNOTATION asserts the opposite of the fact. + +`project-engine.ts`'s `hasAutoHealableVerificationBufferFailure` takes the review-lane answer as an +optional parameter defaulting to `task.column === "in-review"`, and its own note says: + + "Both call sites pass the resolved answer; the default exists so an unconverted caller keeps + exactly today's behaviour rather than silently changing meaning." + +There are THREE call sites, not two: + + canMergeTask:2657 threads its own `isReviewColumn` through CONVERTED + <- canMergeTask:2903 passes `t.column === reviewLane` CONVERTED + <- canMergeTask:3334 passes `task.column === mergeLoopReviewLane` CONVERTED + merge loop:3655 calls it DIRECTLY with no review-lane answer unconverted + +So the note counts the two gating callers and misses the healing one. On a renamed board the auto-heal +branch inside the merge loop keys on `in-review`, does not match, and — in the words of the same +comment — "a task whose merge verification died on a buffer-overflow error was never auto-healed; it +sat retry-exhausted until a human reset it. The failure is invisible because 'no auto-heal' looks +identical to 'nothing to heal'." + +Note which half is converted: the sites deciding whether a card MAY merge resolve the lane; the site +that would RECOVER a stuck card does not. + +WHY THIS FILE IS A SOURCE AUDIT AND NOT AN E2E. The predicate and its caller are both PRIVATE methods +of `ProjectEngine`, so there is no seam to drive them through without standing up the merge loop. The +three sibling files in this series each carry a behavioural differential because their predicates are +exported; this one cannot, and inventing a mock ProjectEngine to assert a private method would prove +only that the mock behaves as written. Stated plainly rather than substituted for — the finding is a +call-site fact, and a call-site fact is what is asserted. + +It is an alarm in both directions: a new unconverted caller fails it, and converting site 3655 fails +it too, which is the moment to delete this file and record the fix. + +SYNTAX, NOT TEXT. The call sites are found by parsing `project-engine.ts` and inspecting real call +expressions, matching the repo's existing precedent for the same problem +(`core/.../sync-workflow-ir-callsite-allowlist.test.ts`). An audit that reasons about the text +following a name can be broken — or silently misled — by a formatting-only change, which is the +failure mode this whole series is about. The one prose assertion below is unavoidably textual and is +whitespace-normalised for that reason. +*/ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import ts from "typescript"; + +const FILE = join(__dirname, "..", "project-engine.ts"); +const SOURCE = readFileSync(FILE, "utf8"); +const PREDICATE = "hasAutoHealableVerificationBufferFailure"; + +/** Every call of the predicate, as its argument-expression list. A declaration is not a call + * expression, so it is excluded structurally rather than by guessing at its text. */ +function callSites(): ts.NodeArray[] { + const sf = ts.createSourceFile(FILE, SOURCE, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const found: ts.NodeArray[] = []; + const visit = (node: ts.Node) => { + if (ts.isCallExpression(node)) { + const callee = node.expression; + const name = ts.isPropertyAccessExpression(callee) + ? callee.name.text + : ts.isIdentifier(callee) ? callee.text : undefined; + if (name === PREDICATE) found.push(node.arguments); + } + ts.forEachChild(node, visit); + }; + ts.forEachChild(sf, visit); + return found; +} + +describe("auto-heal review-lane call sites", () => { + it("finds every call site, so the audit cannot pass vacuously", () => { + /* A renamed predicate or a moved file would otherwise leave this suite green while measuring + nothing — the failure mode this whole series is about. */ + expect(callSites().length).toBeGreaterThan(0); + }); + + it("has exactly one call site that does NOT pass the resolved review lane", () => { + /* + ARITY is the property asserted, deliberately. A first version of this file filtered on whether the + argument TEXT mentioned `isReviewColumn` / `ReviewLane`; converting the unconverted site to pass a + plain `true` then left the count at one and the suite stayed green, so the "alarm in both + directions" claimed above did not exist. Argument count cannot be spelled around, and parsing + means a reflowed call cannot be miscounted either. + */ + const unconverted = callSites().filter((args) => args.length < 3); + + expect(unconverted).toHaveLength(1); + }); + + it("the DELIBERATE-LITERAL note still claims both call sites are converted", () => { + /* + Pinned deliberately. The note is the artefact that would stop a reviewer looking further, so the + audit fails when the note is corrected — forcing whoever corrects it to also decide what to do + about the third site, rather than fixing the sentence and leaving the gap. + + Whitespace-normalised because the source wraps this sentence mid-phrase; a comment is prose and + has no syntax to parse, so this one assertion is textual by necessity rather than by choice. + */ + expect(SOURCE.replace(/\s+/g, " ")).toContain("Both call sites pass the resolved answer"); + }); +});