From 50c8d6c86d19ead94cd894f4226618fdc92a98c3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 04:51:48 -0700 Subject: [PATCH] fix: close the remaining 5 pre-existing test failures on main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows #9276, which fixed 8 files / 12 cases and left these 4 files / 5 cases still red on main (and therefore red on every open PR). Each diagnosed to its introducing commit; two are real source gaps, two are stale sibling tests a deliberate behavior change never updated. SOURCE fixes: - selfhost/metrics.ts: register two counters emitted since #9243 and #9245 but never added to DEFAULT_METRIC_META, so renderMetrics() emitted them as bare undocumented samples with no HELP/TYPE — exactly what that drift guard exists to catch. Both commits even name the counter in their own body text; a pure registration miss, not a behavior decision. TEST fixes (source verified correct in both cases): - selfhost-pg-retention: the fake Postgres still matched the pre-#9083 ctid semi-join, so every DELETE fell through to rowCount 0. #9083 deliberately moved retention to an index-backed PK range delete with ORDER BY (the fix for prune-retention blowing its timeout and stalling permanently); it updated the SQLite twin but not the Postgres one. Regex now captures whichever key column is in play, keeping BOTH the mapped-PK and ctid-fallback paths exercised. - salvageability: #9085 made an absent blocker confidence degrade to CONFIDENCE_WHEN_UNSTATED (0.5) instead of 1.0, so it is sub-floor against the 0.93 default and the low-confidence hold owns the case. It renamed both sibling assertions in rules.test.ts but missed this third consumption site. Both calls kept — they are the only coverage of the default-floor arm and the nullish-confidence arm respectively. - worker-entry-boundary: false positive, not a real dependency leak. The check grepped whole-file text, so #9230's user-facing string "crossed the visual-diff threshold" failed a green tree over a sentence, in a file worker-reachable since #4120 that imports none of these deps. Narrowed to scan module specifiers (reusing the file's own parseImportSpecifiers) — the only way a Node-only dep can actually reach the bundle. Added a discriminating test so it cannot pass vacuously; verified by injecting a real `import sharp` and confirming it fails, naming the file and specifier. --- src/selfhost/metrics.ts | 2 ++ test/unit/salvageability.test.ts | 16 +++++++++++--- test/unit/selfhost-pg-retention.test.ts | 9 ++++++-- test/unit/worker-entry-boundary.test.ts | 28 +++++++++++++++++++++---- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index 60feae4909..d135305d51 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -168,6 +168,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_active_review_reconciliation_terminalized_total", { help: "Orphaned active_review_tracking rows terminalized after a live GitHub check confirmed the PR is closed, by repo.", type: "counter" }], ["loopover_open_pr_reconciliation_missing_total", { help: "Open PRs found missing from local tracking during reconciliation, by repo.", type: "counter" }], ["loopover_orb_relay_malformed_events_total", { help: "Orb relay batch entries dropped for missing/mistyped required fields (deliveryId/eventName/rawBody).", type: "counter" }], + ["loopover_orb_relay_multiple_live_enrollments_total", { help: "Forwarded orb events where more than one LIVE enrollment existed for the installation (a blue/green swap, or a secret rotated but not yet revoked) -- the winner is still elected deterministically, but the overlap is no longer silent (#9150).", type: "counter" }], ["loopover_orb_relay_register_total", { help: "Orb relay registration attempts, by mode and result (registered/recovered/failed).", type: "counter" }], ["loopover_pr_outcomes_total", { help: "Recorded PR gate outcomes, by decision.", type: "counter" }], ["loopover_close_audit_holdouts_total", { help: "Would-auto-close PRs diverted to human adjudication by the close-audit holdout (#8831).", type: "counter" }], @@ -181,6 +182,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_rees_enrich_request_duration_seconds", { help: "REES /v1/enrich call duration in seconds, for calls that were actually attempted (excludes the auth-rejected circuit-breaker skip).", type: "histogram" }], ["loopover_metrics_sampler_errors_total", { help: "Scrape-time gauge sampler failures, by metric name -- a failing sampler previously emitted no series at all, silently. Any occurrence means that metric's value is currently invisible to Prometheus this scrape (see the sentinel gauges' own -1-on-failure convention).", type: "counter" }], ["loopover_review_source_fresh", { help: "1 when a review/ops/reputation source table has a row inside its own consumer's window, 0 when stale -- labeled by table and window_days. review_targets was silently orphaned by the 2026-06-22 convergence cutover for months before anyone noticed; this makes the next such orphaning loud instead.", type: "gauge" }], + ["loopover_private_manifest_warnings_total", { help: "Private-manifest layer warnings (a malformed shared/global/repo config layer dropped during load), counted one per warning rather than one per load -- a sustained run means a mount is repeatedly serving truncated or invalid config (#9065).", type: "counter" }], ]; const metricMeta = new Map(DEFAULT_METRIC_META); diff --git a/test/unit/salvageability.test.ts b/test/unit/salvageability.test.ts index 2f4c8c843f..d217fb8c3f 100644 --- a/test/unit/salvageability.test.ts +++ b/test/unit/salvageability.test.ts @@ -81,11 +81,21 @@ describe("resolveAiReviewSalvageableHold", () => { expect(hold!.comment).toContain("HELD with guidance"); }); - it("defaults: no configured floor uses the gate default, and a confidence-less blocker counts as certainty (at/above floor)", () => { + // #9085 (landed via #9237): an absent confidence used to degrade to 1.0 -- maximum certainty -- so "the + // model said nothing" silently cleared even the default floor here. It now degrades to + // CONFIDENCE_WHEN_UNSTATED (0.5), which is sub-floor against the 0.93 gate default, so the low-confidence + // hold owns that case and this resolver stands down. #9085 renamed both sibling assertions in + // rules.test.ts but missed this third consumption site, leaving it asserting the pre-change semantics. + // Both calls are load-bearing for coverage: the first is the ONLY case in this file that exercises the + // `aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE` default arm (every other case passes an + // explicit floor), and the second is the only one exercising the `confidence ?? CONFIDENCE_WHEN_UNSTATED` + // nullish arm. + it("defaults: no configured floor uses the gate default (0.93), and a confidence-less blocker is sub-floor", () => { + expect(resolveAiReviewSalvageableHold(aiEval(0.99), { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeDefined(); + const noConfidence = aiEval(0.99); delete (noConfidence.blockers[0] as { confidence?: number }).confidence; - const hold = resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv); - expect(hold).toBeDefined(); + expect(resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeUndefined(); }); it("knob unset (the default) or no score: never changes a disposition", () => { diff --git a/test/unit/selfhost-pg-retention.test.ts b/test/unit/selfhost-pg-retention.test.ts index 96c9742d70..eab1b84fa9 100644 --- a/test/unit/selfhost-pg-retention.test.ts +++ b/test/unit/selfhost-pg-retention.test.ts @@ -32,10 +32,15 @@ function makeRetentionPgPool(remaining: Record = {}): MockPgPool return { rows: [{ n: remaining[table] ?? 0 }], rowCount: 1 }; } - const deleteMatch = /^DELETE FROM (\w+) WHERE ctid IN \(SELECT ctid FROM \1 WHERE .*? LIMIT (\d+)\)$/i.exec(q); + // #9083 (via #9237) changed the emitted shape: retention now deletes by PRIMARY KEY (`id`, `delivery_id`, + // ...) with an explicit `ORDER BY `, falling back to the physical row key only for the + // composite-key tables that have no single-column PK -- an index-backed range delete replacing a + // full-scan-per-batch semi-join. Group 2 captures whichever key column is in play, so BOTH paths (mapped + // PK and the `ctid` fallback) stay exercised by this mock rather than one silently ceasing to match. + const deleteMatch = /^DELETE FROM (\w+) WHERE (\w+) IN \(SELECT \2 FROM \1 WHERE .*? ORDER BY \w+ LIMIT (\d+)\)$/i.exec(q); if (deleteMatch) { const table = deleteMatch[1] as string; - const limit = Number(deleteMatch[2]); + const limit = Number(deleteMatch[3]); const have = remaining[table] ?? 0; const changes = Math.min(have, limit); remaining[table] = have - changes; diff --git a/test/unit/worker-entry-boundary.test.ts b/test/unit/worker-entry-boundary.test.ts index 3f0b8bab59..5f6ec43459 100644 --- a/test/unit/worker-entry-boundary.test.ts +++ b/test/unit/worker-entry-boundary.test.ts @@ -72,14 +72,34 @@ describe("worker entry boundary", () => { expect(forbidden, `worker entry must not reach agent-only modules: ${forbidden.join(", ")}`).toEqual([]); }); - it("does not reference pixelmatch, pngjs, visual-diff, gifenc, or sharp in worker-reachable source", () => { + // Scans MODULE SPECIFIERS, not raw file text. What this guards is the Worker BUNDLE: a Node-only dep can + // only get bundled by being imported (statically or dynamically), so parsing the same specifiers + // collectReachableSources already walks catches every real inclusion path. Grepping whole-file content + // instead produced a false positive the moment ordinary English prose contained one of these words -- + // #9230 added the user-facing string "route(s) crossed the visual-diff threshold" to + // src/review/visual/visual-findings.ts (a file worker-reachable since #4120, importing none of these deps), + // and the raw-content regex failed a green tree over a sentence. Bending correct user-facing copy to dodge + // a test regex would have been the wrong repair; narrowing the check to what it actually means is the right + // one. + it("does not import pixelmatch, pngjs, visual-diff, gifenc, or sharp from worker-reachable source", () => { const hits = collectReachableSources(WORKER_ENTRY) .map((file) => { - const content = readFileSync(file, "utf8"); - return FORBIDDEN_IDENTIFIERS.test(content) ? relativeToRoot(file) : null; + const offending = parseImportSpecifiers(file).filter((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier)); + return offending.length > 0 ? `${relativeToRoot(file)} (${offending.join(", ")})` : null; }) .filter((entry): entry is string => entry !== null); - expect(hits, `worker-reachable files must not mention Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]); + expect(hits, `worker-reachable files must not import Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]); + }); + + // Proves the specifier-scoped check above is still DISCRIMINATING, not vacuously passing: the same regex + // must still flag a real dependency import, and must still ignore the same word in prose. Without this, a + // future edit that broke the matching entirely would look identical to a clean tree. + it("the forbidden-identifier check still flags a real import specifier and still ignores prose", () => { + expect(["sharp", "pixelmatch", "gifenc", "pngjs", "@foo/visual-diff"].every((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier))).toBe(true); + expect(["./capture", "../../types", "node:fs", "hono"].some((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier))).toBe(false); + // The exact #9230 prose that broke the old whole-file scan is not a module specifier, so it is correctly + // invisible to a specifier-scoped check -- while the bare dep name it contains still is not. + expect(parseImportSpecifiers(join(srcRoot, "review/visual/visual-findings.ts")).some((s) => FORBIDDEN_IDENTIFIERS.test(s))).toBe(false); }); it("does not reference visual diff or GIF modules in the published MCP bin bundle", () => {