Codex review: found issues before merge. Reviewed June 14, 2026, 10:53 PM ET / 02:53 UTC.

Summary
The PR adds an AST-based check:database-first-legacy-stores script, wires it into architecture/preflight/changed checks, narrows the database-first docs, and adds regression tests.

PR surface: Tests +8464, Docs -9, Config +1, Other +9786. Total +18242 across 7 files.

Reproducibility: yes. Source inspection shows current main documents check:database-first-legacy-stores while package.json does not define it, and the PR scanner gap is source-reproducible through resolveDefaultSessionStorePath() returning sessions.json. I did not run local tests because this review is read-only.

Review metrics: 1 noteworthy metric.

• Custom guard scale: 9,783-line script, 8,461-line test. This unusually large custom analyzer is being added to mandatory repository automation, so maintainers should review long-term false-positive and false-negative ownership before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• [P2] Add resolver-call detection and regression coverage for current legacy path APIs.
• [P2] Get maintainer sign-off on the required guard scope and sequencing with the database-first migration.

Risk before merge

• [P1] The required guard still misses writes whose path comes from current resolver APIs, so a new sessions.json write can pass without using a literal legacy filename.
• [P1] The PR makes a large custom flow-sensitive analyzer part of preflight, architecture, and changed-check automation, so false positives or false negatives can block unrelated source work.
• [P1] The protected maintainer label and open canonical database-first migration mean the permanent guard scope and rollout sequencing need explicit owner approval.

Maintainer options:

1. Fix Resolver Coverage Before Merge (recommended)
   Detect writes whose path comes from current legacy-store resolver APIs and add direct plus assigned-call regression cases before the guard becomes required.
2. Accept A Narrow Guard Contract
   Maintainers may merge the current scanner only if they explicitly accept resolver-returned legacy paths as a known follow-up gap.
3. Sequence With The SQLite Migration
   Pause this PR until the canonical database-first migration settles the permanent state boundary and guard ownership.

Next step before merge

• [P2] Manual review is needed because the PR has a protected maintainer label and the remaining blocker is the owner-approved guard contract and migration sequencing, not only a mechanical code edit.

Security
Cleared: No concrete security or supply-chain regression was found; the diff adds local static-analysis tooling, tests, docs, and package-script wiring without new dependencies, lockfile changes, secrets handling, or broader permissions.

Review findings

• [P2] Cover resolver-returned legacy paths — scripts/check-database-first-legacy-stores.mjs:8548-8549

Maintainer review after rebase to current origin/main (d93ece95d5d10867ec877e20e85bf87369209c4f): one blocker remains.

The new guard is wired into check, check:architecture, and changed-check lanes, and the alias/wrapper coverage is much stronger after the latest commit. Local proof on the rebased branch passed:

pnpm check:database-first-legacy-stores
node scripts/run-vitest.mjs test/scripts/changed-lanes.test.ts test/scripts/check.test.ts test/scripts/check-database-first-legacy-stores.test.ts -- --reporter=verbose
git diff --check origin/main...HEAD

Finding:

• [P2] Align the guard with the documented non-write contract before making it required. docs/refactor/database-first.md says check:database-first-legacy-stores also fails reintroduced sessionFile, storePath, SessionManager.open(...), old static SessionManager facades, the old session JSONL downloader hook/class, sidecar-shaped plugin-state/task SQLite helper names, and old memory-index table names. The pushed scanner still only treats transcriptLocator and sqlite-transcript:// as bridge markers in scripts/check-database-first-legacy-stores.mjs, so probes like export const sessionFile = currentSqlitePath, export const storePath = currentSqlitePath, SessionManager.open("sessions.json"), and SessionManager.listAll() return no violations. Because this guard is now in preflight/architecture/changed-check paths, it should either implement the documented contract with explicit current-owner handling, or the docs/PR scope should be narrowed to a filesystem-write-only guard before merge.

Best-fix verdict: acceptable mitigation only if the contract is deliberately narrowed; otherwise too narrow for the documented database-first guard.

Prep update for b6b37afe040aeb38ba8653a1b3187c212e254816:

• Resolved the earlier documented-contract blocker by narrowing docs/refactor/database-first.md to the guard this PR actually implements: legacy store names paired with filesystem writes plus the retired transcriptLocator / sqlite-transcript://... bridge markers. The broader sessionFile / storePath / old SessionManager facade guard is now called out as separate migration-guard follow-up work because those names still have current owners.
• Fixed the CI check-lint failures in scripts/check-database-first-legacy-stores.mjs: unnecessary type conversion, shadowed names, lonely if, and implicit boolean coercion.

Local proof after the prep patch:

pnpm lint:scripts
pnpm check:database-first-legacy-stores
node scripts/run-vitest.mjs test/scripts/changed-lanes.test.ts test/scripts/check.test.ts test/scripts/check-database-first-legacy-stores.test.ts -- --reporter=verbose
pnpm format:docs:check docs/refactor/database-first.md
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

Result: all local proof passed, and autoreview reported autoreview clean: no accepted/actionable findings reported. GitHub CI has restarted on the new SHA and is still in progress.

Land-ready proof for head 34dde2c.

Work done before landing:

• Rebased onto current main ac1042b.
• Fixed the accepted review gaps in scripts/check-database-first-legacy-stores.mjs, including nested wrapper/default/spread coverage and the overly broad extension migration exemption.
• Added focused regressions in test/scripts/check-database-first-legacy-stores.test.ts, including the migration-like extension runtime write case.
• Included one unrelated current-main lint unblocker in extensions/memory-core/src/memory/manager-embedding-ops.ts after CI check-lint failed on an unnecessary non-null assertion.

Local verification:

pnpm check:database-first-legacy-stores
pnpm lint:scripts
node scripts/run-vitest.mjs test/scripts/check-database-first-legacy-stores.test.ts -- --reporter=verbose
node scripts/run-oxlint.mjs extensions/memory-core/src/memory/manager-embedding-ops.ts
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main

Result: all local commands passed. The Vitest guard suite passed 468 tests, and the final branch autoreview reported no accepted/actionable findings.

GitHub verification:

• PR is mergeable against main at head 34dde2c.
• Relevant CI checks are green, including check-lint, check-guards, check-docs, check-test-types, check-prod-types, build-artifacts, check:database-first guard coverage through CI lanes, Windows test, security-fast, dependency guard, Opengrep, Socket, and ci-timings-summary.
• Routine skipped/noise lanes remain skipped as expected: selected Critical Quality shards, Mintlify, macos-swift, node22 compat, and native Telegram proof lanes that were not selected for this tooling PR.

Known proof gap: no live provider/channel run was needed or performed because this PR changes repository tooling/architecture guard behavior, not runtime provider or channel behavior.

Landed.

Source head: 34dde2c
Landed commit: 50c82b3

Linked issue #91628 was closed by the merge.