From d38950d3e8d70e7258dbfa8f6dfd18ac822f960a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:55:24 -0700 Subject: [PATCH] fix(db): map visual_capture_unobtainable_sha, so #9881's degrade can actually fire (#10270) The column was written, declared on PullRequestRecord, and read at two sites -- and the row->record mapper never populated it. So pr.visualCaptureUnobtainableSha was undefined at every reader, captureUnobtainable was permanently false, and the degrade #9881 exists for has never fired once. Its two visualCapture* neighbours are mapped on the adjacent lines; this one was missed. The consequence lands on contributors: a repo with no preview deployment at all cannot produce screenshot evidence by any action, and #9881 exists so the screenshot-table gate holds instead of closing those PRs. They have been getting the undegraded close, which they cannot reopen. Two follow-ups (#10059, #10060) were then built on the same signal and are inert for the same reason. Orb, 2026-07-31: one pull_requests row carries the mark, so the write path demonstrably works, and there are zero github_app.screenshot_table_close_degraded_capture_unobtainable events in 30 days. Written once, read never. Nothing could have caught it. TSC cannot: the field is optional, so "never populated" and "populated with undefined" are the same type. A test asserting the COLUMN passes throughout -- only reading it back through getPullRequest, the way every real caller does, fails. The three regression tests here are written that way and all three fail against the unfixed mapper. Making the field required WOULD catch it, and is not worth it: the engine keeps a structurally parallel PullRequestRecord (packages/loopover-engine/src/types/reward-risk-types.ts) that would have to gain the field too, coupling two types kept independent on purpose. scripts/check-record-mapper-fields.ts closes the hole without that coupling, and closes it for every field rather than this one. The checker's predicate is deliberately narrow, because the obvious rule is wrong. "Every column must be mapped" fires on ten legitimate cases -- JSON columns mapped under a parsed name, the primary key, columns read through dedicated selects or aggregates -- and a checker that cries wolf gets muted. It takes the conjunction of four properties: declared on the record, backed by a column on the mapper's OWN table, read via record.field, and absent from the mapper. The third and fourth alone report PullRequestRecord.changedFiles, which is correctly unmapped (callers that already resolved the diff set it, and its same-named column belongs to recentMergedPullRequests). On this tree the four-way intersection is exactly the one real defect, and it fires again the moment the mapper line is removed. Closes #10270 --- package.json | 3 +- scripts/check-record-mapper-fields.ts | 163 +++++++++++++++++++ src/db/repositories.ts | 3 + src/types.ts | 10 +- test/unit/check-record-mapper-fields.test.ts | 149 +++++++++++++++++ test/unit/db-persistence.test.ts | 44 +++++ 6 files changed, 370 insertions(+), 2 deletions(-) create mode 100644 scripts/check-record-mapper-fields.ts create mode 100644 test/unit/check-record-mapper-fields.test.ts diff --git a/package.json b/package.json index 4050a1938..dcce4b75f 100644 --- a/package.json +++ b/package.json @@ -102,6 +102,7 @@ "command-redelivery-guards:check": "node --experimental-strip-types scripts/check-command-redelivery-guards.ts", "dispatch-gate-reasons:check": "node --experimental-strip-types scripts/check-dispatch-gate-reasons.ts", "dispatch-provenance:check": "tsx scripts/check-dispatch-provenance-stamped.ts", + "record-mapper-fields:check": "node --experimental-strip-types scripts/check-record-mapper-fields.ts", "replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts", "replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write", "replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check", @@ -150,7 +151,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run dispatch-provenance:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run record-mapper-fields:check && npm run dispatch-provenance:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-record-mapper-fields.ts b/scripts/check-record-mapper-fields.ts new file mode 100644 index 000000000..d90f843fd --- /dev/null +++ b/scripts/check-record-mapper-fields.ts @@ -0,0 +1,163 @@ +#!/usr/bin/env node +// #10270: a DB column that is written, declared on the record, and read via `record.field` -- but never +// populated by the row→record mapper. +// +// The instance that prompted this: `visual_capture_unobtainable_sha`. `markPullRequestVisualCaptureUnobtainable` +// wrote it, `PullRequestRecord` declared it, `processors.ts` read it at two sites to decide +// `captureUnobtainable` -- and `mapPullRequestRow` never mapped it. So the read was permanently `undefined`, +// #9881's "this repo has no preview pipeline, degrade the close" path never fired once, and contributor PRs +// kept being closed over evidence they could not produce. One row on the Orb carried the mark; zero degrade +// events in 30 days. +// +// Nothing caught it, and nothing could have: +// +// • TSC cannot: the field is optional (`?:`), so "never populated" and "populated with undefined" are the +// same type. Making it required would catch it, but the engine keeps a structurally-parallel +// `PullRequestRecord` that would then have to gain the field too -- coupling two types that are +// independent on purpose. The type system is the wrong tool here; this is. +// • A test cannot, cheaply: every assertion about the field passes vacuously. A test reading it back gets +// `undefined`, which is indistinguishable from "not set yet" unless the test asserts against the COLUMN. +// +// WHAT THIS CHECKS, and why the predicate is narrow. "Every column must appear in the mapper" is the obvious +// rule and it is wrong: of the 11 unmapped `pull_requests` columns, 10 are legitimate (JSON columns mapped +// under a parsed name, the primary key, columns read through dedicated selects or aggregates). A checker that +// fired on all ten would be muted within a week. The predicate that isolates the real bug is the conjunction: +// +// declared on the record type +// AND backed by a column on the mapper's OWN table +// AND read via `record.field` +// AND absent from the mapper +// +// A field nobody reads off the record is not a bug. A field the mapper populates is not a bug. And a field +// with no column behind it is not a bug either -- `PullRequestRecord.changedFiles` is populated by callers +// that already resolved the diff, and its only same-named column belongs to a DIFFERENT table +// (`recentMergedPullRequests`). Dropping that fourth conjunct makes this checker report it, which is the +// false positive that would get the whole thing muted. Only the intersection of all four is a real defect, +// and on this tree that intersection is exactly the one instance. +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +/** One record type, its mapper, and the sources that read fields off it. */ +type RecordSurface = { + /** Human name, used in the failure message. */ + name: string; + /** File declaring the TS record type. */ + typeFile: string; + /** The `export type = {` / `export interface {` block to read field declarations from. */ + typeName: string; + /** File containing the row→record mapper. */ + mapperFile: string; + /** The `schema.ts` table the mapper reads its row from. A record field with no column here is + * caller-populated, not row-backed, so its absence from the mapper is correct. */ + schemaFile: string; + /** The `export const = sqliteTable(` binding for that table. */ + schemaTable: string; + /** A string unique to the mapper's object literal, used to locate it. */ + mapperAnchor: string; + /** Files whose `.` reads count as "someone reads this off the record". */ + readerFiles: readonly string[]; + /** Bindings that hold a record of this type in the reader files (e.g. `pr.headSha`). */ + readerBindings: readonly string[]; +}; + +const SURFACES: readonly RecordSurface[] = [ + { + name: "PullRequestRecord", + typeFile: "src/types.ts", + typeName: "PullRequestRecord", + mapperFile: "src/db/repositories.ts", + mapperAnchor: "screenshotTablePresenceSatisfied: parseJson<", + schemaFile: "src/db/schema.ts", + schemaTable: "pullRequests", + readerFiles: ["src/queue/processors.ts"], + readerBindings: ["pr", "pullRequest", "storedPr"], + }, +]; + +export type Violation = { surface: string; field: string }; + +/** Field names declared on `typeName`'s object body. */ +export function declaredFields(source: string, typeName: string): string[] { + const start = source.search(new RegExp(`export (?:type ${typeName} = \\{|interface ${typeName} \\{)`)); + if (start === -1) return []; + // The declaration ends at the first line that is a closing brace in column 0 (`};` or `}`), which is how + // every record type in this repo is formatted. + const rest = source.slice(start); + const end = rest.search(/\n\}[;]?\n/); + const body = end === -1 ? rest : rest.slice(0, end); + return [...new Set([...body.matchAll(/^\s{2}(\w+)\??:/gm)].map((match) => match[1]!))]; +} + +/** Field names the mapper's object literal populates. */ +export function mappedFields(source: string, anchor: string): string[] { + const at = source.indexOf(anchor); + if (at === -1) return []; + const open = source.lastIndexOf("return {", at); + if (open === -1) return []; + const close = source.indexOf("\n };", at); + const body = source.slice(open, close === -1 ? source.length : close); + return [...new Set([...body.matchAll(/^\s{4}(\w+):/gm)].map((match) => match[1]!))]; +} + +/** The TS-side column names declared on `tableName`'s Drizzle table body. */ +export function tableColumns(source: string, tableName: string): string[] { + const start = source.indexOf(`export const ${tableName} = `); + if (start === -1) return []; + const rest = source.slice(start); + const end = rest.search(/\n\)/); + const body = end === -1 ? rest : rest.slice(0, end); + return [...new Set([...body.matchAll(/^\s{4}(\w+):\s*\w+\(/gm)].map((match) => match[1]!))]; +} + +/** Fields read as `.` anywhere in `sources`. */ +export function fieldsReadOffRecord(sources: readonly string[], bindings: readonly string[]): Set { + const read = new Set(); + const pattern = new RegExp(`\\b(?:${bindings.join("|")})\\??\\.(\\w+)`, "g"); + for (const source of sources) { + for (const match of source.matchAll(pattern)) read.add(match[1]!); + } + return read; +} + +/** Declared AND read off the record AND not mapped — see the header for why all three conjuncts are needed. */ +export function findViolations( + surface: RecordSurface, + files: { type: string; mapper: string; schema: string; readers: readonly string[] }, +): Violation[] { + const declared = declaredFields(files.type, surface.typeName); + const mapped = new Set(mappedFields(files.mapper, surface.mapperAnchor)); + const columns = new Set(tableColumns(files.schema, surface.schemaTable)); + const read = fieldsReadOffRecord(files.readers, surface.readerBindings); + return declared + .filter((field) => columns.has(field) && read.has(field) && !mapped.has(field)) + .map((field) => ({ surface: surface.name, field })); +} + +function main(): void { + const violations: Violation[] = []; + for (const surface of SURFACES) { + violations.push( + ...findViolations(surface, { + type: readFileSync(surface.typeFile, "utf8"), + mapper: readFileSync(surface.mapperFile, "utf8"), + schema: readFileSync(surface.schemaFile, "utf8"), + readers: surface.readerFiles.map((file) => readFileSync(file, "utf8")), + }), + ); + } + if (violations.length === 0) { + console.log(`check-record-mapper-fields: every declared-and-read record field is populated by its mapper.`); + return; + } + process.stderr.write(`check-record-mapper-fields: ${violations.length} field(s) are declared, read, and never mapped (#10270):\n\n`); + for (const violation of violations) process.stderr.write(` ${violation.surface}.${violation.field}\n`); + process.stderr.write( + "\nEach of these reads `undefined` at every call site, forever, with no compiler error and no failing\n" + + "test -- the field is optional, so 'never populated' and 'populated with undefined' are the same type.\n" + + "That is how #9881's capture-unobtainable degrade sat dead in production while its column was being\n" + + "written. Map the field in the row->record mapper beside its neighbours.\n", + ); + process.exit(1); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index dac76652b..7aff4d2c9 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -7506,6 +7506,9 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull linkedIssueHardRuleViolationIssues: parseJson(row.linkedIssueHardRuleViolationIssuesJson, []), linkedIssueHardRuleViolationReason: row.linkedIssueHardRuleViolationReason, visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha, + // #10270: this one was missing while its two neighbours were mapped, so markPullRequestVisualCaptureUnobtainable + // wrote a column no reader could ever see and #9881's degrade never fired once in production. + visualCaptureUnobtainableSha: row.visualCaptureUnobtainableSha, visualCaptureRetryPendingSha: row.visualCaptureRetryPendingSha, visualCaptureRetryPendingAt: row.visualCaptureRetryPendingAt, manualReviewLabelAppliedSha: row.manualReviewLabelAppliedSha, diff --git a/src/types.ts b/src/types.ts index c4105b08a..0cd75cbc7 100644 --- a/src/types.ts +++ b/src/types.ts @@ -774,7 +774,15 @@ export type PullRequestRecord = { visualCaptureSatisfiedSha?: string | null | undefined; /** #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable here -- the * deployments read succeeded, found none at all, and the poll budget is spent. The screenshotTableGate - * degrades its CLOSE to advisory while this equals the current head. */ + * degrades its CLOSE to advisory while this equals the current head. Publish-written; read straight from + * the row. + * + * #10270: this field was WRITTEN and READ for months while the row->record mapper omitted it, so every + * reader saw `undefined` and the degrade never fired once. Making it required would turn an unmapped field + * into a build error, but `?:` is load-bearing here -- the engine's structurally-parallel + * `PullRequestRecord` (packages/loopover-engine/src/types/reward-risk-types.ts) would have to gain the + * field too, coupling two types kept independent on purpose. `scripts/check-record-mapper-fields.ts` + * closes the same hole without that coupling, and closes it for every field rather than this one. */ visualCaptureUnobtainableSha?: string | null | undefined; /** False-positive close guard (#9030): the head SHA a bounded visual-capture recapture retry is currently * scheduled/in-flight for -- set only when the capture pipeline errored, or the preview is still building, diff --git a/test/unit/check-record-mapper-fields.test.ts b/test/unit/check-record-mapper-fields.test.ts new file mode 100644 index 000000000..029675ff3 --- /dev/null +++ b/test/unit/check-record-mapper-fields.test.ts @@ -0,0 +1,149 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; +import { declaredFields, fieldsReadOffRecord, findViolations, mappedFields, tableColumns } from "../../scripts/check-record-mapper-fields"; + +// #10270: the checker's value is entirely in which cases it fires on. A version that missed the real bug is +// useless; a version that fired on the ten legitimate unmapped columns would be muted inside a week. So each +// case below is written as the tree-shape that produces it, and the false-positive cases matter as much as the +// true one. + +const SURFACE = { + name: "PullRequestRecord", + typeFile: "src/types.ts", + typeName: "PullRequestRecord", + mapperFile: "src/db/repositories.ts", + mapperAnchor: "screenshotTablePresenceSatisfied: parseJson<", + schemaFile: "src/db/schema.ts", + schemaTable: "pullRequests", + readerFiles: ["src/queue/processors.ts"], + readerBindings: ["pr", "pullRequest", "storedPr"], +} as const; + +const TYPE = ` +export type PullRequestRecord = { + headSha?: string | null | undefined; + visualCaptureSatisfiedSha?: string | null | undefined; + visualCaptureUnobtainableSha?: string | null | undefined; + changedFiles?: string[] | undefined; +}; + +export type IssueRecord = { + unrelated: string; +}; +`; + +const SCHEMA = ` +export const pullRequests = sqliteTable( + "pull_requests", + { + headSha: text("head_sha"), + visualCaptureSatisfiedSha: text("visual_capture_satisfied_sha"), + visualCaptureUnobtainableSha: text("visual_capture_unobtainable_sha"), + }, +); + +export const recentMergedPullRequests = sqliteTable( + "recent_merged_pull_requests", + { + changedFilesJson: text("changed_files_json"), + }, +); +`; + +const mapper = (fields: readonly string[]) => ` +function mapPullRequestRow(row: Row) { + return { +${fields.map((field) => ` ${field}: row.${field},`).join("\n")} + screenshotTablePresenceSatisfied: parseJson(row.screenshotTablePresenceSatisfiedJson, null), + }; +} +`; + +const READER = ` + const a = pr.headSha; + const b = pr.visualCaptureSatisfiedSha; + const c = Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === pr.headSha; + const d = pr.changedFiles; +`; + +const run = (fields: readonly string[], type = TYPE, reader = READER) => + findViolations(SURFACE, { type, mapper: mapper(fields), schema: SCHEMA, readers: [reader] }); + +describe("check-record-mapper-fields (#10270)", () => { + it("fires on the real bug: declared, column-backed, read, and unmapped", () => { + expect(run(["headSha", "visualCaptureSatisfiedSha"])).toEqual([{ surface: "PullRequestRecord", field: "visualCaptureUnobtainableSha" }]); + }); + + it("is silent once the field is mapped", () => { + expect(run(["headSha", "visualCaptureSatisfiedSha", "visualCaptureUnobtainableSha"])).toEqual([]); + }); + + it("does NOT fire on a caller-populated field whose only same-named column belongs to another table", () => { + // `changedFiles` is read off the record and never mapped, and that is correct -- callers that already + // resolved the diff set it. Dropping the column-backed conjunct reports it, and this checker would then be + // muted for the one case it exists to catch. + const violations = run(["headSha", "visualCaptureSatisfiedSha", "visualCaptureUnobtainableSha"]); + expect(violations.map((violation) => violation.field)).not.toContain("changedFiles"); + }); + + it("does NOT fire on a column-backed field nobody reads off the record", () => { + // Ten of the eleven unmapped pull_requests columns are this shape. A rule of "every column must be mapped" + // would report all of them. + expect(run(["visualCaptureSatisfiedSha", "visualCaptureUnobtainableSha"], TYPE, "const x = 1;")).toEqual([]); + }); + + it("does NOT fire on a declared field with no column behind it at all", () => { + const typeOnly = TYPE.replace(" changedFiles?: string[] | undefined;", " computedElsewhere?: string | undefined;"); + expect(run(["headSha", "visualCaptureSatisfiedSha", "visualCaptureUnobtainableSha"], typeOnly, "const y = pr.computedElsewhere;")).toEqual([]); + }); + + it("passes on the real tree", () => { + expect( + findViolations(SURFACE, { + type: readFileSync(SURFACE.typeFile, "utf8"), + mapper: readFileSync(SURFACE.mapperFile, "utf8"), + schema: readFileSync(SURFACE.schemaFile, "utf8"), + readers: SURFACE.readerFiles.map((file) => readFileSync(file, "utf8")), + }), + ).toEqual([]); + }); + + describe("helpers", () => { + it("declaredFields reads only the named type's own body", () => { + const fields = declaredFields(TYPE, "PullRequestRecord"); + expect(fields).toContain("visualCaptureUnobtainableSha"); + expect(fields).not.toContain("unrelated"); + expect(declaredFields(TYPE, "NoSuchRecord")).toEqual([]); + }); + + it("declaredFields handles an interface declaration and an unterminated body", () => { + expect(declaredFields("export interface Thing {\n a?: string;\n}\n", "Thing")).toEqual(["a"]); + expect(declaredFields("export type Thing = {\n a?: string;", "Thing")).toEqual(["a"]); + }); + + it("mappedFields reads the mapper's object literal, and returns nothing without an anchor", () => { + expect(mappedFields(mapper(["headSha"]), SURFACE.mapperAnchor)).toEqual(["headSha", "screenshotTablePresenceSatisfied"]); + expect(mappedFields(mapper(["headSha"]), "no-such-anchor")).toEqual([]); + expect(mappedFields("screenshotTablePresenceSatisfied: parseJson<", SURFACE.mapperAnchor)).toEqual([]); + }); + + it("mappedFields runs to end-of-source when the literal is unterminated", () => { + expect(mappedFields("return {\n a: row.a,\n screenshotTablePresenceSatisfied: parseJson<", SURFACE.mapperAnchor)).toEqual(["a", "screenshotTablePresenceSatisfied"]); + }); + + it("tableColumns reads one table's columns, and returns nothing for an unknown table", () => { + expect(tableColumns(SCHEMA, "pullRequests")).toEqual(["headSha", "visualCaptureSatisfiedSha", "visualCaptureUnobtainableSha"]); + expect(tableColumns(SCHEMA, "recentMergedPullRequests")).toEqual(["changedFilesJson"]); + expect(tableColumns(SCHEMA, "noSuchTable")).toEqual([]); + }); + + it("tableColumns runs to end-of-source when the table body is unterminated", () => { + expect(tableColumns('export const t = sqliteTable(\n "t",\n {\n a: text("a"),', "t")).toEqual(["a"]); + }); + + it("fieldsReadOffRecord picks up optional-chained reads and ignores other bindings", () => { + const read = fieldsReadOffRecord(["const a = pr?.headSha; const b = other.notARecordField;"], ["pr"]); + expect([...read]).toEqual(["headSha"]); + }); + }); +}); diff --git a/test/unit/db-persistence.test.ts b/test/unit/db-persistence.test.ts index 49efc6cdb..2ff1d46c6 100644 --- a/test/unit/db-persistence.test.ts +++ b/test/unit/db-persistence.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it } from "vitest"; import { getPullRequest, markPullRequestManualReviewLabelApplied, + markPullRequestVisualCaptureSatisfied, + markPullRequestVisualCaptureUnobtainable, clearPullRequestManualReviewLabelProvenance, getActiveReviewStartedAt, getBounty, @@ -785,3 +787,45 @@ describe("listMergedPullRequestsInWindow (#10168)", () => { expect(await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-30T00:00:00Z", "2026-07-30T23:59:59Z")).toEqual([]); }); }); + +describe("visual-capture unobtainable provenance (#10270)", () => { + const seed = async (env: ReturnType) => { + await upsertRepositoryFromGitHub(env, { full_name: "owner/repo", name: "repo", id: 1, private: false } as never, 4242); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 12, title: "t", state: "open", user: { login: "c" }, head: { sha: "head1" }, labels: [], created_at: "2026-07-05T10:00:00.000Z", + } as never); + }; + + it("REGRESSION: the mark round-trips to the RECORD, not just to the column", async () => { + // The bug: the write worked, the column held the value, and the row->record mapper never populated the + // field -- so `pr.visualCaptureUnobtainableSha` was `undefined` at every reader, `captureUnobtainable` was + // permanently false, and #9881's degrade never fired once in production (0 events in 30 days on the Orb + // while a row carried the mark). Asserting the COLUMN would have passed throughout; only reading it back + // through getPullRequest, the way every real caller does, catches it. + const env = createTestEnv(); + await seed(env); + await markPullRequestVisualCaptureUnobtainable(env, "owner/repo", 12, "head1"); + + const stored = await getPullRequest(env, "owner/repo", 12); + expect(stored?.visualCaptureUnobtainableSha).toBe("head1"); + // The comparison every reader actually performs -- this is what was permanently false. + expect(Boolean(stored?.headSha) && stored?.visualCaptureUnobtainableSha === stored?.headSha).toBe(true); + }); + + it("absent by default, so an unmarked PR never reads as unobtainable", async () => { + const env = createTestEnv(); + await seed(env); + expect((await getPullRequest(env, "owner/repo", 12))?.visualCaptureUnobtainableSha).toBeNull(); + }); + + it("a successful capture CLEARS it -- evidence exists now, so the degrade must stop applying", async () => { + const env = createTestEnv(); + await seed(env); + await markPullRequestVisualCaptureUnobtainable(env, "owner/repo", 12, "head1"); + await markPullRequestVisualCaptureSatisfied(env, "owner/repo", 12, "head1"); + + const stored = await getPullRequest(env, "owner/repo", 12); + expect(stored?.visualCaptureUnobtainableSha).toBeNull(); + expect(stored?.visualCaptureSatisfiedSha).toBe("head1"); + }); +});