feat(recency): recipe_recency substrate — last_run_at + run_count#76
Merged
SutuSebastian merged 16 commits intomainfrom May 6, 2026
Merged
feat(recency): recipe_recency substrate — last_run_at + run_count#76SutuSebastian merged 16 commits intomainfrom
SutuSebastian merged 16 commits intomainfrom
Conversation
Plan PR for §1.9 recipe-recency tracking (roadmap.md backlog item). Pre-locked decisions L.1–L.8 lifted from the roadmap entry; open decisions Q1–Q12 grilled inline with Resolution subsections. Key decisions: - Q1: minimal three-column shape (recipe_id TEXT PK, last_run_at INTEGER, run_count INTEGER), STRICT, WITHOUT ROWID + idx_recipe_recency_last_run - Q2: two write sites, one shared recordRecipeRun helper — handleQueryRecipe (MCP+HTTP) + runQueryCmd (CLI). Fact-checked architecturally; CLI does NOT route through tool-handlers.ts despite the original L.2 wording - Q3: lazy prune on --recipes-json reads (NOT eager on writes — keeps the recipe-execution hot path a pure upsert) - Q5: inline last_run_at + run_count fields per entry; (b) wrapping shape rejected after verifying current --recipes-json is a bare JSON array - Q7: survives --full / SCHEMA_VERSION rebuilds (joins query_baselines / coverage user-data precedent — intentionally absent from dropAll()) - Q8: NO SCHEMA_VERSION bump (additive table; CREATE TABLE IF NOT EXISTS auto-creates on next boot per pre-v1 patch-default lessons) - Q12: boundary-check codified — only tool-handlers.ts + cmd-query.ts + the test file may import recordRecipeRun (forbidden-edge SQL inline) Five tracer-bullet slices defined; Slice 5 cleanup runbook deletes this plan file per docs/README.md Rule 3.
…ncy plan)
Schema (src/db.ts):
- New recipe_recency(recipe_id PK, last_run_at, run_count) STRICT, WITHOUT ROWID
- idx_recipe_recency_last_run partial index for the lazy 90-day prune scan
- Sibling to query_baselines + coverage (user-data substrate)
- Intentionally absent from dropAll() (Q7) — survives --full / SCHEMA_VERSION
- No SCHEMA_VERSION bump (Q8) — additive table, IF NOT EXISTS auto-creates
Engine (src/application/recipe-recency.ts, new):
- recordRecipeRun({db, recipeId, now?}) — pure upsert, ON CONFLICT DO UPDATE
- pruneRecipeRecency({db, cutoffMs}) — DELETE WHERE last_run_at < cutoffMs
- loadRecipeRecency({db, now?}) — calls prune internally per Q3 (lazy
read-time pruning), returns Map<recipe_id, {last_run_at, run_count}>
- RECENCY_WINDOW_MS = 90 days exported for tests
- Mirrors application/coverage-engine.ts shape (pure transport-agnostic)
Tests (src/application/recipe-recency.test.ts, new):
- 12 cases covering: schema-empty-after-create, RECENCY_WINDOW_MS constant,
recordRecipeRun (create / increment / distinct-ids / default-now),
pruneRecipeRecency (cutoff-strict-< / no-op-on-empty),
loadRecipeRecency (empty / populated / lazy-prune-on-load)
Slice 1 of the recipe-recency plan; Slice 2 wires recordRecipeRun into
the two write sites (handleQueryRecipe + runQueryCmd).
Hooks recipe-recency tracking into the two transports per Q2 / L.2 of
the recipe-recency plan:
- MCP + HTTP path: handleQueryRecipe in application/tool-handlers.ts
records recency after both runFormattedQuery (success only — `result.ok`)
and executeQuery (success only — !isEnginePayloadError) succeed.
- CLI path: runQueryCmd in cli/cmd-query.ts records via a finally block
that observes process.exitCode as the unified success signal. Every
failure path (emitErrorMaybeJson, printQueryResult non-zero) sets
exitCode=1 before its early return; the finally observes the verdict
regardless of which branch fired. Q9: success only.
Engine (application/recipe-recency.ts):
- New tryRecordRecipeRun(recipeId, opts?) wrapper opens its own DB
(executeQuery runs PRAGMA query_only=1 so can't double as writer),
Q10-isolated try/catch swallows every error with stderr warning unless
quiet. Optional `_openDb` injection seam for the failure-mode test.
Tests (recipe-recency.test.ts, +3 cases, 874 pass total):
- swallows openDb failures and emits stderr warning
- respects quiet flag (no warning)
- production path smoke (writes successfully when openDb works)
Q12 boundary check codified — verified empty:
SELECT DISTINCT file_path FROM imports
WHERE source LIKE '%application/recipe-recency%'
AND specifiers LIKE '%recordRecipeRun%'
AND file_path NOT IN ('src/application/tool-handlers.ts',
'src/cli/cmd-query.ts',
'src/application/recipe-recency.test.ts')
…e 3)
Surfaces recipe recency on every catalog read site per Q5 Resolution
(inline per-entry fields, not a separate top-level recency: map):
- CLI \`bun src/index.ts query --recipes-json\` (cli/cmd-query.ts +
cli/main.ts): each entry gains \`last_run_at: number | null\` and
\`run_count: number\`. The verb still runs BEFORE bootstrapCodemap()
(the catalog has historically been "no DB required"). To preserve
zero-side-effect posture while still enriching when an indexed DB
exists, printRecipesCatalogJson uses a path-based factory:
- resolveRecencyDbPath({root, stateDir}) computes \`<state-dir>/index.db\`
via the same precedence as application/state-dir's resolveStateDir
(cliFlag > env CODEMAP_STATE_DIR > '.codemap').
- existsSync gate skips the open when the file's missing —
enrichWithRecency catches the throw and falls back to null/0
across all entries; never-indexed projects get the catalog clean
with no .codemap dir created.
- MCP/HTTP \`codemap://recipes\` and \`codemap://recipes/{id}\`
(application/resource-handlers.ts): the recipes / one-recipe caches
were dropped — caching the JSON.stringify result alongside recency
would freeze last_run_at at first-read forever per long-running
\`codemap mcp\` / \`codemap serve\` lifetime. The underlying
listQueryRecipeCatalog() / getQueryRecipeCatalogEntry() are themselves
module-cached upstream in query-recipes.ts, so the extra cost is one
DB-read + one JSON.stringify per call. Schema / skill stay cached
(neither changes mid-session). _resetResourceCachesForTests still
exists for the surviving caches.
- Engine (application/recipe-recency.ts): new \`enrichWithRecency<T>\`
generic + \`RecipeRecencyFields\` interface + \`resolveRecencyDbPath\`
helper. Failure-isolated like tryRecordRecipeRun (L.8) — open / read /
close errors swallow to null/0 fallbacks. Test seam renamed to public
\`openDb\` factory (production callers like cmd-query supply a
path-based opener; tests stub a thrower).
Tests (resource-handlers.test.ts, +4 cases — 878 pass total):
- includes last_run_at + run_count on every entry
- populates real recency for recipes seeded in the DB
- live-reads every call (mutation between reads visible)
- codemap://recipes/{id} returns enriched entry
Verified empirically:
- Indexed project: \`--recipes-json\` shows real run counts (fan-out=3 after
3 runs); never-run recipes show null/0.
- Fresh /tmp project: \`--recipes-json\` emits the catalog with null/0
fallbacks and creates ZERO files (no .codemap dir, no index.db).
- MCP/HTTP \`codemap://recipes\` reflects mutations immediately on the
next read (cache-removal verified end-to-end).
Adds the `.codemap/config` `recipe_recency: boolean` field per L.4 of the recipe-recency plan. Default ON (opt-out, not opt-in). Schema (src/config.ts): - New optional Zod field `recipe_recency` on codemapUserConfigSchema. - ResolvedCodemapConfig gains a required `recipeRecency: boolean`. - resolveCodemapConfig defaults to true; only `recipe_recency: false` flips it off (`parsed?.recipe_recency !== false`). Runtime (src/runtime.ts): - New getRecipeRecencyEnabled() helper, parallel to getFts5Enabled(). Engine (src/application/recipe-recency.ts): - tryRecordRecipeRun short-circuits BEFORE openDb when the toggle is off — no DB connection, no upsert, no rows ever land. Cleanest opt-out shape per Q11 (not "ignore the data after writing it"). - The toggle-read itself is wrapped in try/catch because getRecipeRecencyEnabled() throws when the runtime singleton isn't initialised (e.g. CLI smoke paths before bootstrapCodemap). On throw, fall through to the openDb path; the outer try/catch swallows any follow-on failure. L.8 contract holds either way. Tests (recipe-recency.test.ts, +2 cases — 17 local, 880 project-wide): - short-circuits the upsert when recipe_recency: false (verified the injected openDb thrower never fired + table stayed empty) - writes normally when recipe_recency: true (default) Verified empirically against /tmp fixture project: - recipe_recency: false → recipe runs cleanly, table stays at 0 rows - recipe_recency: true (or omitted) → recipe runs cleanly, table populates with run_count=1
…ice 5)
Final slice of the recipe-recency plan. Lifts durable design into the
permanent docs homes per docs/README.md Rule 2 + docs-governance "delete
+ lift, no tombstones" discipline; deletes the plan file per Rule 3.
docs/architecture.md § Schema:
- New \`recipe_recency\` table description sibling to query_baselines /
coverage. Documents the user-data lifecycle (intentionally absent
from dropAll), the two write sites (handleQueryRecipe + runQueryCmd
via shared recordRecipeRun helper), the 90-day lazy-prune contract,
the opt-out config, and the boundary discipline.
docs/glossary.md:
- New "recipe_recency (table) / recipe recency / recipe_recency: false"
entry between "recipe shadows" and "research".
docs/roadmap.md § Backlog:
- Recipe-recency entry removed (item shipped).
Bundled agent surface (templates/agents/) + dev-side mirror (.agents/),
in lockstep per docs/README.md Rule 10:
- Both rule files: --recipes-json row mentions the new last_run_at +
run_count fields, the jq sort idiom, and the opt-out config.
- Both skill files: --recipes-json description updated; codemap://recipes
+ codemap://recipes/{id} resource descriptions add the recency fields
and note the cache-removal ("live-read every call").
Source comments slimmed:
- Plan-file references (now-dead pointers in src/config.ts, src/db.ts,
src/application/recipe-recency.ts) repointed at the durable
architecture.md home.
- One backtick-in-SQL gotcha hit + fixed per .agents/lessons.md.
Plan file:
- docs/plans/recipe-recency.md DELETED. Recipe-recency lives at:
- docs/architecture.md § recipe_recency (schema + lifecycle)
- docs/glossary.md (term entry)
- templates/agents/{rules,skills}/codemap (consumer agent surface)
- .agents/{rules,skills}/codemap (dev-side mirror)
Verified: rg "plans/recipe-recency|recipe-recency.md" returns 0 hits;
880 tests + 26 goldens green.
Per .agents/rules/concise-comments + agents-tier-system durability rule:
new comments authored during the recipe-recency feature were too long
and cited dead plan-internal markers (Q-N / Slice-N / L.X) — the plan
file was deleted in the previous commit, so those references no longer
resolve to anything.
Source comments slimmed:
- src/application/recipe-recency.ts — JSDoc on every public symbol
trimmed to 1-3 lines; rationale now stands on its own without "Q5
Resolution" / "Slice 3" / "L.2" citations.
- src/application/tool-handlers.ts — recency call-site comment cut
from 3 lines to 1.
- src/application/resource-handlers.ts — cache-removal comment cut
from 9 lines to 5; readRecipesCatalog / readOneRecipe inner
comments dropped (information was already on the function above).
- src/cli/cmd-query.ts — printRecipesCatalogJson docstring cut from
15 lines to 5; recency-record finally-block comment cut from 7 to 4.
- Tests: describe / inline labels stop citing dead plan markers
("Slice 3 recency inline" → "recency inline"; "L.8 / Q10" → just
"failure isolation").
Docs staleness fixes:
- docs/architecture.md — "per Q4 of the recipe-recency plan" rewritten
to standalone rationale ("only one version is ever reachable per id").
- docs/glossary.md — "(`Q9` of the recipe-recency plan)" parenthetical
dropped; the surrounding sentence already conveys the constraint.
- docs/research/non-goals-reassessment-2026-05.md — "§ 1 Shipped since"
table gains the recipe-recency row; "§ 5 cadence" narrative + "§ 7
Lifted to" table both flip 1.9 from pending → shipped, pointing at
architecture.md / glossary.md as the canonical homes (per docs-
governance "research notes get closed: lift adopted decisions").
880 tests + 26 goldens still green.
🦋 Changeset detectedLatest commit: 5c4485e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR implements per-recipe run recency tracking via a new ChangesRecipe Recency Tracking
Sequence DiagramsequenceDiagram
participant User as User / CLI
participant CLI as cmd-query.ts<br/>(runQueryCmd)
participant Handlers as tool-handlers.ts<br/>(handleQueryRecipe)
participant DB as SQLite<br/>recipe_recency
participant Resources as resource-handlers.ts<br/>(readRecipesCatalog)
User->>CLI: --recipe X --full --ci
CLI->>CLI: recipeQuerySucceeded = false
CLI->>Handlers: runQueryCmd(recipe=X)
Handlers->>Handlers: run query, format output
alt success
Handlers->>Handlers: result.ok = true
Handlers->>CLI: return result
CLI->>CLI: recipeQuerySucceeded = true
else failure
Handlers->>CLI: return result (ok: false)
CLI->>CLI: recipeQuerySucceeded = false
end
CLI->>CLI: finally: if(recipeQuerySucceeded)
CLI->>DB: tryRecordRecipeRun(recipe=X)<br/>upsert run_count, last_run_at<br/>prune rows > 90-day window
DB-->>CLI: success (non-blocking)
CLI-->>User: exit with appropriate code
User->>CLI: --recipes-json
CLI->>CLI: printRecipesCatalogJson({root, stateDir})
CLI->>DB: openCodemapDatabase (if DB exists)
CLI->>DB: loadRecipeRecency()
DB-->>CLI: Map<recipe_id, {last_run_at, run_count}>
CLI->>Resources: enrichWithRecency(catalog, recency_map)
Resources-->>CLI: catalog with last_run_at/run_count fields
CLI-->>User: JSON with recency inline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`runSql()` on Node splits multi-statement SQL on `;` (better-sqlite3
allows one statement per prepare()); the `recipe_recency` table comment
contained `(matches bundled or project recipe ids; no FK to a recipes
table because there isn't one)` — the inner `;` was treated as a
statement boundary, producing a comment-only fragment that prepare()
rejects with `RangeError: The supplied SQL string contains no statements`.
Bun tests didn't catch this — `bun:sqlite` accepts multi-statement SQL
natively, so `bun test` + `bun run check` were both green; the failure
surfaces only when CI runs `node dist/index.mjs --full` against
fixtures/minimal.
Fix: rewrite the comment to use an em-dash instead of a semicolon. The
underlying gotcha is documented in architecture.md § Runtime and database
("Do not put `;` inside `--` line comments in db.ts DDL strings"), but
the lesson wasn't in `.agents/lessons.md` — appended now alongside the
backticks-in-template-literals lesson (same family of subtle template-
substring failures, different trigger).
Verified locally:
rm -f fixtures/minimal/.codemap.db && \
CODEMAP_ROOT=$PWD/fixtures/minimal node dist/index.mjs --full
returns the expected row counts. Validator script (one-liner that splits
the createTables template on `;` and prints any all-`--`-comment fragment)
flagged the bad fragment cleanly — adding it as a `bun run smoke:node`
step is a candidate followup so the lesson becomes self-enforcing.
`bun audit` flagged GHSA-v2v4-37r5-5v8g (moderate XSS in `ip-address <= 10.1.0` Address6 HTML-emitting methods) via the transitive chain `@modelcontextprotocol/sdk → express-rate-limit → ip-address`. The job is named "non-blocking" but exits 1, surfacing as red noise on every PR. Codemap's MCP server uses STDIO transport, never HTTP, so `express-rate-limit` is dead code in our usage and the XSS surface (`Address6.toString()` HTML output) is unreachable. Real risk is ~zero; fix is one line. `@modelcontextprotocol/sdk` is already at the latest version (1.29.0) and still ships with the bad transitive — pinning at the consumer side is the only path until upstream updates. Bun's `overrides` field is the npm-compatible escape hatch. Verified: `bun audit` → "No vulnerabilities found"; 880 tests still pass.
Three audits (Codex 5.3 / Composer / gpt-5.5) reviewed PR #76 in parallel. Triangulated and applied per .agents/rules/pr-comment-fact-check. Finding 1 — `process.exitCode` for success oracle (HIGH; 2/3 agents) - `runQueryCmd`'s finally keyed `tryRecordRecipeRun` off `process.exitCode !== 1`, but `--ci` deliberately sets exitCode=1 as the CI gating signal on findings. Result: every successful `--ci` recipe run was undercounted. Repro'd: `query --recipe fan-out --format sarif --ci` exited 1 (gate fired correctly), `run_count` stayed unchanged. - Refactored `printFormattedQuery` return type from `number` to a discriminated union: `{ ok: true, exitCode: 0 | 1 } | { ok: false }`. Callers can now disambiguate "succeeded with CI gate" from "real error". - `runQueryCmd` now uses an explicit `recipeQuerySucceeded` local flag set true at each successful exit point; the finally checks the flag, not exitCode. Per-branch (saveBaseline / baseline / groupBy / formatted / text) success snapshots account for cross-call exitCode poisoning when the helper is invoked programmatically. Finding 5 — Read-side lazy DELETE in catalog reads (MEDIUM; gpt-5.5) - `loadRecipeRecency` ran `DELETE FROM recipe_recency WHERE last_run_at < cutoff` before SELECT, so `--recipes-json` and the MCP `codemap://recipes` / `codemap://recipes/{id}` resources mutated the DB on every read. CLI help still says `--recipes-json` is "No DB"; the contract was broken. Recipe-recency plan Q3 had locked lazy-on-read with reasoning that didn't model the read-purity invariant — agent caught the gap. - `loadRecipeRecency` now filters at SELECT time (`WHERE last_run_at >= cutoff`) — pure read. - `recordRecipeRun` does eager prune (cheap on a tiny table — single indexed DELETE per write) before its upsert, so the table stays bounded. - `pruneRecipeRecency` retained as a maintenance helper for tests / future ad-hoc CLI verbs; production reads never call it. Finding 7 — CLI write-site test gap (MEDIUM; gpt-5.5) - Added `src/cli/cmd-query-recency.test.ts` — 6 subprocess-based integration tests exercising `runQueryCmd` end-to-end: - records on plain `--recipe + --json` - records on `--ci + --format sarif` WITH findings (regression test for Finding 1; exit=1 + run_count incremented) - records on `--ci + --format sarif` with NO findings (exit=0 + incremented) - does NOT record on real failures - does NOT record on ad-hoc SQL (no recipe id) - does NOT record when `recipe_recency: false` config Engine test updates: - `recipe-recency.test.ts` — old "lazy prune on load" test renamed to "filters out without mutating the table (read purity)" and asserts the ancient row STILL physically exists after load. New test "recordRecipeRun — eager prune" asserts write-side cleanup. Test count: 880 → 887 (+7 net new). Full project check green. Findings 2, 3, 4, 6 (LOW — doc inaccuracy / clarification) addressed in the next commit (separate concerns: docs vs code). Lessons updated in `.agents/lessons.md` (process.exitCode + read-purity patterns now durable for future contributors).
Findings 2/3/4/6 from the triangulated audit (LOW-severity doc fixes) + a concise-comments sweep on the comments authored in the prior audit-fix commit. Finding 2 (Composer) — architecture.md claimed forbidden-edge SQL "in the engine module's docstring" that didn't exist. Resolution: lift the re-runnable kit into a new `architecture.md § Boundary verification — recipe_recency write path` subsection (durable home — recipe-recency.ts docstring just links there per concise-comments rule). Finding 3 (Composer) — prose said write sites "call recordRecipeRun" but production callers bind `tryRecordRecipeRun` (the failure-isolated wrapper around the inner `recordRecipeRun` upsert). Updated: - `architecture.md` § recipe_recency - `glossary.md` recipe_recency entry Finding 4 (Composer) — read-path importer (`resource-handlers.ts`, binding `enrichWithRecency`) wasn't echoed in the boundary text. Now explicit: write-path is restricted, read-path is unrestricted by design. Finding 6 (gpt-5.5) — `db.ts` JSDoc on `SCHEMA_VERSION` said "Bump on any DDL change", but actual policy (per `.agents/lessons.md` "changesets bump policy" + the `query_baselines` / `coverage` / `recipe_recency` precedent) is patch-for-additive. Slimmed the JSDoc to one-liner + expanded the rationale in `architecture.md § Schema Versioning` so the declaration carries the marker, the docs carry the policy. Concise-comments sweep on the prior commit's authored comments (per `.agents/rules/concise-comments.md`): - Engine docstrings in `recipe-recency.ts` for `recordRecipeRun` / `loadRecipeRecency` / `pruneRecipeRecency` — slimmed from 6-9 lines each to 2-3 lines (the rule's hard budget for code comments). The long-form rationale already lives in architecture.md. - `cmd-query.ts` — `FormattedQueryResult` JSDoc (5→3 lines, dropped the "PR #76 audit" cite since audits files are mortal under docs-lifecycle-sweep), `recipeQuerySucceeded` flag comment (6→3), formatted-branch inline comment (3→1). - `cmd-query-recency.test.ts` — header docstring (8→4), per-test inline rationale slimmed; no test references "Finding N" or "PR #76 audit" any more. - `recipe-recency.test.ts` — minor inline trims. Also adds `.gitkeep` to `docs/audits/` (pre-existing) and `docs/research/` (matches docs-governance § 4 .gitkeep discipline — "every potentially-empty docs subdirectory carries a .gitkeep"). 887 tests + project check still green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/application/recipe-recency.ts (2)
64-95: 💤 Low valueOpt-out check is bypassed when runtime is uninitialised.
If
getRecipeRecencyEnabled()throws because the runtime singleton isn't set, the catch swallows it and execution falls through toopenDb(). That's labelled "let the openDb path try" — butopenDb()typically also needs the runtime (forgetDatabasePath()), so in practice it throws, gets caught at line 81, and emits a stderr warning even when the user has explicitly disabled the feature. Worth flipping the order so the disabled-by-config case stays warning-free in pre-bootstrap CLI smoke paths:Possible tightening
try { if (!getRecipeRecencyEnabled()) return; } catch { - // Runtime not initialised — let the openDb path try. + // Runtime not initialised — both opt-out check and openDb would fail. + // Bail rather than emit a confusing "[recency] write failed" warning. + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/recipe-recency.ts` around lines 64 - 95, In tryRecordRecipeRun, the current try/catch around getRecipeRecencyEnabled() swallows runtime-init errors and falls through to openDb(), causing spurious warnings; change the catch so that if getRecipeRecencyEnabled() throws (runtime uninitialised) we simply return early (no openDb()/recordRecipeRun/console.warn) instead of continuing; update the try/catch that calls getRecipeRecencyEnabled() in tryRecordRecipeRun accordingly so only when the toggle getter succeeds and returns true do we proceed to call openDb(), recordRecipeRun({ db, recipeId }) and finally closeDb(db).
38-52: 💤 Low valueWrap the prune+upsert in a single transaction.
recordRecipeRunissues two separate statements; in better-sqlite3 each runs in its own implicit transaction (two fsyncs per recipe run). Wrapping them in adb.transaction(...)(assign the returned function and invoke it) makes the pair atomic and cuts disk I/O ~in half on the hot write path. It also closes a tiny window where a crash between DELETE and INSERT could leave the table pruned but the just-run recipe unrecorded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/recipe-recency.ts` around lines 38 - 52, The two separate db.run calls in recordRecipeRun (the DELETE using RECENCY_WINDOW_MS and the INSERT/ON CONFLICT upsert) must be executed inside a single sqlite transaction: create a transaction function with db.transaction(...) that contains both the DELETE and the INSERT statements (assign the returned function to a variable and immediately invoke it) so the prune+upsert become atomic and reduce fsyncs; ensure you still pass the same parameters (recipeId, now) into the statements and keep using the RECENCY_WINDOW_MS constant.src/application/resource-handlers.ts (1)
114-129: 💤 Low valueMinor: per-request DB open on every catalog read.
Both
readRecipesCatalogandreadOneRecipenow open a fresh DB connection on every call (via the defaultopenDbinsideenrichWithRecency). That's the explicit design — live reads under--watch— but forcodemap://recipes/{id}you're loading the whole 90-day window fromrecipe_recencyto enrich one entry. For the v1 catalog size this is negligible; flagging only so the assumption is recorded and revisited ifloadRecipeRecencyever grows anidsfilter.No change requested for this PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/resource-handlers.ts` around lines 114 - 129, readOneRecipe currently causes enrichWithRecency to open the DB and load the full 90-day recipe_recency window for a single entry; to avoid per-request full scans, add an optional ids filter to loadRecipeRecency and thread it through enrichWithRecency so callers can request recency only for specific ids. Update enrichWithRecency to accept an optional ids: string[] parameter and, in readOneRecipe, call enrichWithRecency([entry], [id]) (while leaving readRecipesCatalog unchanged), or alternatively reuse a shared DB/connection when performing many lookups; reference functions: readOneRecipe, readRecipesCatalog, enrichWithRecency, loadRecipeRecency, listQueryRecipeCatalog.src/cli/cmd-query-recency.test.ts (2)
27-27: 💤 Low valueModule-load
!runs beforebeforeAll's guard.
Bun.which("bun")returnsstring | null. The non-null assertion executes at file-evaluation time, beforebeforeAllcan throw the friendly diagnostic on line 65-69. Ifbunisn't onPATHin CI, the guard message never fires — you get a TypeError onBun.spawn([null, ...])instead.Either move the lookup into
beforeAllor fail soft:♻️ Fail-soft variant
-const bunBin = Bun.which("bun")!; +const bunBin = Bun.which("bun");Then dereference inside
runCli/beforeAllafter the existence check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/cmd-query-recency.test.ts` at line 27, The file-level non-null assertion on bunBin (const bunBin = Bun.which("bun")!) runs at module load time and can throw before the beforeAll guard; move the Bun.which("bun") lookup into the test setup (beforeAll) or make bunBin nullable and check its existence before use (e.g., inside runCli or beforeAll) so the friendly diagnostic in beforeAll can run; update references to bunBin in runCli/tests to dereference only after the existence check.
135-156: 💤 Low valueConditional assertion weakens the test.
Branching the assertion on
r.exitCodemeans the test passes regardless of whether the recipe rejectskind=invalid_kind_valueor accepts it. If param-value semantics change, this won't catch the regression — it'll just silently switch branches. Consider replacing with a real failure trigger that's stable across param-validation evolutions (e.g., a known-malformed SQL injection point or a recipe id that doesn't exist), so the "no record on failure" contract is asserted unconditionally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/cmd-query-recency.test.ts` around lines 135 - 156, Test currently branches on r.exitCode making it vacuously pass; change it to force a deterministic recipe-side failure and assert no recency was recorded unconditionally. Replace the unstable param-based failure in the "does NOT record recency on SQL parse errors (real failure)" test (which calls runCli with recipe "find-symbol-by-kind" and params) with a guaranteed-failure invocation (for example call runCli with a non-existent recipe id like "recipe-does-not-exist" or a known-malformed SQL payload) so the command fails reliably, keep using loadRunCount("find-symbol-by-kind") to read the before count, remove the if/else branch on r.exitCode, and assert that await loadRunCount("find-symbol-by-kind") is equal to before (no increment). Ensure the test name and use of runCli and loadRunCount remain unchanged so the intent is clear.src/cli/cmd-query.ts (2)
849-895: ⚖️ Poor tradeoffTwo success-detection mechanisms now coexist — consider unifying.
The
before = process.exitCode/ post-check pattern (lines 850, 866, 882) contradicts the comment on lines 818-820 in spirit, even if it's correct: thebefore === 1clause is specifically there to avoid the cross-call poisoning the comment warns about. Meanwhile,printFormattedQueryalready adopted the cleaner discriminated-union return value.The fragility is subtle but real:
- It assumes
runSaveBaseline/runBaselineDiff/runGroupedQueryonly ever setprocess.exitCodeto1on failure (any future helper that sets2would silently flip the success branch).- It relies on the
formatIncompatibilityparser-level guard never permitting--ci+--save-baselineetc., since those paths assume exitCode=1 means failure.A follow-up refactor making these helpers return
{ ok: boolean }(matchingFormattedQueryResult) would letrecipeQuerySucceededbe set unambiguously without snapshottingprocess.exitCode. Not blocking — works today.♻️ Direction (illustrative, not exhaustive)
if (opts.saveBaseline !== undefined) { - const before = process.exitCode; - runSaveBaseline({ ... }); - if (process.exitCode !== 1 || before === 1) recipeQuerySucceeded = true; + const ok = runSaveBaseline({ ... }); + recipeQuerySucceeded = ok; + if (!ok) process.exitCode = 1; return; }
runSaveBaseline/runBaselineDiff/runGroupedQuerywould each returnboolean(or the sameFormattedQueryResultshape), centralizing the failure signal in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/cmd-query.ts` around lines 849 - 895, The current success check mixes snapshotting process.exitCode with implicit failure semantics, which is fragile; change runSaveBaseline, runBaselineDiff, and runGroupedQuery to return a discriminated result (e.g., boolean ok or the same FormattedQueryResult shape used by printFormattedQuery) and use that return value to set recipeQuerySucceeded instead of comparing process.exitCode (remove the before/process.exitCode checks and the before === 1 clause). Update callers in cmd-query.ts to read the returned { ok } (or boolean) and set recipeQuerySucceeded = true when ok, and adjust any downstream code that relies on process.exitCode to continue using explicit return values; keep formatIncompatibility/CLI guarding as-is but prefer the explicit return for future-proofing.
612-643: 💤 Low valueSide-effect-free catalog read achieved cleanly — small nit on factory throw.
Skipping
bootstrapCodemap()and using a path-based opener that bails out before any.codemapdirectory creation is exactly right for the historic "no DB required" contract of--recipes-json. TheexistsSync(dbPath)short-circuit prevents accidental DB creation underopenCodemapDatabase.Minor stylistic nit: the factory throws (
throw new Error(...)) for the not-indexed case purely soenrichWithRecencycan catch and fall back to null/0. That's control-flow-via-exception. A cleaner contract would let the factory returnnullorundefined, withenrichWithRecencybranching on it explicitly. Trade-off: would require changingenrichWithRecency's shape, so probably not worth it for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/cmd-query.ts` around lines 612 - 643, The dbFactory in printRecipesCatalogJson currently throws an Error when the recency DB is missing; change it to return null (or undefined) instead of throwing (use resolveRecencyDbPath + existsSync and return null when missing, otherwise return openCodemapDatabase(dbPath)), and update enrichWithRecency to accept an optional openDb factory (or handle a null/undefined return from the factory) by branching: if openDb is absent or returns null, skip opening the DB and fall back to null/0 recency values rather than relying on exception control-flow; reference functions/values: printRecipesCatalogJson, dbFactory, resolveRecencyDbPath, existsSync, openCodemapDatabase, and enrichWithRecency.docs/architecture.md (1)
590-599: 💤 Low valueBoundary check pattern works but the substring match is implicit.
specifiers LIKE '%recordRecipeRun%'matches bothrecordRecipeRunandtryRecordRecipeRunbecause the latter contains the former as a substring. This is presumably intentional (both are write-path symbols), but it's load-bearing on the naming pattern — if a future symbol ever introduces e.g.recordRecipeRunner, it would also match and trigger a false-positive escalation.Consider being explicit about the intent in the snippet, either via a comment in the SQL or a more precise pattern:
♻️ More explicit pattern
```bash bun src/index.ts query --json " SELECT DISTINCT file_path FROM imports WHERE source LIKE '%application/recipe-recency%' - AND specifiers LIKE '%recordRecipeRun%' + -- catches both `recordRecipeRun` and `tryRecordRecipeRun` (substring) + AND (specifiers LIKE '%\"recordRecipeRun\"%' + OR specifiers LIKE '%\"tryRecordRecipeRun\"%') AND file_path NOT IN ('src/application/tool-handlers.ts', 'src/cli/cmd-query.ts', 'src/application/recipe-recency.test.ts') " ```The quoted-name match also avoids matching prose mentions in
imports.specifiersif the JSON shape ever shifts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 590 - 599, The current SQL uses a substring match (specifiers LIKE '%recordRecipeRun%') which can false-positive on names like recordRecipeRunner; change the WHERE clause to explicitly match quoted exported names (e.g., specifiers LIKE '%"recordRecipeRun"%' OR specifiers LIKE '%"tryRecordRecipeRun"%') or enumerate the exact symbols you want, and add an inline SQL comment explaining that you're intentionally matching both "recordRecipeRun" and "tryRecordRecipeRun" to avoid accidental matches from similarly named symbols; target the WHERE specifiers condition in the shown query over the imports table and keep the file_path NOT IN exclusion intact.src/config.ts (1)
93-98: ⚡ Quick winNaming inconsistency in user-facing config schema —
recipe_recencyshould berecipeRecencyfor API consistency.Every other user-facing key in
codemapUserConfigSchemais camelCase (databasePath,excludeDirNames,tsconfigPath) or a single lowercase word (fts5,boundaries).recipe_recencyis the lone snake_case key, which is inconsistent when users authordefineConfig({ ... })in TypeScript.Since the resolved-config side already exposes
recipeRecency(line 188), renaming the user-facing key to match preserves API consistency and lets users grep a single name across their config files.♻️ Proposed rename
- recipe_recency: z + recipeRecency: z .boolean() .optional() .describe( "Track per-recipe `last_run_at` + `run_count` in the `recipe_recency` table; surfaces inline on `--recipes-json` for agent-host ranking. Default `true` (opt-out). Set `false` to short-circuit every write — no rows ever land. Local-only — no upload primitive. See `docs/architecture.md` § `recipe_recency`.", ),And at line 300:
- const recipeRecency = parsed?.recipe_recency !== false; + const recipeRecency = parsed?.recipeRecency !== false;Test fixtures in
cmd-query-recency.test.ts(line 178) andrecipe-recency.test.ts(lines 323, 349) would also need updating fromrecipe_recency:torecipeRecency:.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 93 - 98, Change the user-facing key in the codemapUserConfigSchema from snake_case recipe_recency to camelCase recipeRecency to match the rest of the API: update the schema entry (the z.boolean().optional().describe(...) block currently labeled recipe_recency) to use recipeRecency, adjust any schema parsing/merge code that references recipe_recency to use recipeRecency (the resolved-config already exposes recipeRecency so align the input name), and update test fixtures that set recipe_recency (e.g., in cmd-query-recency.test.ts and recipe-recency.test.ts) to use recipeRecency so tests reflect the new key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/recipe-recency.md:
- Line 7: Update the changeset paragraph to reflect that the CLI uses an
explicit success flag rather than observing process.exitCode: mention that both
write sites call the shared recordRecipeRun helper (from
application/recipe-recency.ts), that handleQueryRecipe
(application/tool-handlers.ts) covers MCP+HTTP, and runQueryCmd
(cli/cmd-query.ts) uses an explicit success flag to decide whether to record
recency; keep the notes about counting only successful runs, swallowing
recency-write failures with a stderr warning, and lazy 90-day window enforcement
on --recipes-json reads.
In `@src/db.ts`:
- Around line 197-205: The comment incorrectly says the 90-day window is
enforced on read; update the db.ts block to state that pruning is performed as
part of the write/upsert path during recipe-run recording (recordRecipeRun) and
that pruneRecipeRecency is only a maintenance helper for tests/CLI (reads do not
invoke it), while loadRecipeRecency only queries with WHERE last_run_at >= ? and
does not delete; preserve the note that this table is intentionally omitted from
dropAll() so full rebuilds keep user activity history.
In `@templates/agents/rules/codemap.md`:
- Line 32: Update the stale caching wording in
templates/agents/rules/codemap.md: change the MCP section that claims the recipe
catalog resources lazy-cache on first read_resource so it states that
codemap://recipes exposes live recency fields (last_run_at and run_count) and is
not frozen after first read; ensure the doc references the current behavior used
by `codemap query --recipes-json` (which includes `last_run_at: number | null`
and `run_count: number`) and note that local opt-out remains via
`.codemap/config` `recipe_recency: false`, using consistent terminology "live
recency" instead of "lazy-cache" or "frozen".
---
Nitpick comments:
In `@docs/architecture.md`:
- Around line 590-599: The current SQL uses a substring match (specifiers LIKE
'%recordRecipeRun%') which can false-positive on names like recordRecipeRunner;
change the WHERE clause to explicitly match quoted exported names (e.g.,
specifiers LIKE '%"recordRecipeRun"%' OR specifiers LIKE
'%"tryRecordRecipeRun"%') or enumerate the exact symbols you want, and add an
inline SQL comment explaining that you're intentionally matching both
"recordRecipeRun" and "tryRecordRecipeRun" to avoid accidental matches from
similarly named symbols; target the WHERE specifiers condition in the shown
query over the imports table and keep the file_path NOT IN exclusion intact.
In `@src/application/recipe-recency.ts`:
- Around line 64-95: In tryRecordRecipeRun, the current try/catch around
getRecipeRecencyEnabled() swallows runtime-init errors and falls through to
openDb(), causing spurious warnings; change the catch so that if
getRecipeRecencyEnabled() throws (runtime uninitialised) we simply return early
(no openDb()/recordRecipeRun/console.warn) instead of continuing; update the
try/catch that calls getRecipeRecencyEnabled() in tryRecordRecipeRun accordingly
so only when the toggle getter succeeds and returns true do we proceed to call
openDb(), recordRecipeRun({ db, recipeId }) and finally closeDb(db).
- Around line 38-52: The two separate db.run calls in recordRecipeRun (the
DELETE using RECENCY_WINDOW_MS and the INSERT/ON CONFLICT upsert) must be
executed inside a single sqlite transaction: create a transaction function with
db.transaction(...) that contains both the DELETE and the INSERT statements
(assign the returned function to a variable and immediately invoke it) so the
prune+upsert become atomic and reduce fsyncs; ensure you still pass the same
parameters (recipeId, now) into the statements and keep using the
RECENCY_WINDOW_MS constant.
In `@src/application/resource-handlers.ts`:
- Around line 114-129: readOneRecipe currently causes enrichWithRecency to open
the DB and load the full 90-day recipe_recency window for a single entry; to
avoid per-request full scans, add an optional ids filter to loadRecipeRecency
and thread it through enrichWithRecency so callers can request recency only for
specific ids. Update enrichWithRecency to accept an optional ids: string[]
parameter and, in readOneRecipe, call enrichWithRecency([entry], [id]) (while
leaving readRecipesCatalog unchanged), or alternatively reuse a shared
DB/connection when performing many lookups; reference functions: readOneRecipe,
readRecipesCatalog, enrichWithRecency, loadRecipeRecency,
listQueryRecipeCatalog.
In `@src/cli/cmd-query-recency.test.ts`:
- Line 27: The file-level non-null assertion on bunBin (const bunBin =
Bun.which("bun")!) runs at module load time and can throw before the beforeAll
guard; move the Bun.which("bun") lookup into the test setup (beforeAll) or make
bunBin nullable and check its existence before use (e.g., inside runCli or
beforeAll) so the friendly diagnostic in beforeAll can run; update references to
bunBin in runCli/tests to dereference only after the existence check.
- Around line 135-156: Test currently branches on r.exitCode making it vacuously
pass; change it to force a deterministic recipe-side failure and assert no
recency was recorded unconditionally. Replace the unstable param-based failure
in the "does NOT record recency on SQL parse errors (real failure)" test (which
calls runCli with recipe "find-symbol-by-kind" and params) with a
guaranteed-failure invocation (for example call runCli with a non-existent
recipe id like "recipe-does-not-exist" or a known-malformed SQL payload) so the
command fails reliably, keep using loadRunCount("find-symbol-by-kind") to read
the before count, remove the if/else branch on r.exitCode, and assert that await
loadRunCount("find-symbol-by-kind") is equal to before (no increment). Ensure
the test name and use of runCli and loadRunCount remain unchanged so the intent
is clear.
In `@src/cli/cmd-query.ts`:
- Around line 849-895: The current success check mixes snapshotting
process.exitCode with implicit failure semantics, which is fragile; change
runSaveBaseline, runBaselineDiff, and runGroupedQuery to return a discriminated
result (e.g., boolean ok or the same FormattedQueryResult shape used by
printFormattedQuery) and use that return value to set recipeQuerySucceeded
instead of comparing process.exitCode (remove the before/process.exitCode checks
and the before === 1 clause). Update callers in cmd-query.ts to read the
returned { ok } (or boolean) and set recipeQuerySucceeded = true when ok, and
adjust any downstream code that relies on process.exitCode to continue using
explicit return values; keep formatIncompatibility/CLI guarding as-is but prefer
the explicit return for future-proofing.
- Around line 612-643: The dbFactory in printRecipesCatalogJson currently throws
an Error when the recency DB is missing; change it to return null (or undefined)
instead of throwing (use resolveRecencyDbPath + existsSync and return null when
missing, otherwise return openCodemapDatabase(dbPath)), and update
enrichWithRecency to accept an optional openDb factory (or handle a
null/undefined return from the factory) by branching: if openDb is absent or
returns null, skip opening the DB and fall back to null/0 recency values rather
than relying on exception control-flow; reference functions/values:
printRecipesCatalogJson, dbFactory, resolveRecencyDbPath, existsSync,
openCodemapDatabase, and enrichWithRecency.
In `@src/config.ts`:
- Around line 93-98: Change the user-facing key in the codemapUserConfigSchema
from snake_case recipe_recency to camelCase recipeRecency to match the rest of
the API: update the schema entry (the z.boolean().optional().describe(...) block
currently labeled recipe_recency) to use recipeRecency, adjust any schema
parsing/merge code that references recipe_recency to use recipeRecency (the
resolved-config already exposes recipeRecency so align the input name), and
update test fixtures that set recipe_recency (e.g., in cmd-query-recency.test.ts
and recipe-recency.test.ts) to use recipeRecency so tests reflect the new key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a38e0866-7ce3-432c-82b9-3d7d2144875e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.agents/lessons.md.agents/rules/codemap.md.agents/skills/codemap/SKILL.md.changeset/recipe-recency.mddocs/architecture.mddocs/audits/.gitkeepdocs/glossary.mddocs/research/.gitkeepdocs/research/non-goals-reassessment-2026-05.mddocs/roadmap.mdpackage.jsonsrc/application/recipe-recency.test.tssrc/application/recipe-recency.tssrc/application/resource-handlers.test.tssrc/application/resource-handlers.tssrc/application/tool-handlers.tssrc/cli/cmd-query-recency.test.tssrc/cli/cmd-query.tssrc/cli/main.tssrc/config.tssrc/db.tssrc/runtime.tstemplates/agents/rules/codemap.mdtemplates/agents/skills/codemap/SKILL.md
💤 Files with no reviewable changes (1)
- docs/roadmap.md
SutuSebastian
changed the title
3 unresolved review threads from CodeRabbit, all flagging stale doc
references introduced by my audit-fix refactor (lazy-on-read →
eager-on-write) where I missed updating these specific spots. All three
verdict ✅ CORRECT per .agents/rules/pr-comment-fact-check (verified
each against current code).
Finding A — `.changeset/recipe-recency.md:7`
- Said: CLI finally-block "observes process.exitCode as the unified
success signal" + "90-day rolling window enforced lazily on
--recipes-json reads".
- Now: explicit `recipeQuerySucceeded` flag (NOT process.exitCode);
90-day window enforced eagerly on the write path; reads are pure.
Finding B — `src/db.ts` recipe_recency DDL block (lines ~197-205)
- Said: "90-day rolling window is enforced lazily by pruneRecipeRecency
on read, not on write — keeps the recipe-execution hot path a pure
upsert."
- Now: "90-day window is eager-on-write (recordRecipeRun DELETEs stale
rows before its upsert); reads stay pure." Also slimmed the comment
block to ~7 lines (matches `coverage` / `query_baselines` precedent
length; concise-comments rule).
Finding C — `templates/agents/rules/codemap.md` + `.agents/rules/codemap.md`
- Said: "v1 ships ... four lazy-cached resources" + "Catalog resources
lazy-cache on first read_resource".
- Now: `codemap://recipes` + `codemap://recipes/{id}` are live read
every call so inline `last_run_at` / `run_count` recency stays fresh.
Only `schema` + `skill` lazy-cache. Both rule files updated in
lockstep (CLI-prefix-only delta per Rule 10).
887 tests + project check still green.
Triangulated 9 nitpicks per .agents/rules/pr-comment-fact-check; 6 applied, 3 deferred with reply. Applied: #1 — `recipe-recency.ts` opt-out swallowing Previously: when the runtime singleton wasn't initialised, getRecipeRecencyEnabled() threw, was caught, and execution fell through to openDb() — which itself depends on the same runtime, also threw, and emitted a confusing "[recency] write failed" warning even when the user had opted out via config. Fix: bail in the catch branch instead of falling through. #2 — Wrap prune+upsert in a single transaction Two separate db.run calls = two implicit transactions / two fsyncs per recipe write. Wrapped in db.transaction(fn) — atomic; closes the (tiny) window where a crash between DELETE and INSERT could leave the table pruned but the just-run recipe unrecorded; halves the write-side disk I/O on the hot path. #4 — `cmd-query-recency.test.ts:27` Bun.which non-null assertion `Bun.which("bun")!` ran at module load, racing the beforeAll guard. Moved into beforeAll; runCli now bails with a clear diagnostic if bunBin wasn't set. #5 — `cmd-query-recency.test.ts` vacuous-branching test The "does NOT record on failure" test branched on r.exitCode and asserted different things in each branch — vacuously passed regardless of whether the recipe accepted or rejected the param. Replaced with a deterministic-failure trigger (unknown recipe id — the catalog-lookup error is fundamental and won't drift with param-validation rule evolution). #8 — `architecture.md` boundary-check SQL substring match `LIKE '%recordRecipeRun%'` matched both `recordRecipeRun` and `tryRecordRecipeRun` (correct today by accident — substring), but would also match a hypothetical future `recordRecipeRunner`. Tightened to quoted-name match (`%"recordRecipeRun"%` OR `%"tryRecordRecipeRun"%`) since `imports.specifiers` is JSON. #9 — `config.ts` `recipe_recency` → `recipeRecency` (camelCase) Top-level user-config keys are camelCase (`databasePath` / `excludeDirNames` / `tsconfigPath` / `fts5` / `boundaries`); `recipe_recency` was the lone snake_case top-level key. Renamed for API consistency. Field is brand-new in this PR (never shipped) so no migration concern. Cascading updates: - `src/config.ts` schema + resolver - `src/application/recipe-recency.test.ts` fixtures (2 sites) - `src/cli/cmd-query-recency.test.ts` fixture + describe label - `.changeset/recipe-recency.md` - `docs/architecture.md` + `docs/glossary.md` + `docs/research/non-goals-*` - `.agents/rules/codemap.md` + `templates/agents/rules/codemap.md` The SQL TABLE name `recipe_recency` stays snake_case (SQL convention). Deferred (reply only — see PR threads): #3 `resource-handlers.ts` per-request DB open (CodeRabbit: "no change requested for this PR") #6 `cmd-query.ts` two coexisting success-detection mechanisms (CodeRabbit: "Not blocking — works today"; refactor for follow-up) #7 `cmd-query.ts` factory-throws-as-control-flow (CodeRabbit: "probably not worth it for this PR") 887 tests still pass; project-wide check green.
CI failed again with the same `RangeError: The supplied SQL string contains no statements` from yesterday's lesson — I introduced a `;` inside the freshly-rewritten recipe_recency DDL block comment when applying CodeRabbit nitpick #2 (the lazy-on-read prose fix). Two changes: 1. Replaced "(recordRecipeRun DELETEs stale rows before its upsert); reads stay pure." with em-dash to drop the inner `;`. (Same fix shape as 649f7d2 from the previous round.) 2. **Added a regression guard** in `src/db.test.ts` that splits the `createTables()` template on `;` and asserts no fragment is pure `--` comments. Hitting this lesson twice in one PR is the trigger the existing entry in `.agents/lessons.md` flagged as "candidate roadmap item." Test runs in `bun test` (which Bun's `bun:sqlite` masks), so it catches the regression locally before CI burns a cycle. 887 → 888 tests; CI's `node dist/index.mjs --full` smoke verified locally before push.
Two comment blocks authored in earlier audit-fix commits were over the 3-line budget: - src/db.test.ts — regression-guard test had a 7-line comment reproducing the .agents/lessons.md entry. Slimmed to 2 lines that link to the lesson (the rule's "extract to docs" path; the lesson IS the durable doc). - src/application/recipe-recency.ts — opt-out short-circuit had a 4-line block. Slimmed to 2; the runtime-init nuance fits. 888 tests + project check still green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
recipe_recencysubstrate — per-recipelast_run_at+run_countso agent hosts can rank live recipes ahead of historic ones viajq 'sort_by(.last_run_at // 0) | reverse'. Local-only; no upload primitive. Default ON; opt-out via.codemap/configrecipe_recency: false.What changed
Schema (
src/db.ts, +23)recipe_recency(recipe_id PK, last_run_at, run_count) STRICT, WITHOUT ROWID+idx_recipe_recency_last_run. Sibling toquery_baselines/coverage(user-data; intentionally absent fromdropAll()— survives--full/SCHEMA_VERSIONrebuilds).SCHEMA_VERSIONbump — additive table lands viaCREATE TABLE IF NOT EXISTSon next boot. Patch changeset.SCHEMA_VERSIONJSDoc clarified: "bump only on rebuild-forcing DDL changes (NOT additive)" — full policy inarchitecture.md § Schema Versioning.Engine (
src/application/recipe-recency.ts, new — 208 LoC)recordRecipeRun— eager-prune-then-upsert (single transaction; tiny indexed DELETE before conflict-update). Pure write-side.tryRecordRecipeRun— failure-isolated wrapper. Opens its own DB (executeQueryrunsPRAGMA query_only=1); short-circuits before any DB interaction when opt-out config is set; swallows errors with stderr[recency] write failed: <reason>so they NEVER block the recipe response.loadRecipeRecency— pure read; filter at SELECT (WHERE last_run_at >= cutoff); never DELETEs. ReturnsMap<recipe_id, {…}>.enrichWithRecency<T>— generic helper that injectslast_run_at+run_countper catalog entry.pruneRecipeRecency/resolveRecencyDbPath— maintenance helpers (path-resolver lets--recipes-jsonenrich withoutinitCodemap(), preserving the "no DB required" contract on never-indexed projects).Wiring
handleQueryRecipeinapplication/tool-handlers.ts(covers MCP + HTTP) andrunQueryCmdincli/cmd-query.ts(CLI). The CLI path keys recency off a localrecipeQuerySucceededflag, NOTprocess.exitCode—--cideliberately exits 1 on findings (the gating signal) even though the recipe ran cleanly. RefactoredprintFormattedQueryreturn to a discriminated union ({ ok: true, exitCode } | { ok: false }) so callers can disambiguate.--recipes-jsonenriches inline; falls back tonull/0when no DB exists (zero side effects on never-indexed projects — verified empirically).codemap://recipes+codemap://recipes/{id}— previous catalog cache dropped (caching alongside live recency would freezelast_run_atat first-read for the lifetime ofcodemap mcp/codemap serve).architecture.md § Boundary verification — recipe_recency write path): onlytool-handlers.ts+cmd-query.ts(+ test file) may import the write-path symbols. Read-path is unrestricted by design.Opt-out (
src/config.ts, +18 /src/runtime.ts, +4).codemap/configrecipe_recency: boolean(Zod-validated; defaulttrue). Whenfalse,tryRecordRecipeRunshort-circuits before openDb — no rows ever land. Cleanest opt-out shape (not "ignore the data after writing it").Docs lockstep (per Rule 10)
docs/architecture.md— newrecipe_recencytable description +§ Boundary verificationsubsection +§ Schema Versioningpolicy expansion.docs/glossary.md—recipe_recencyentry.docs/research/non-goals-reassessment-2026-05.md—§ 1.9 recipe-recencyflipped from(pending)→(shipped)in three places (research note closed per docs-governance "lift adopted decisions").docs/roadmap.md § Backlog— entry removed (item shipped).templates/agents/{rules,skills}/codemap+.agents/{rules,skills}/codemap— both pairs updated in lockstep, CLI-prefix-only delta.Misc
bun.lock+package.jsonoverrides— pinnedip-address >= 10.2.0(transitive XSS via@modelcontextprotocol/sdk → express-rate-limit; codemap MCP uses STDIO only, so the surface is dead code in our usage; CI audit was flagging it red on every PR)..agents/lessons.md— three durable lessons captured:process.exitCodeunsafe as success oracle (CI gating + cross-call poisoning); read-side substrate must be pure; semicolons inside--line comments break Node CI (Bun tests don't catch —bun:sqliteaccepts multi-statement SQL natively).Testing
src/application/recipe-recency.test.tscovering schema, helpers, eager prune, failure isolation, opt-outsrc/application/resource-handlers.test.tscoveringcodemap://recipesenrichment + cache-removal (live re-read between calls)src/cli/cmd-query-recency.test.tsexercisingrunQueryCmdend-to-end via subprocess (regression for the--ciundercount; opt-out; ad-hoc; failures)bun run checkclean; CI all 10 checks greenAcknowledgements
PR was triangulated by three review agents (Codex 5.3 / Composer / gpt-5.5) per
.agents/rules/pr-comment-fact-check.md. Two real bugs caught and fixed during review:process.exitCodefor success oracle —--cirecipe runs with findings exit 1 (the gating signal) → recency tracker treated successful runs as failures. Fixed via the discriminated-union refactor + explicit success flag.--recipes-jsoncontract. Reads are now pure (filter at SELECT); pruning moved to write-side eager.Plus one CI-only regression caught locally during reproduction (
;inside a--line comment indb.tsbroke Node's split-on-semicolon path; Bun's multi-statement-friendlysqlitemasked it).