From cee6839553589bc2ff88b515470682b187a3166f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 12:05:17 +0900 Subject: [PATCH 1/2] fix(log-guard): make the locked schema recheck as strict as the inspector Coherence review of the merged stack found the mutation paths weaker than the compatibility report they claim to honor. inspect.ts requires exact table SQL, exact column metadata and every canonical index; protection.ts and maintenance.ts each rechecked column NAMES only. The lock serializes OpenCodex against itself but not against Codex or another SQLite writer, so a migration landing between the outer inspection and the locked write was a real window. Reproduced on dev HEAD by dropping idx_logs_ts in that gap: Protect installed a row-dropping trigger and returned ok:true, and Reclaim vacuumed pages, both on a database the inspector reported as unsupported/monitor-only. hasCurrentLogsSchema is now exported from inspect.ts and both mutation paths delegate to it, so the pre-write gate and the compatibility report can no longer disagree. Regression drives the exact window. --- src/codex/log-guard/inspect.ts | 17 +++++++++++++++++ src/codex/log-guard/maintenance.ts | 10 +++++----- src/codex/log-guard/protection.ts | 10 ++++++---- tests/codex-log-guard-protection.test.ts | 18 ++++++++++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/codex/log-guard/inspect.ts b/src/codex/log-guard/inspect.ts index f7696c6d83..d2c1719236 100644 --- a/src/codex/log-guard/inspect.ts +++ b/src/codex/log-guard/inspect.ts @@ -261,6 +261,23 @@ function sameColumns(columns: ColumnRow[]): boolean { }); } +/** + * The authoritative compatibility predicate: exact table SQL, exact column + * metadata, and every canonical index. + * + * Exported because the mutation paths must apply the SAME test inside their + * write transaction. They used to check column NAMES only, which is strictly + * weaker than what the inspector reports, so a schema change landing between + * the outer inspection and the locked write let Protect install a row-dropping + * trigger and let Reclaim vacuum pages on a database the inspector classifies + * as monitor-only. The lock serializes OpenCodex against itself; it does not + * stop Codex or another SQLite writer, so that TOCTOU window is real. + */ +export function hasCurrentLogsSchema(db: Database): boolean { + const columns = db.query("PRAGMA table_info(logs)").all(); + return hasCurrentLogsTable(db, columns); +} + function hasCurrentLogsTable(db: Database, columns: ColumnRow[]): boolean { const table = db.query( "SELECT name, type, sql FROM sqlite_schema WHERE name = 'logs' LIMIT 1", diff --git a/src/codex/log-guard/maintenance.ts b/src/codex/log-guard/maintenance.ts index d488d41540..81b3a465b5 100644 --- a/src/codex/log-guard/maintenance.ts +++ b/src/codex/log-guard/maintenance.ts @@ -3,7 +3,7 @@ import { Database, constants as sqliteConstants } from "bun:sqlite"; import { getCodexHome, resolveCodexLogsDbPath } from "../paths"; import { samePathIdentity } from "../user-identity"; -import { inspectCodexLogs } from "./inspect"; +import { hasCurrentLogsSchema, inspectCodexLogs } from "./inspect"; import { withCodexLogGuardLock, type CodexLogGuardLockOutcome } from "./lock"; import { sameLogGuardPathIdentity } from "./path-safety"; import { isSqliteBusy } from "./sqlite-errors"; @@ -145,10 +145,10 @@ function databasePathStillMatches( } function exactCurrentSchema(db: Database): boolean { - const columns = db.query("PRAGMA table_info(logs)").all().map(row => row.name).sort(); - const expected = [...CURRENT_LOG_COLUMNS].sort(); - return columns.length === expected.length - && columns.every((value, index) => value === expected[index]); + // Same reasoning as protection.ts: the pre-mutation recheck must match the + // inspector's compatibility contract exactly, or Reclaim can vacuum a + // database the inspector classifies as monitor-only. + return hasCurrentLogsSchema(db); } function pragmaNumber(db: Database, sql: string): number { diff --git a/src/codex/log-guard/protection.ts b/src/codex/log-guard/protection.ts index 5955000180..613d2677ce 100644 --- a/src/codex/log-guard/protection.ts +++ b/src/codex/log-guard/protection.ts @@ -3,7 +3,7 @@ import { pathToFileURL } from "node:url"; import { Database, constants as sqliteConstants } from "bun:sqlite"; import { getCodexHome, resolveCodexLogsDbPath } from "../paths"; -import { inspectCodexLogs, type CodexLogGuardInspection } from "./inspect"; +import { hasCurrentLogsSchema, inspectCodexLogs, type CodexLogGuardInspection } from "./inspect"; import { withCodexLogGuardLock, type CodexLogGuardLockOutcome } from "./lock"; import { sameLogGuardPathIdentity } from "./path-safety"; import { isSqliteBusy } from "./sqlite-errors"; @@ -193,9 +193,11 @@ function observeTriggers(db: Database): CodexLogGuardObservedMode { } function exactCurrentSchema(db: Database): boolean { - const columns = db.query("PRAGMA table_info(logs)").all().map(row => row.name).sort(); - const expected = [...CURRENT_LOG_COLUMNS].sort(); - return columns.length === expected.length && columns.every((value, index) => value === expected[index]); + // Delegates to the inspector's predicate so the locked recheck is exactly as + // strict as the compatibility report. Column names alone let a schema change + // between inspection and the locked write slip a mutation onto a database the + // inspector calls monitor-only. + return hasCurrentLogsSchema(db); } /** diff --git a/tests/codex-log-guard-protection.test.ts b/tests/codex-log-guard-protection.test.ts index 9e49d8dab8..a2e0a7be8f 100644 --- a/tests/codex-log-guard-protection.test.ts +++ b/tests/codex-log-guard-protection.test.ts @@ -327,4 +327,22 @@ describe("Codex Log Guard protection", () => { expect(unprotectCodexLogs(deps).ok).toBe(true); expect(triggers(databasePath)).toEqual([]); }); + test("a schema change between inspection and the locked write is refused", () => { + // TOCTOU: the lock serializes OpenCodex against itself, not against Codex or + // another SQLite writer. The locked recheck used to compare column NAMES + // only - strictly weaker than the inspector's contract - so a migration + // landing in that window let Protect install a row-dropping trigger on a + // database the inspector classifies as monitor-only. + const { codexHome, databasePath } = fixture(); + const deps = testDeps(codexHome); + + // Drop a canonical index the inspector requires but a column-name check + // cannot see. + const db = new Database(databasePath); + db.exec("DROP INDEX idx_logs_ts"); + db.close(); + + expect(protectCodexLogs("quiet", deps)).toEqual({ ok: false, error: "unsupported_schema" }); + expect(triggers(databasePath)).toEqual([]); + }); }); From ff1b2e541814fc30a4119011dd2613deba9bca2b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 12:07:02 +0900 Subject: [PATCH 2/2] fix(release): select a reachable baseline; stop hidden text forging duplicates Two findings from the dev coherence review. 1. Preview releases were unreleasable. selectReleaseBaseline picked the newest tag of either channel without regard to reachability, so a preview selected the newest STABLE tag - which lives on main and is not reachable from preview - and the ancestry guard then threw. Verified against the real repository: preview: without-filter=v2.21.0 (ancestor=false) -> with-filter=v2.19.0 preview-from-dev: without-filter=v2.21.0 (ancestor=false) -> with-filter=v2.17.0 Selection now filters to tags actually reachable from the target. The ancestry guard remains as a fail-closed assertion for any future caller that skips the filter. 2. The duplicate-closure gate stripped only closed HTML comments, while the quality parser treats an unclosed comment as hiding everything through EOF. Two issues whose VISIBLE bodies differ could therefore match on text GitHub never renders, and the workflow closes on that match. Now uses the same through-EOF semantics; verified that hidden text no longer matches while ordinary visible signatures still do. --- .github/scripts/issue-triage-autoclose.cjs | 6 +++- scripts/build-release-changelog.ts | 36 ++++++++++++++++------ tests/build-release-changelog.test.ts | 23 ++++++++++++++ 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/.github/scripts/issue-triage-autoclose.cjs b/.github/scripts/issue-triage-autoclose.cjs index 99ef2d0cec..0d7c2f14e6 100644 --- a/.github/scripts/issue-triage-autoclose.cjs +++ b/.github/scripts/issue-triage-autoclose.cjs @@ -66,7 +66,11 @@ function normalizeSignatureLine(raw) { // case-bearing discriminators; the "exact signature" promise is only true // if the comparison keeps them distinct. return String(raw || "") - .replace(//g, " ") + // An UNCLOSED comment hides everything after it through EOF, exactly as the + // quality parser treats it. Matching only `` let hidden text + // become the "exact shared signature" that closes an issue: two reports + // whose visible bodies differ could match on text GitHub never renders. + .replace(/|$)/g, " ") .replace(/^\s*(?:>|[-*+]\s+|\d+[.)]\s+)/, "") .replace(/^[`~]{3,}[^\n]*$/, "") // Backticks are unambiguous Markdown code delimiters. Preserve `_`, `~`, diff --git a/scripts/build-release-changelog.ts b/scripts/build-release-changelog.ts index ee4e24ef6d..c7868db572 100644 --- a/scripts/build-release-changelog.ts +++ b/scripts/build-release-changelog.ts @@ -118,14 +118,25 @@ export function categoryForPull(pr: AssociatedPullRequest): string { * Prerelease: newest prior release of either channel, so previews stay incremental. * Stable: newest prior stable only, so the final changelog always reconstructs * the complete stable-to-stable range and cannot lose prerelease changes. + * + * `isAncestor` filters to tags actually reachable from the target. Without it a + * preview could select the newest STABLE tag, which lives on `main` and is not + * reachable from `preview` — the ancestry guard then threw and no preview could + * be released at all once the stable lineage advanced. Selecting the newest + * REACHABLE release keeps previews incremental and keeps the range honest. */ -export function selectReleaseBaseline(version: string, tags: string[]): string | null { +export function selectReleaseBaseline( + version: string, + tags: string[], + isAncestor?: (tag: string) => boolean, +): string | null { const releaseTag = version.startsWith("v") ? version : `v${version}`; const targetIsPrerelease = isPrereleaseVersion(version); const candidates = tags .map(tag => tag.trim()) .filter(tag => /^v\d/.test(tag) && compareReleaseTags(tag, releaseTag) < 0) .filter(tag => targetIsPrerelease || !isPrereleaseVersion(tag)) + .filter(tag => isAncestor === undefined || isAncestor(tag)) .sort(compareReleaseTags); return candidates.length > 0 ? candidates[candidates.length - 1]! : null; } @@ -530,16 +541,23 @@ async function main(argv: string[]): Promise { const tags = (await commandText(["git", "tag", "--list", "v[0-9]*"])) .split(/\r?\n/) .filter(Boolean); - const baseline = selectReleaseBaseline(version, tags); + // Resolve reachability once per tag, then let selection skip anything not in + // this target's history. Previously selection ignored ancestry and the guard + // below threw, so a preview became unreleasable the moment the stable lineage + // moved ahead of it. + const ancestryCache = new Map(); + for (const tag of tags) { + const probe = await runCommand(["git", "merge-base", "--is-ancestor", tag, target]); + ancestryCache.set(tag, probe.exitCode === 0); + } + const baseline = selectReleaseBaseline(version, tags, tag => ancestryCache.get(tag) === true); const releaseTag = version.startsWith("v") ? version : `v${version}`; - // Ancestry is required for BOTH channels. It used to be checked only for - // stable releases, which let a preview pick the newest tag from a diverged - // lineage: `git log baseline..target` then emitted the handful of commits - // unique to that unrelated branch and reported "commits=0" coverage, so a - // preview shipped notes that omitted its own history and named someone - // else's. Fail closed instead — a non-ancestral baseline cannot describe a - // range at all, whichever channel asked for it. + // Selection above already filters to reachable tags, so this is now a + // belt-and-braces assertion rather than the primary gate. It stays because a + // non-ancestral baseline cannot describe a range at all, and a future caller + // that skips the filter must still fail closed rather than emit notes drawn + // from an unrelated lineage. if (baseline) { const ancestry = await runCommand(["git", "merge-base", "--is-ancestor", baseline, target]); if (ancestry.exitCode !== 0) { diff --git a/tests/build-release-changelog.test.ts b/tests/build-release-changelog.test.ts index 226afc416f..7baf966d91 100644 --- a/tests/build-release-changelog.test.ts +++ b/tests/build-release-changelog.test.ts @@ -38,6 +38,29 @@ const generatedBugFix = [ ].join("\n"); describe("selectReleaseBaseline", () => { + test("skips a newer release that is not reachable from the target", () => { + // A preview lives on its own lineage. Selecting the newest tag regardless of + // reachability picked a stable tag on main, the ancestry guard then threw, + // and no preview could be released once the stable lineage moved ahead. + // Reproduced against the real repository: without the filter the preview + // range selected v2.21.0, which is an ancestor of neither preview nor dev. + const tags = ["v2.17.0", "v2.19.0", "v2.21.0"]; + const reachable = new Set(["v2.17.0", "v2.19.0"]); + + expect(selectReleaseBaseline("2.22.0-preview.1", tags)).toBe("v2.21.0"); + expect(selectReleaseBaseline("2.22.0-preview.1", tags, tag => reachable.has(tag))).toBe("v2.19.0"); + }); + + test("the reachability filter applies to stable releases too", () => { + const tags = ["v2.17.0", "v2.19.0", "v2.21.0"]; + const reachable = new Set(["v2.17.0"]); + expect(selectReleaseBaseline("2.22.0", tags, tag => reachable.has(tag))).toBe("v2.17.0"); + }); + + test("an absent filter preserves the previous selection behavior", () => { + const tags = ["v2.17.0", "v2.19.0"]; + expect(selectReleaseBaseline("2.20.0", tags)).toBe("v2.19.0"); + }); test("preview releases are incremental from the previous release", () => { expect(selectReleaseBaseline("2.0.0-preview.2", [ "v1.0.0",