refactor: dashboard routes, SQLite-first architecture, auto-update#64
Open
WellDunDun wants to merge 23 commits intomainfrom
Open
refactor: dashboard routes, SQLite-first architecture, auto-update#64WellDunDun wants to merge 23 commits intomainfrom
WellDunDun wants to merge 23 commits intomainfrom
Conversation
… of truth Inverts the data architecture: SQLite becomes the primary write target and query store. JSONL is demoted to an append-only audit trail with on-demand export via `selftune export`. Phase 1 — Direct-Write Layer: - New `localdb/direct-write.ts` with fail-open insert functions for all 11 tables - DB singleton pattern (`getDb()`) with prepared statement cache (WeakMap) - Two new tables: `queries`, `improvement_signals` (previously JSONL-only) Phase 2 — Dashboard Cut-Over: - Dashboard skips materializer when dual-write active - SSE watchers switched from JSONL file watching to SQLite WAL polling - Dashboard server split from 1205 → 549 lines (7 route handlers extracted) Phase 3 — Drop JSONL Writes: - All hooks, ingestors, evolution, orchestrate write only to SQLite - All 23 CLI consumers read from SQLite queries instead of readJsonl - `selftune export` command generates JSONL from SQLite on demand - Tests updated to verify return values instead of JSONL file contents DX Improvements: - Backend HMR via `bun --watch` in dev scripts - Dashboard typecheck added to CI workflow + Makefile - 2 React component smoke tests (Overview, SkillReport) Results: 1389 pass / 12 fail (baseline 1388/13), dashboard load 9.5s → 86ms Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `bun --watch run dashboard-server.ts` dev script needs the file to auto-start when executed directly, not just when imported. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…mments /simplify review found 17 issues across 3 categories. Fixed: - Converted 24 files from openDb()/close() to getDb() singleton (eliminates repeated DDL + PRAGMA overhead on every call) - Removed dead readJsonl imports from evolve.ts, evolve-body.ts, orchestrate.ts - Removed dead logPath/signalLogPath parameters in prompt-log.ts, session-stop.ts - Removed isDualWriteActive() + setMeta dead code from dashboard-server.ts - Made refreshV2Data/refreshV2DataImmediate no-ops (materializer is startup-only) - Migrated computeStatusFromLogs to computeStatusFromDb (SQLite queries) - Exported safeParseJson/safeParseJsonArray from queries.ts, deduplicated - Removed unnecessary import aliases in repair/skill-usage.ts, sync.ts - Updated stale header comments in normalization.ts, session-stop.ts, skill-eval.ts, prompt-log.ts to reflect SQLite writes Net: -137 lines across 30 files. 1390 pass / 11 fail (all pre-existing). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three improvements from the alternate architecture plan:
1. Dynamic imports in hooks — SQLite modules lazy-loaded at point of use
instead of on every hook startup. Saves ~2-5ms per invocation when
hooks exit early (most invocations). 4 hooks + 5 test files updated.
2. Test migration to _setTestDb() — 8 test files migrated from JSONL
temp paths to in-memory SQLite via _setTestDb(openDb(":memory:")).
Removed JSONL fallback branches from audit.ts, evidence.ts, rollback.ts
production code. Clean SQLite-only read/write paths.
3. Dedicated localdb unit tests — 31 new tests:
- tests/localdb/write.test.ts (17 tests for all direct-write functions)
- tests/localdb/read-queries.test.ts (14 tests for all query functions)
- tests/localdb/localdb.test.ts (+5 assertions for new tables/indexes)
Results: 1424 pass / 11 fail (was 1390/11, +34 new tests, +1 previously
failing autonomy test now passes).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughMigrates the repo to a SQLite‑first architecture: adds a DB singleton, migrations, prepared‑statement fail‑open writers, read query helpers, incremental materializer, JSONL export CLI, extracted HTTP routes, SSE live updates with a client hook, and converts many hooks/ingestors/tests to use SQLite (with JSONL fallback). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant SPA as Local Dashboard SPA
participant Server as Dashboard Server
participant DB as SQLite
participant Hooks as Agent Hooks
Client->>SPA: open page
SPA->>SPA: useSSE() (EventSource -> /api/v2/events)
SPA->>Server: EventSource connects
Hooks->>DB: write*ToDb(...) (fail‑open)
Server->>DB: query overview/skills/status
DB-->>Server: rows
Server->>SPA: SSE "update"
SPA->>SPA: queryClient.invalidateQueries()
SPA->>Server: GET /api/v2/overview
Server->>DB: fetch latest payload
DB-->>Server: payload
Server-->>SPA: JSON response
sequenceDiagram
autonumber
participant CLI
participant Hook as Session Hook
participant DB as SQLite
participant Export as Export CLI
CLI->>Hook: session-end event
Hook->>DB: writeSessionTelemetryToDb(...) (non‑blocking)
Hook->>DB: writeCanonicalToDb(...) (fail‑open)
CLI->>Export: user runs `selftune export`
Export->>DB: SELECT rows (since?)
DB-->>Export: rows
Export->>CLI: writes JSONL files and reports counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 65
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
skill/Workflows/Baseline.md (1)
95-101:⚠️ Potential issue | 🟡 MinorExpand the agent CLI mapping for all options.
The mapping table combines "claude", "codex", and "opencode" into a single "2b (specify agent)" row. This is ambiguous—agents need explicit mappings for each selection:
- 2a (auto-detect) → (no flag)
- 2b (claude) →
--agent claude- 2c (codex) →
--agent codex- 2d (opencode) →
--agent opencodeClarify whether the agent names should be used verbatim as flag values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill/Workflows/Baseline.md` around lines 95 - 101, Update the agent CLI mapping in the Selection table so each agent has its own row and explicit flag value: replace the single "2b (specify agent)" entry with separate rows for "2b (claude) → --agent claude", "2c (codex) → --agent codex", and "2d (opencode) → --agent opencode", keep "2a (auto-detect) → (no flag, default)"; use the agent names verbatim as the flag values so the README/table entries exactly match the expected CLI input.docs/operator-guide.md (1)
207-208:⚠️ Potential issue | 🟡 MinorClarify whether audit JSONL files are still written in the new architecture.
Lines 207-208 introduce
evolution_audit_log.jsonlandorchestrate_runs.jsonlas audit trails. However, the PR's phase 3 "drops JSONL writes" and moves all producers to SQLite. Are these audit files exempt from the phase 3 migration, or should the documentation note they're now stored in SQLite instead?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/operator-guide.md` around lines 207 - 208, The docs list `~/.claude/evolution_audit_log.jsonl` and `~/.claude/orchestrate_runs.jsonl` as audit trails but the PR’s phase 3 removes JSONL writes and moves producers to SQLite; update the operator guide to state whether these two artifacts are still written or have been migrated to the new SQLite-backed storage. Edit the section referencing `evolution_audit_log.jsonl` and `orchestrate_runs.jsonl` to either (A) explicitly mark them as deprecated and point to the new SQLite location and table names used by the provenance/orchestrate producers, or (B) note that they remain unchanged (if exempt) and explain why; mention the phase 3 change (“drops JSONL writes” / producers → SQLite) and link the relevant producer names so readers can find the new persistence behavior.cli/selftune/hooks/session-stop.ts (1)
46-66:⚠️ Potential issue | 🟠 MajorMake stale-lock recovery atomic and self-healing.
The
EEXISTpath has two failure modes: a partially written lock file makes every future run returnfalse, and overwriting a stale lock in place lets two concurrent hooks both "win" and spawn duplicate orchestrates. Reclaim invalid/stale locks by deleting them and retryingopenSync(lockPath, "wx")instead ofwriteFileSync(lockPath, ...).Possible fix
- let fd: number; - try { - fd = openSync(lockPath, "wx"); - writeFileSync(fd, JSON.stringify({ timestamp: new Date().toISOString(), pid: process.pid })); - closeSync(fd); - } catch (lockErr: unknown) { + const claimLock = () => { + const fd = openSync(lockPath, "wx"); + try { + writeFileSync( + fd, + JSON.stringify({ timestamp: new Date().toISOString(), pid: process.pid }), + ); + } finally { + closeSync(fd); + } + }; + + try { + claimLock(); + } catch (lockErr: unknown) { // Lock exists — check if stale if ((lockErr as NodeJS.ErrnoException).code === "EEXIST") { + let shouldReclaim = false; try { const lockContent = readFileSync(lockPath, "utf8"); - const lock = JSON.parse(lockContent); - const lockAge = Date.now() - new Date(lock.timestamp).getTime(); - if (lockAge < LOCK_STALE_MS) return false; // Active lock, skip - // Stale lock — override - writeFileSync( - lockPath, - JSON.stringify({ timestamp: new Date().toISOString(), pid: process.pid }), - ); + const lock = JSON.parse(lockContent) as { timestamp?: string }; + const lockAge = lock.timestamp + ? Date.now() - new Date(lock.timestamp).getTime() + : Number.POSITIVE_INFINITY; + shouldReclaim = lockAge >= LOCK_STALE_MS; } catch { - return false; // Can't read lock, skip + shouldReclaim = true; + } + + if (!shouldReclaim) return false; + try { + unlinkSync(lockPath); + } catch (unlinkErr) { + if ((unlinkErr as NodeJS.ErrnoException).code !== "ENOENT") return false; + } + + try { + claimLock(); + } catch (retryErr) { + if ((retryErr as NodeJS.ErrnoException).code === "EEXIST") return false; + return false; } } else { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/session-stop.ts` around lines 46 - 66, When handling the EEXIST branch for lock acquisition (openSync, writeFileSync, readFileSync, lockPath, LOCK_STALE_MS), make stale-lock recovery atomic by deleting the stale lock and retrying an exclusive open instead of overwriting in place: on EEXIST read and parse the lock, compute lockAge, and if stale call unlinkSync(lockPath) then immediately retry openSync(lockPath, "wx") to create a new lock and write the payload (ensuring you close the fd). Also handle JSON/read errors by treating them as stale candidates (unlink+retry) but bail out if unlink or the retry open fails with a non-EEXIST error; ensure only a single successful openSync ownership leads to spawning orchestration to avoid duplicate runs.tests/hooks/skill-eval.test.ts (1)
84-115:⚠️ Potential issue | 🟠 MajorThese happy-path cases still don't verify the SQLite write.
processToolUse()returns theSkillUsageRecordeven if the DB insert fails, so assertions onresultalone can pass while the SQLite migration is broken. At least one success case here should read back the row fromskill_usage(or assert the count increases) instead of only checking the returned object.Also applies to: 217-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/skill-eval.test.ts` around lines 84 - 115, Update the test in skill-eval.test.ts to verify the SQLite write instead of only asserting the returned SkillUsageRecord from processToolUse; after calling processToolUse (the function under test), open the same SQLite DB used by the code and either query the skill_usage table for the new row (matching session_id and skill_name) or assert the table row count increased by one, and assert the row's fields (skill_name, skill_path, triggered, source) match the expected values; ensure you reference the same DB file/path that processToolUse writes to so the test fails if the insert did not occur.skill/SKILL.md (1)
96-108:⚠️ Potential issue | 🟠 MajorDisambiguate
exportfromcontributein the routing table.
export datanow matches both theContributeandExportrows, so the agent has two equally plausible routes for the same request. Please make those trigger sets disjoint.Based on learnings, “selftune is agent-first: users interact through their coding agent, not the CLI directly. SKILL.md and workflow docs are the product surface” and “The SKILL.md routing table and trigger keywords are as important as the CLI code itself — they determine whether the agent can find and use the feature.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill/SKILL.md` around lines 96 - 108, The routing table has an overlapping trigger: "export data" appears in both the Contribute and Export rows, creating ambiguity; remove or rename the conflicting phrase from the Contribute trigger set so the Export row is the sole holder of "export data" (update the Contribute row keywords in SKILL.md for the Contribute entry and, if any routing code or helper that builds trigger sets references Contribute/Export by name, ensure it now only sees "export data" under Export); verify the Contribute triggers still cover sharing/anonymized/export-related intents using non-conflicting phrases like "share data" or "anonymized contribution" and confirm the Export row remains as the canonical trigger for export/dump/jsonl/export sqlite actions.skill/Workflows/Evals.md (1)
177-210:⚠️ Potential issue | 🟠 MajorThe pre-flight no longer collects enough data to build the documented command.
The new prompt only asks for mode/model/max, but the mapping table still emits
--skill-path <path>and--out <path>, and the selection labels (4a/4b/4c,Custom output path) no longer exist in the JSON question set. In synthetic mode, the agent still cannot deterministically construct a valid command from these instructions.As per coding guidelines, skill workflow documents must provide clear step-by-step instructions and no ambiguous references.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skill/Workflows/Evals.md` around lines 177 - 210, The workflow's question set is missing prompts for skill-path and output path and the selection labels in the mapping table (e.g., 4a/4b/4c, "Custom output path") no longer match the JSON questions, so update the AskUserQuestion flow to include explicit questions for "Skill path (required for synthetic mode)" and "Output path (optional)" and adjust option labels to match the mapping table; ensure that when the user picks "Synthetic" the agent records both --synthetic and the provided --skill-path <path>, when a model is chosen map to --model haiku|sonnet|opus, when max entries are chosen map to --max <value>, and when an output path is provided map to --out <path>; keep the fallback behavior to inline numbered options if AskUserQuestion isn't available and make sure the documented selection labels (e.g., 4a/4b/4c) and the mapping table are updated to exactly reflect the new questions and their CLI flag translations.cli/selftune/activation-rules.ts (1)
110-127:⚠️ Potential issue | 🔴 CriticalStale-evolution timestamp selection is inverted for DB-backed reads.
Line 125 picks
auditEntries[auditEntries.length - 1], butqueryEvolutionAudit()returns entries in descending timestamp order. This selects the oldest entry and over-reports staleness.Proposed fix
- const lastEntry = auditEntries[auditEntries.length - 1]; - const lastTimestamp = new Date(lastEntry.timestamp).getTime(); + const lastTimestamp = auditEntries.reduce((latest, entry) => { + const ts = new Date(entry.timestamp).getTime(); + return Number.isFinite(ts) && ts > latest ? ts : latest; + }, 0); + if (lastTimestamp === 0) return null; const ageMs = Date.now() - lastTimestamp;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/activation-rules.ts` around lines 110 - 127, The code assumes auditEntries are in ascending time order but queryEvolutionAudit() returns descending, so selecting auditEntries[auditEntries.length - 1] picks the oldest entry for DB-backed reads; instead determine the most recent entry explicitly (e.g., pick the entry with the greatest timestamp) and use that for lastTimestamp/age calculation. Update the selection logic around auditEntries/lastEntry/lastTimestamp in the block that computes ageMs (used with checkFalseNegatives and ctx.evolution_audit_log_path/EVOLUTION_AUDIT_LOG) to compute the latest timestamp regardless of source ordering (queryEvolutionAudit vs readJsonl).cli/selftune/evolution/evolve.ts (2)
1011-1023: 🧹 Nitpick | 🔵 TrivialPre-flight check inconsistency: DB for skills, filesystem for queries.
Line 1013 checks skill usage via DB (
querySkillUsageRecords(dbCheck).length > 0), but line 1014 checks query log via filesystem (existsSync(QUERY_LOG)). This is inconsistent with the DB-first approach used elsewhere.Consider using
queryQueryLog(dbCheck).length > 0for consistency.♻️ Suggested consistency fix
const dbCheck = getDb(); const hasSkillLog = querySkillUsageRecords(dbCheck).length > 0; - const hasQueryLog = existsSync(QUERY_LOG); + const hasQueryLog = queryQueryLog(dbCheck).length > 0; if (!hasSkillLog && !hasQueryLog) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/evolution/evolve.ts` around lines 1011 - 1023, Pre-flight check mixes DB and filesystem checks: when evalSetPath is not provided and sync-first is false, the code uses getDb() + querySkillUsageRecords(dbCheck) to check skill logs but uses existsSync(QUERY_LOG) to check query logs; change the query log check to use the DB consistently by calling queryQueryLog(dbCheck).length > 0 instead of existsSync(QUERY_LOG). Update the conditional that sets hasQueryLog, and ensure queryQueryLog is imported/available in this module (or use the same DB helper used for skills) so both hasSkillLog and hasQueryLog are derived from dbCheck.
1-1136:⚠️ Potential issue | 🟡 MinorUpdate evolution pipeline docs to document DB-backed data retrieval and new sync options.
The code introduces significant changes —
getDb()withquerySkillUsageRecords(),queryQueryLog(),querySessionTelemetry(), and new--sync-first/--sync-forceoptions — butskill/Workflows/Evolve.mdanddocs/design-docs/evolution-pipeline.mdstill describe generic "build from logs" behavior without documenting the SQLite implementation, query mechanism, or telemetry sync workflow.Update both docs to clarify:
- Data flows from SQLite DB via
querySkillUsageRecords()andqueryQueryLog()--sync-firsttriggers telemetry refresh viacreateDefaultSyncOptions()- New token efficiency scoring uses
querySessionTelemetry()when--token-efficiencyis enabled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/evolution/evolve.ts` around lines 1 - 1136, Update the two docs (skill/Workflows/Evolve.md and docs/design-docs/evolution-pipeline.md) to reflect the new DB-backed and sync-driven data flow: document that eval/telemetry are read from SQLite via getDb() and the query helpers querySkillUsageRecords() and queryQueryLog(), explain that --sync-first (and --sync-force) invoke the sync workflow via createDefaultSyncOptions() / syncSources to refresh telemetry before building evals, and note that token-efficiency scoring reads session telemetry via querySessionTelemetry() when --token-efficiency is enabled; include brief notes on where these functions are invoked in the codebase (evolve(), cliMain()) and the expected effect on eval-set generation and gating.cli/selftune/ingestors/claude-replay.ts (1)
1-380:⚠️ Potential issue | 🔴 CriticalUpdate
skill/Workflows/Ingest.mdto document SQLite writes for claude-replay ingestor.The
claude-replayingestor now writes to SQLite database (~/.selftune/selftune.db) viawriteQueryToDb(),writeSessionTelemetryToDb(), andwriteSkillUsageToDb()(lines 162, 182, 206), butIngest.mdstill describes only JSONL output paths. Update the "ingest claude" Output section to reflect the SQLite-first architecture and clarify that JSONL is now secondary/backup. Also verifyREADME.mdPlatforms section describes this correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/ingestors/claude-replay.ts` around lines 1 - 380, Update the docs to reflect that claude-replay now writes primary records to SQLite by calling writeQueryToDb, writeSessionTelemetryToDb, and writeSkillUsageToDb (used in the claude-replay ingestion flow), and that the JSONL paths previously listed are now secondary/backup outputs; specifically modify the "ingest claude" Output section in Ingest.md to state the SQLite-first architecture and mention the JSONL files as additive/backup, and update the Platforms/README.md to match this behavior and note the DB location (~/.selftune/selftune.db) and the functions involved for future maintainers.cli/selftune/hooks/evolution-guard.ts (2)
67-72:⚠️ Potential issue | 🔴 CriticalLogic bug: accessing the wrong entry due to DESC ordering.
queryEvolutionAuditreturns entries ordered bytimestamp DESC(newest first), but line 71 accessesskillEntries[skillEntries.length - 1]which gets the oldest entry. To check if the skill is currently monitored, you need the most recent action.Fix
// Filter entries for this skill by skill_name field const skillEntries = entries.filter((e) => e.skill_name === skillName); if (skillEntries.length === 0) return false; - const lastEntry = skillEntries[skillEntries.length - 1]; + const lastEntry = skillEntries[0]; // DESC order — newest first return lastEntry.action === "deployed";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/evolution-guard.ts` around lines 67 - 72, The code is selecting the oldest entry because queryEvolutionAudit returns entries ordered by timestamp DESC but the code reads skillEntries[skillEntries.length - 1]; change the logic to use the most recent entry (e.g., skillEntries[0]) or explicitly sort by timestamp and pick the newest, and then check that entry.action === "deployed" (update usage of skillEntries and lastEntry variables accordingly in evolution-guard.ts).
48-72:⚠️ Potential issue | 🔴 CriticalInconsistent entry ordering between SQLite and JSONL paths.
The SQLite path returns entries in DESC order (newest first) while the JSONL fallback returns them in file order (oldest first). This causes
skillEntries[skillEntries.length - 1]to return different entries depending on the code path.Either normalize ordering or access entries consistently:
Fix: Always get the first entry after filtering (since SQLite is DESC)
} else { - entries = readJsonl<{ skill_name?: string; action: string }>(auditLogPath); + entries = readJsonl<{ skill_name?: string; action: string }>(auditLogPath).reverse(); }Or access index 0 for SQLite path, length-1 for JSONL path. The cleanest fix is normalizing the order.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/evolution-guard.ts` around lines 48 - 72, The checkActiveMonitoring function has inconsistent entry ordering between the SQLite branch (queryEvolutionAudit returns DESC/newest-first) and the readJsonl fallback (oldest-first), so normalize ordering after you obtain entries: use the same sort direction (e.g., newest-first) for both paths and then pick the first element for the most recent record. Specifically, after entries are populated by getDb/queryEvolutionAudit or readJsonl, sort or reverse the entries array consistently (by timestamp or assumed append order) so skillEntries = entries.filter(...) yields a newest-first list, then use skillEntries[0] (instead of skillEntries[length-1]) to determine if the last action is "deployed"; keep EVOLUTION_AUDIT_LOG, getDb, queryEvolutionAudit, readJsonl, entries, and skillEntries as reference points when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: defa7de3-e0ab-4f7e-82d2-6c5ed2cc5f85
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (105)
.github/workflows/ci.ymlAGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.mdMakefileapps/local-dashboard/HANDOFF.mdapps/local-dashboard/package.jsonapps/local-dashboard/src/App.tsxapps/local-dashboard/src/components/app-sidebar.tsxapps/local-dashboard/src/hooks/useDoctor.tsapps/local-dashboard/src/hooks/useOrchestrateRuns.tsapps/local-dashboard/src/hooks/useOverview.tsapps/local-dashboard/src/hooks/useSSE.tsapps/local-dashboard/src/hooks/useSkillReport.tsapps/local-dashboard/src/pages/Overview.test.tsxapps/local-dashboard/src/pages/Overview.tsxapps/local-dashboard/src/pages/SkillReport.test.tsxapps/local-dashboard/src/pages/SkillReport.tsxapps/local-dashboard/src/styles.cssapps/local-dashboard/vite.config.tscli/selftune/activation-rules.tscli/selftune/auto-update.tscli/selftune/badge/badge.tscli/selftune/canonical-export.tscli/selftune/contribute/bundle.tscli/selftune/dashboard-contract.tscli/selftune/dashboard-server.tscli/selftune/eval/baseline.tscli/selftune/eval/hooks-to-evals.tscli/selftune/evolution/audit.tscli/selftune/evolution/evidence.tscli/selftune/evolution/evolve-body.tscli/selftune/evolution/evolve.tscli/selftune/evolution/rollback.tscli/selftune/export.tscli/selftune/grading/auto-grade.tscli/selftune/grading/grade-session.tscli/selftune/hooks/evolution-guard.tscli/selftune/hooks/prompt-log.tscli/selftune/hooks/session-stop.tscli/selftune/hooks/skill-eval.tscli/selftune/index.tscli/selftune/ingestors/claude-replay.tscli/selftune/last.tscli/selftune/localdb/db.tscli/selftune/localdb/direct-write.tscli/selftune/localdb/materialize.tscli/selftune/localdb/queries.tscli/selftune/localdb/schema.tscli/selftune/monitoring/watch.tscli/selftune/normalization.tscli/selftune/orchestrate.tscli/selftune/quickstart.tscli/selftune/repair/skill-usage.tscli/selftune/routes/actions.tscli/selftune/routes/badge.tscli/selftune/routes/doctor.tscli/selftune/routes/index.tscli/selftune/routes/orchestrate-runs.tscli/selftune/routes/overview.tscli/selftune/routes/report.tscli/selftune/routes/skill-report.tscli/selftune/status.tscli/selftune/sync.tscli/selftune/types.tscli/selftune/utils/jsonl.tscli/selftune/utils/skill-log.tscli/selftune/utils/transcript.tscli/selftune/workflows/workflows.tsdocs/design-docs/index.mddocs/design-docs/live-dashboard-sse.mddocs/design-docs/sqlite-first-migration.mddocs/exec-plans/tech-debt-tracker.mddocs/operator-guide.mdpackage.jsonpackages/telemetry-contract/src/types.tspackages/ui/src/components/section-cards.tsxskill/SKILL.mdskill/Workflows/Baseline.mdskill/Workflows/Dashboard.mdskill/Workflows/Evals.mdskill/Workflows/Evolve.mdskill/Workflows/EvolveBody.mdskill/assets/multi-skill-settings.jsonskill/assets/single-skill-settings.jsonskill/references/invocation-taxonomy.mdskill/settings_snippet.jsontests/autonomy-proof.test.tstests/evolution/audit.test.tstests/evolution/evidence.test.tstests/evolution/integration.test.tstests/evolution/rollback.test.tstests/hooks/evolution-guard.test.tstests/hooks/prompt-log.test.tstests/hooks/session-stop.test.tstests/hooks/signal-detection.test.tstests/hooks/skill-eval.test.tstests/ingestors/claude-replay.test.tstests/ingestors/codex-rollout.test.tstests/ingestors/openclaw-ingest.test.tstests/ingestors/opencode-ingest.test.tstests/localdb/localdb.test.tstests/localdb/read-queries.test.tstests/localdb/write.test.ts
💤 Files with no reviewable changes (1)
- apps/local-dashboard/HANDOFF.md
Comment on lines
+61
to
+63
| system: { label: "System", icon: <ServerIcon className="size-4" /> }, | ||
| admin: { label: "Admin", icon: <GlobeIcon className="size-4" /> }, | ||
| unknown: { label: "Unknown", icon: <HelpCircleIcon className="size-4" /> }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tighten scope typing to prevent contract drift.
scope/SCOPE_CONFIG/order are still stringly-typed. This weakens compiler checks and can silently desync from dashboard contract evolution. Use a shared SkillScope union (or imported contract type) and type SCOPE_CONFIG + order against it.
Type-safe scope wiring
+type SkillScope = "global" | "project" | "system" | "admin" | "unknown"
interface SkillNavItem {
name: string
- scope: string | null
+ scope: SkillScope | null
status: SkillHealthStatus
passRate: number | null
checks: number
}
-const SCOPE_CONFIG: Record<string, { label: string; icon: React.ReactNode }> = {
+const SCOPE_CONFIG: Record<SkillScope, { label: string; icon: React.ReactNode }> = {
project: { label: "Project", icon: <FolderIcon className="size-4" /> },
global: { label: "Global", icon: <GlobeIcon className="size-4" /> },
system: { label: "System", icon: <ServerIcon className="size-4" /> },
admin: { label: "Admin", icon: <GlobeIcon className="size-4" /> },
unknown: { label: "Unknown", icon: <HelpCircleIcon className="size-4" /> },
}
-const order = ["global", "project", "system", "admin", "unknown"]
+const order: SkillScope[] = ["global", "project", "system", "admin", "unknown"]Also applies to: 150-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/components/app-sidebar.tsx` around lines 61 - 63,
The scope strings are currently untyped and can drift from the dashboard
contract; introduce or import a SkillScope union type (or the contract type) and
use it to type SCOPE_CONFIG, its keys, and the order array so the compiler
enforces valid scopes. Update the SCOPE_CONFIG declaration, the order variable,
and any scope-related props/params (e.g., the scope variable used in the sidebar
render) to use SkillScope instead of plain string, and adjust any object index
signatures (SCOPE_CONFIG[...]) to reflect Record<SkillScope, ...> so mismatches
surface at compile time. Ensure imports reference the shared contract type
(e.g., SkillScope or the dashboard contract type) and run type checks to fix
resulting type errors.
Comment on lines
+47
to
+51
| it("module exports Overview component", async () => { | ||
| const { Overview } = await import("./Overview"); | ||
| expect(Overview).toBeDefined(); | ||
| expect(typeof Overview).toBe("function"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test currently validates module shape, not component behavior.
Line 47–51 only asserts export existence/type. Add at least one render-level assertion (e.g., expected section heading or loading state) so UI regressions in Overview are caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/pages/Overview.test.tsx` around lines 47 - 51, The
test only checks that the module exports Overview and not its UI; update the
test for the Overview component to render it (use the existing test framework
helpers or RTL render) and add at least one render-level assertion such as
checking for a known heading, a loading indicator, or specific text that
Overview should display (e.g., call render(Overview) or render(<Overview />) and
assert with screen.getByText/screen.getByRole or queryByText to verify the
expected element appears), keeping the existing export assertions but adding the
rendering assertion to catch UI regressions.
Comment on lines
+80
to
+85
| describe("SkillReport", () => { | ||
| it("module exports SkillReport component", async () => { | ||
| const { SkillReport } = await import("./SkillReport"); | ||
| expect(SkillReport).toBeDefined(); | ||
| expect(typeof SkillReport).toBe("function"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add at least one behavior assertion, not just export shape.
Line [81]–Line [85] only verifies the symbol exists. This can still pass if SkillReport breaks during render/data-state handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/pages/SkillReport.test.tsx` around lines 80 - 85,
The test currently only checks the symbol shape; update the "SkillReport" spec
to actually render the SkillReport component (use react-testing-library's render
or equivalent), provide minimal required props or mocked context/data, and
assert one meaningful behavior such as presence of expected text/node, a role,
or a piece of state-dependent output (or a focused snapshot) to verify
render/data-state handling for the SkillReport component; reference the
SkillReport symbol and the existing "module exports SkillReport component" test
to replace or augment the shape assertions with the runtime render assertion.
CLAUDE.md
Outdated
| @@ -0,0 +1 @@ | |||
| @AGENTS.md No newline at end of file | |||
There was a problem hiding this comment.
Fix markdownlint violations for this new file.
Line 1 currently triggers MD041 (missing top-level heading) and MD047 (missing trailing newline). Either add a heading + final newline, or explicitly exclude this file from markdownlint if it must remain a single include directive.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 1-1: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 1, The file CLAUDE.md is triggering markdownlint rules
MD041 (missing top-level heading) and MD047 (missing trailing newline); fix by
either adding a top-level heading (e.g., prepend a line like "# CLAUDE" or
another appropriate H1) to the top of CLAUDE.md and ensure the file ends with a
newline, or if the file must remain a single include directive, add an explicit
markdownlint disable comment for MD041 and MD047 at the top of CLAUDE.md to
exclude it from linting.
Comment on lines
+71
to
+75
| if (cache && Date.now() - cache.lastCheck < CHECK_INTERVAL_MS) { | ||
| // Even with a recent check, if we know we're outdated, try updating | ||
| if (cache.latestVersion && compareSemver(currentVersion, cache.latestVersion) < 0) { | ||
| await performUpdate(currentVersion, cache.latestVersion); | ||
| } |
There was a problem hiding this comment.
Throttle failed installs in the cached fast path.
When cache.latestVersion is newer, this branch reruns performUpdate() on every invocation until the cache expires. A single permission or path failure will therefore block each command with the same 30s synchronous install attempt.
Comment on lines
+454
to
463
| // Verify canonical records structure via the exported builder | ||
| const canonicalRecords = buildCanonicalRecordsFromOpenClaw(session); | ||
| const canonicalSession = canonicalRecords.find((r) => r.record_kind === "session"); | ||
| expect(canonicalSession).toBeTruthy(); | ||
| expect(canonicalSession.platform).toBe("openclaw"); | ||
| expect(canonicalSession.capture_mode).toBe("batch_ingest"); | ||
|
|
||
| const canonicalInvocation = readFileSync(canonicalLog, "utf-8") | ||
| .trim() | ||
| .split("\n") | ||
| .map((l: string) => JSON.parse(l)) | ||
| .find((r: Record<string, unknown>) => r.record_kind === "skill_invocation"); | ||
| expect(canonicalInvocation?.invocation_mode).toBe("inferred"); | ||
| expect((canonicalSession as Record<string, unknown>).platform).toBe("openclaw"); | ||
| expect((canonicalSession as Record<string, unknown>).capture_mode).toBe("batch_ingest"); | ||
|
|
||
| const canonicalInvocation = canonicalRecords.find((r) => r.record_kind === "skill_invocation"); | ||
| expect((canonicalInvocation as Record<string, unknown>)?.invocation_mode).toBe("inferred"); | ||
| }); |
There was a problem hiding this comment.
Test no longer verifies writeSession canonical file output.
This block validates the builder directly, but it no longer checks what writeSession(...) actually persisted to canonicalLog. A regression in canonical file writes would now be missed.
Proposed fix
- // Verify canonical records structure via the exported builder
- const canonicalRecords = buildCanonicalRecordsFromOpenClaw(session);
+ // Verify canonical records persisted by writeSession
+ const canonicalLines = readFileSync(canonicalLog, "utf-8").trim().split("\n");
+ const canonicalRecords = canonicalLines.map((line) => JSON.parse(line) as Record<string, unknown>);
const canonicalSession = canonicalRecords.find((r) => r.record_kind === "session");
expect(canonicalSession).toBeTruthy();
- expect((canonicalSession as Record<string, unknown>).platform).toBe("openclaw");
- expect((canonicalSession as Record<string, unknown>).capture_mode).toBe("batch_ingest");
+ expect(canonicalSession?.platform).toBe("openclaw");
+ expect(canonicalSession?.capture_mode).toBe("batch_ingest");
const canonicalInvocation = canonicalRecords.find((r) => r.record_kind === "skill_invocation");
- expect((canonicalInvocation as Record<string, unknown>)?.invocation_mode).toBe("inferred");
+ expect(canonicalInvocation?.invocation_mode).toBe("inferred");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ingestors/openclaw-ingest.test.ts` around lines 454 - 463, The test
currently validates buildCanonicalRecordsFromOpenClaw(session) but no longer
asserts what writeSession(...) actually persisted; update the test to call
writeSession with the session (or ensure the existing writeSession call ran)
then read back the produced canonicalLog (or the file it writes) and assert its
parsed contents match the canonicalRecords (e.g., verify record_kind "session"
has platform "openclaw" and capture_mode "batch_ingest", and the
"skill_invocation" record has invocation_mode "inferred"). Locate the test
helpers buildCanonicalRecordsFromOpenClaw and the writeSession invocation and
add assertions that parse canonicalLog output and compare key fields to the
builder output to prevent regressions in file writes.
Comment on lines
+436
to
+440
| // Verify canonical records structure via the exported builder | ||
| const canonicalRecords = buildCanonicalRecordsFromOpenCode(session); | ||
| expect(canonicalRecords.some((r) => r.record_kind === "session")).toBe(true); | ||
| const canonicalInvocation = canonicalRecords.find((r) => r.record_kind === "skill_invocation"); | ||
| expect((canonicalInvocation as Record<string, unknown>)?.invocation_mode).toBe("inferred"); |
There was a problem hiding this comment.
Canonical assertions bypass writeSession output.
At Line 437, the test validates builder output directly instead of the canonicalLog generated by writeSession(...). This can pass even if canonical file writing regresses.
💡 Suggested fix (assert canonical file output)
- // Verify canonical records structure via the exported builder
- const canonicalRecords = buildCanonicalRecordsFromOpenCode(session);
- expect(canonicalRecords.some((r) => r.record_kind === "session")).toBe(true);
- const canonicalInvocation = canonicalRecords.find((r) => r.record_kind === "skill_invocation");
- expect((canonicalInvocation as Record<string, unknown>)?.invocation_mode).toBe("inferred");
+ // Verify canonical records written by writeSession
+ const canonicalLines = readFileSync(canonicalLog, "utf-8").trim().split("\n");
+ const canonicalRecords = canonicalLines.map((line) => JSON.parse(line) as Record<string, unknown>);
+ expect(canonicalRecords.some((r) => r.record_kind === "session")).toBe(true);
+ const canonicalInvocation = canonicalRecords.find((r) => r.record_kind === "skill_invocation");
+ expect(canonicalInvocation?.invocation_mode).toBe("inferred");
+
+ // Optional: keep builder parity check to catch schema drift
+ expect(canonicalRecords).toEqual(
+ buildCanonicalRecordsFromOpenCode(session) as unknown as Record<string, unknown>[],
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ingestors/opencode-ingest.test.ts` around lines 436 - 440, The test
currently asserts builder output via buildCanonicalRecordsFromOpenCode(session)
which bypasses file-writing; instead invoke writeSession(...) to produce the
actual canonical output, read/parse the resulting canonicalLog file, and assert
the same expectations (presence of a record with record_kind === "session" and a
skill_invocation record with invocation_mode === "inferred"). Update the test to
call writeSession(session, ...) (matching existing test harness args), locate
and parse the produced canonicalLog file, perform the two expects against that
parsed file, and remove any direct assertion against
buildCanonicalRecordsFromOpenCode so the test covers real file output.
Comment on lines
+152
to
+153
| {invocations.map((inv, i) => ( | ||
| <TableRow key={i} className={!inv.triggered ? "bg-destructive/5" : ""}> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid using array index as React key for dynamic lists.
Using i as the key in invocations.map((inv, i) => ...) can cause rendering issues if invocations are reordered or filtered. Consider using a composite key from the invocation data.
Suggested fix
- {invocations.map((inv, i) => (
- <TableRow key={i} className={!inv.triggered ? "bg-destructive/5" : ""}>
+ {invocations.map((inv) => (
+ <TableRow key={`${inv.timestamp}-${inv.session_id}-${inv.query?.slice(0, 20)}`} className={!inv.triggered ? "bg-destructive/5" : ""}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {invocations.map((inv, i) => ( | |
| <TableRow key={i} className={!inv.triggered ? "bg-destructive/5" : ""}> | |
| {invocations.map((inv) => ( | |
| <TableRow key={`${inv.timestamp}-${inv.session_id}-${inv.query?.slice(0, 20)}`} className={!inv.triggered ? "bg-destructive/5" : ""}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/pages/SkillReport.tsx` around lines 152 - 153, The
TableRow key currently uses the array index in invocations.map((inv, i) => ...),
which can break React reconciliation when the list is reordered or filtered;
update the key to use a stable unique identifier from the invocation object
(e.g., inv.id, inv.invocationId, or a composite such as
`${inv.id}-${inv.timestamp}`) and only fall back to the index if no stable id
exists so the TableRow key becomes stable across renders.
Comment on lines
+295
to
+328
| const canonicalByTimestamp = new Map( | ||
| (canonical_invocations ?? []).map((ci) => [ci.timestamp || ci.occurred_at, ci]) | ||
| ) | ||
| const mergedInvocations = recent_invocations.map((ri) => { | ||
| const ci = canonicalByTimestamp.get(ri.timestamp) | ||
| return { | ||
| timestamp: ri.timestamp, | ||
| session_id: ri.session_id, | ||
| triggered: ri.triggered, | ||
| query: ri.query, | ||
| source: ri.source, | ||
| invocation_mode: ci?.invocation_mode ?? null, | ||
| confidence: ci?.confidence ?? null, | ||
| tool_name: ci?.tool_name ?? null, | ||
| agent_type: ci?.agent_type ?? null, | ||
| } | ||
| }) | ||
| // Also add any canonical entries not matched by timestamp | ||
| for (const ci of canonical_invocations ?? []) { | ||
| const ts = ci.timestamp || ci.occurred_at | ||
| if (!recent_invocations.some((ri) => ri.timestamp === ts)) { | ||
| mergedInvocations.push({ | ||
| timestamp: ts, | ||
| session_id: ci.session_id, | ||
| triggered: ci.triggered, | ||
| query: "", | ||
| source: "", | ||
| invocation_mode: ci.invocation_mode, | ||
| confidence: ci.confidence, | ||
| tool_name: ci.tool_name ?? null, | ||
| agent_type: ci.agent_type ?? null, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Timestamp-based deduplication may cause data loss.
Using timestamp alone as a key can cause collisions when multiple invocations occur in the same millisecond. Additionally, the some() check inside the loop is O(n) per iteration.
Suggested optimization
const canonicalByTimestamp = new Map(
(canonical_invocations ?? []).map((ci) => [ci.timestamp || ci.occurred_at, ci])
)
+ const recentTimestamps = new Set(recent_invocations.map((ri) => ri.timestamp))
const mergedInvocations = recent_invocations.map((ri) => {
// ... existing mapping
})
// Also add any canonical entries not matched by timestamp
for (const ci of canonical_invocations ?? []) {
const ts = ci.timestamp || ci.occurred_at
- if (!recent_invocations.some((ri) => ri.timestamp === ts)) {
+ if (!recentTimestamps.has(ts)) {
mergedInvocations.push({
// ...
})
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/local-dashboard/src/pages/SkillReport.tsx` around lines 295 - 328, The
dedup logic using timestamp as the Map key (canonicalByTimestamp) can collide
for multiple invocations in the same millisecond and the current existence check
uses recent_invocations.some(...) making it O(n^2); instead build and use a
composite key (e.g., combine timestamp or occurred_at with session_id or another
stable unique identifier) when creating canonicalByTimestamp and when looking up
ci for each ri in mergedInvocations, and maintain a Set of those composite keys
for constant-time existence checks when iterating canonical_invocations to
append unmatched entries; update references to timestamp/occurred_at,
session_id, canonical_invocations, recent_invocations, canonicalByTimestamp,
mergedInvocations accordingly.
Comment on lines
+181
to
+190
| // Hooks write directly to SQLite, so periodic re-materialization is not needed. | ||
| // These functions are retained as no-ops because they are called from multiple | ||
| // places in the request handler and the file-change watcher. | ||
| function refreshV2Data(): void { | ||
| if (!db) return; | ||
| const now = Date.now(); | ||
| if (now - Math.max(lastV2MaterializedAt, lastV2RefreshAttemptAt) < V2_MATERIALIZE_TTL_MS) { | ||
| return; | ||
| // No-op: materializer runs once at startup only | ||
| } | ||
|
|
||
| function refreshV2DataImmediate(): void { | ||
| // No-op: materializer runs once at startup only | ||
| } |
There was a problem hiding this comment.
These SSE invalidations still follow JSONL, not SQLite.
The adjacent comments say hooks now write directly to SQLite, but the push-update path only watches TELEMETRY_LOG, QUERY_LOG, and EVOLUTION_AUDIT_LOG. Once writes bypass those files, /api/v2/events stops emitting update events for normal dashboard changes.
Also applies to: 217-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/dashboard-server.ts` around lines 181 - 190, The SSE
invalidation path only watches TELEMETRY_LOG, QUERY_LOG, and EVOLUTION_AUDIT_LOG
so updates that now go directly to SQLite never trigger /api/v2/events; to fix,
either: (A) update the push-update/file-change watcher to also listen for SQLite
change events (or call the same broadcast path) and invoke
refreshV2DataImmediate() / refreshV2Data() when SQLite writes occur, or (B)
modify the SQLite write hooks to emit the same update signal the watcher expects
(e.g., write a short JSONL entry or call the event broadcast used by
/api/v2/events). Locate and update the code paths around refreshV2Data,
refreshV2DataImmediate, the file-change watcher that references
TELEMETRY_LOG/QUERY_LOG/EVOLUTION_AUDIT_LOG, and the SQLite write hook to ensure
one of these actions is performed so SSEs resume on normal dashboard changes.
Comment on lines
+30
to
+40
| const db = getDb(); | ||
| const entries = queryEvolutionAudit(db, skillName) as EvolutionAuditEntry[]; | ||
| if (!skillName) return entries; | ||
| // queryEvolutionAudit filters by skill_name field; also filter by details | ||
| // for backward compatibility (some entries may have skill name in details only) | ||
| const needle = skillName.toLowerCase(); | ||
| return entries.filter((e) => (e.details ?? "").toLowerCase().includes(needle)); | ||
| return entries.length > 0 | ||
| ? entries | ||
| : (queryEvolutionAudit(db) as EvolutionAuditEntry[]).filter( | ||
| (e) => (e.details ?? "").toLowerCase().includes(needle), | ||
| ); |
There was a problem hiding this comment.
Merge legacy detail matches with exact skill_name matches.
Lines 36-40 only run the backward-compatibility filter when the exact SQLite query returns zero rows. In a mixed dataset, that drops older rows whose skill_name is null but whose details still mention the skill, so getLastDeployedProposal() can return the wrong deploy history. Union the exact-match and legacy-match sets, then dedupe/sort them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/evolution/audit.ts` around lines 30 - 40, The current logic only
falls back to legacy detail-based matches when exact queryEvolutionAudit(db,
skillName) returns zero, which misses legacy entries when exact matches exist;
modify the function that calls getDb() and queryEvolutionAudit (the block
returning entries) to always union the exact-match set (queryEvolutionAudit(db,
skillName)) with the legacy-match set ((queryEvolutionAudit(db) filtered by
(e.details ?? "").toLowerCase().includes(needle))), then dedupe by a stable key
(e.g., id or timestamp + details) and return the combined list sorted
consistently (e.g., by created_at/timestamp descending) so
getLastDeployedProposal and other callers see both modern and legacy matches in
a deterministic order.
Comment on lines
+90
to
+93
| // Read telemetry and skill usage logs from SQLite | ||
| const db = getDb(); | ||
| const telemetry = querySessionTelemetry(db) as SessionTelemetryRecord[]; | ||
| const usage = querySkillUsageRecords(db) as SkillUsageRecord[]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertions on query results are fragile.
Casting querySessionTelemetry(db) and querySkillUsageRecords(db) with as bypasses compile-time verification. If the query functions already return typed results, the assertions are redundant; if they return unknown[], the cast is unsafe.
Consider having the query functions return properly typed results or adding runtime validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/workflows/workflows.ts` around lines 90 - 93, The casts on
querySessionTelemetry(db) and querySkillUsageRecords(db) are unsafe; update the
query implementations to return properly typed arrays (e.g., change their
signatures to return SessionTelemetryRecord[] and SkillUsageRecord[]), remove
the "as" assertions in workflows.ts, and/or add runtime validation inside
querySessionTelemetry and querySkillUsageRecords to verify records match
SessionTelemetryRecord and SkillUsageRecord shapes before returning them; keep
getDb usage unchanged and ensure any callers expect the newly-typed return
values.
Comment on lines
+8
to
16
| beforeEach(() => { | ||
| counter = 0; | ||
| const testDb = openDb(":memory:"); | ||
| _setTestDb(testDb); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (tempDir) { | ||
| rmSync(tempDir, { recursive: true, force: true }); | ||
| tempDir = ""; | ||
| } | ||
| _setTestDb(null); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider closing the DB before nullifying the singleton.
The test DB is created in beforeEach but not explicitly closed in afterEach. While SQLite in-memory DBs are garbage collected, explicitly closing ensures resources are freed immediately and matches the pattern used in other test files.
Suggested fix
+let db: ReturnType<typeof openDb>;
+
beforeEach(() => {
counter = 0;
- const testDb = openDb(":memory:");
- _setTestDb(testDb);
+ db = openDb(":memory:");
+ _setTestDb(db);
});
afterEach(() => {
+ db.close();
_setTestDb(null);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| counter = 0; | |
| const testDb = openDb(":memory:"); | |
| _setTestDb(testDb); | |
| }); | |
| afterEach(() => { | |
| if (tempDir) { | |
| rmSync(tempDir, { recursive: true, force: true }); | |
| tempDir = ""; | |
| } | |
| _setTestDb(null); | |
| }); | |
| let db: ReturnType<typeof openDb>; | |
| beforeEach(() => { | |
| counter = 0; | |
| db = openDb(":memory:"); | |
| _setTestDb(db); | |
| }); | |
| afterEach(() => { | |
| db.close(); | |
| _setTestDb(null); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/evolution/evidence.test.ts` around lines 8 - 16, The in-memory SQLite
DB created with openDb(":memory:") in beforeEach is never closed; make the DB
instance available to afterEach (e.g., declare let testDb: ReturnType<typeof
openDb> or let testDb: any at the top of the test block), assign it in
beforeEach (testDb = openDb(":memory."); _setTestDb(testDb)), and in afterEach
call testDb.close() before calling _setTestDb(null) to ensure the database is
explicitly closed and resources are freed.
Eliminates the dual-table architecture that caused missed triggers and 1218 errors bugs. skill_invocations now has query, skill_path, skill_scope, source columns from skill_usage. All reads/writes use one table. Also addresses CodeRabbit review: _setTestDb leak fix, SQLite-before-JSONL write ordering, watch.ts sort order, sync.ts --skill-log respect, repo_remote credential sanitization, jsonl.ts EOF handling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sertions - normalization.ts: prompt state recovery reads SQLite first, JSONL fallback - CLAUDE.md: add heading + trailing newline (markdownlint) - Workflow docs: cancel = abort, not use defaults - sqlite-first-migration.md: --since DATE is implemented - Tests: close DB in afterEach, optional chaining, DB write assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- dashboard-server.ts: remove db.close() on singleton, fix async statusLoader type - badge.ts: fix BadgeFormat import from badge-data.js - direct-write.ts: add normalization_run case to both dispatchers - normalization.ts: re-add JSONL append for prompt state recovery - materialize.ts: parameterize SQL to prevent injection pattern - tests: replace require() with ESM import, use optional chaining, add existence assertions, PRAGMA table_info schema checks - operator-guide.md: clarify SQLite is primary, warn about data loss on delete Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/selftune/hooks/session-stop.ts (1)
46-71:⚠️ Potential issue | 🔴 CriticalReclaim stale locks with another atomic claim.
Overwriting the stale file in place is a TOCTOU window: two stop hooks can both decide the lock is stale and both launch
selftune orchestrate. Remove the stale file and retryopenSync(lockPath, "wx")instead.Possible fix
const lockAge = Date.now() - new Date(lock.timestamp).getTime(); if (lockAge < LOCK_STALE_MS) return false; // Active lock, skip - // Stale lock — override - writeFileSync( - lockPath, - JSON.stringify({ timestamp: new Date().toISOString(), pid: process.pid }), - ); + // Stale lock — remove it, then reacquire atomically + unlinkSync(lockPath); + fd = openSync(lockPath, "wx"); + writeFileSync(fd, JSON.stringify({ timestamp: new Date().toISOString(), pid: process.pid })); + closeSync(fd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/session-stop.ts` around lines 46 - 71, The current locking logic (openSync, writeFileSync, readFileSync, closeSync around lockPath and LOCK_STALE_MS) overwrites a stale lock in-place, creating a TOCTOU race; change it to remove the stale file and retry the atomic creation instead: when you detect EEXIST and determine the lock is stale (using the parsed lock.timestamp and LOCK_STALE_MS), call unlinkSync(lockPath) (or equivalent) and then re-attempt openSync(lockPath, "wx") to claim the lock; ensure you closeSync(fd) on success, handle and propagate any errors from unlink/open appropriately, and bound the retry (single retry or small loop) to avoid livelock.
♻️ Duplicate comments (7)
tests/hooks/prompt-log.test.ts (1)
163-168:⚠️ Potential issue | 🟡 MinorUse
await import()instead of CommonJSrequire()for ESM consistency.The test file uses ESNext modules, but line 164 uses CommonJS
require(). Since the test function is async, use dynamic import:- const { readFileSync } = require("node:fs"); + const { readFileSync } = await import("node:fs"); const state = JSON.parse(readFileSync(promptStatePath, "utf-8"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/prompt-log.test.ts` around lines 163 - 168, Replace the CommonJS require with an ESM dynamic import since the test is async: use await import("node:fs") to get readFileSync (or better, use the fs/promises API) and then parse the file; update the assertion block that references promptStatePath and state to use the imported readFileSync (or await fs.readFile if switching to promises) so the test remains ESM-consistent.tests/localdb/localdb.test.ts (1)
54-63: 🧹 Nitpick | 🔵 TrivialTable tests verify accessibility but not schema shape.
These tests only confirm the tables exist and are queryable. To catch schema drift, verify actual columns:
it("creates queries table with expected columns", () => { - const rows = db.query("SELECT * FROM queries LIMIT 1").all(); - expect(rows).toHaveLength(0); + const cols = db.query("PRAGMA table_info(queries)").all() as Array<{ name: string }>; + const names = cols.map((c) => c.name); + expect(names).toEqual(expect.arrayContaining(["timestamp", "session_id", "query"])); }); it("creates improvement_signals table with expected columns", () => { - const rows = db.query("SELECT * FROM improvement_signals LIMIT 1").all(); - expect(rows).toHaveLength(0); + const cols = db.query("PRAGMA table_info(improvement_signals)").all() as Array<{ name: string }>; + const names = cols.map((c) => c.name); + expect(names).toEqual(expect.arrayContaining(["timestamp", "session_id", "signal_type"])); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/localdb/localdb.test.ts` around lines 54 - 63, The tests named "creates queries table with expected columns" and "creates improvement_signals table with expected columns" only check that SELECT works (zero rows) but not the actual schema; update each test to query the schema (e.g., use db.query("PRAGMA table_info('queries')").all() or db.query("SELECT sql FROM sqlite_master WHERE name='queries'").all()) and assert the returned columns match the expected column names/types (compare against an expectedColumns array) instead of only asserting row count; do the same for improvement_signals and use the existing db.query helper to locate these checks.cli/selftune/localdb/schema.ts (1)
10-73:⚠️ Potential issue | 🔴 CriticalThe canonical SQLite schema still drops contract fields.
Even after this expansion, there is still nowhere to persist several fields that the canonical builders emit, e.g.
sessions.agent_id/provider/end_reason,prompts.prompt_hash/parent_prompt_id/source_message_id,skill_invocations.skill_version_hash/tool_call_id, andexecution_facts.bash_commands_redacted/end_reason.writeCanonicalToDb()will continue silently discarding them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/schema.ts` around lines 10 - 73, The SQL schema constants (CREATE_SESSIONS, CREATE_PROMPTS, CREATE_SKILL_INVOCATIONS, CREATE_EXECUTION_FACTS) are missing columns for several canonical fields so writeCanonicalToDb() silently drops data; update these CREATE_* statements to add the missing columns: in CREATE_SESSIONS add agent_id, provider, end_reason; in CREATE_PROMPTS add prompt_hash, parent_prompt_id, source_message_id; in CREATE_SKILL_INVOCATIONS add skill_version_hash and tool_call_id; in CREATE_EXECUTION_FACTS add bash_commands_redacted and end_reason (choose appropriate TEXT/INTEGER/JSON types), and ensure any new FK or NOT NULL constraints mirror how writeCanonicalToDb() writes those fields so persisted data is not discarded.tests/localdb/read-queries.test.ts (1)
568-578:⚠️ Potential issue | 🟡 MinorDrop the non-null assertions in these expectations.
The preceding
toBeDefined()checks are enough for test intent; use optional chaining here to match the test guideline and avoid!.As per coding guidelines: "tests/**/*.ts": "Optional chaining (result?.) not non-null assertions (result!.)"Possible fix
const research = list.find((s) => s.skill_name === "Research"); expect(research).toBeDefined(); - expect(research!.total_checks).toBe(2); - expect(research!.triggered_count).toBe(1); - expect(research!.pass_rate).toBe(0.5); - expect(research!.has_evidence).toBe(true); + expect(research?.total_checks).toBe(2); + expect(research?.triggered_count).toBe(1); + expect(research?.pass_rate).toBe(0.5); + expect(research?.has_evidence).toBe(true); const browser = list.find((s) => s.skill_name === "Browser"); expect(browser).toBeDefined(); - expect(browser!.total_checks).toBe(1); - expect(browser!.has_evidence).toBe(false); + expect(browser?.total_checks).toBe(1); + expect(browser?.has_evidence).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/localdb/read-queries.test.ts` around lines 568 - 578, The assertions use non-null assertions (research! and browser!) after calling list.find and already checking toBeDefined(); remove the `!` and use optional chaining (e.g., research?.total_checks, research?.triggered_count, research?.pass_rate, research?.has_evidence and browser?.total_checks, browser?.has_evidence) in those expect(...) calls so the tests rely on the prior toBeDefined() checks and follow the project guideline against `!` in tests.cli/selftune/normalization.ts (1)
124-125:⚠️ Potential issue | 🟠 MajorEither keep the JSONL fallback alive or remove it.
derivePromptSessionStateFromCanonicalLog()still falls back toCANONICAL_LOG, but these helpers stopped writing that file and ignorelogPath. After state-file loss plus any DB read failure, recovery rebuilds from stale data.Possible fix
export function appendCanonicalRecord(record: CanonicalRecord, logPath: string = CANONICAL_LOG): void { writeCanonicalToDb(record); + try { + appendJsonl(logPath, record); + } catch { /* best-effort legacy fallback */ } } export function appendCanonicalRecords(records: CanonicalRecord[], logPath: string = CANONICAL_LOG): void { writeCanonicalBatchToDb(records); + for (const record of records) { + try { + appendJsonl(logPath, record); + } catch { /* best-effort legacy fallback */ } + } }Also add
appendJsonlback to the imports if you keep the fallback.Also applies to: 388-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/normalization.ts` around lines 124 - 125, The fallback to canonical JSONL is now broken because helpers stopped writing CANONICAL_LOG and ignore logPath; either fully remove the fallback usage in derivePromptSessionStateFromCanonicalLog and related code paths (delete references to CANONICAL_LOG/_canonicalLogPath/canonicalLogPath) or restore the JSONL fallback by reintroducing appendJsonl to the imports and update the writers (the functions that previously emitted canonical JSONL) to honor logPath and write CANONICAL_LOG again so recovery can rebuild from up-to-date data; pick one approach and make sure all references (derivePromptSessionStateFromCanonicalLog, CANONICAL_LOG, _canonicalLogPath, canonicalLogPath) are consistently updated.tests/hooks/signal-detection.test.ts (1)
175-184:⚠️ Potential issue | 🟠 MajorAssert the negative path against SQLite, not
signals.jsonl.
processPrompt()now persists improvement signals via SQLite. This check stays green even if animprovement_signalsrow is inserted, because the file is no longer the source of truth.Possible fix
- const signals = readJsonl<ImprovementSignalRecord>(signalLogPath); - expect(signals).toHaveLength(0); + const db = getDb(); + const row = db + .query("SELECT COUNT(*) AS count FROM improvement_signals WHERE session_id = ?") + .get("sess-int-2") as { count: number }; + expect(row.count).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/signal-detection.test.ts` around lines 175 - 184, The test currently asserts no signals by reading signals.jsonl (readJsonl/ImprovementSignalRecord/signalLogPath) but processPrompt now persists signals to SQLite; update the test to query the SQLite table (improvement_signals) after calling processPrompt and assert zero rows instead of reading the file. Locate the test "does not append signal for normal queries" and replace the readJsonl/assert with a DB query (e.g., via the project's test DB helper or a direct SELECT COUNT(*) FROM improvement_signals WHERE session_id = 'sess-int-2') and expect count === 0.cli/selftune/localdb/direct-write.ts (1)
89-105:⚠️ Potential issue | 🟠 MajorDon't silently succeed on
"normalization_run".
CanonicalRecordincludes that variant, but both dispatchers fall through and returntruewithout persisting anything. Either add an insert path or throw so callers can see the failure.Possible fix
case "execution_fact": insertExecutionFact(db, record as CanonicalExecutionFactRecord); break; + default: + throw new Error(`Unsupported canonical record kind: ${record.record_kind}`); }Also applies to: 113-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/direct-write.ts` around lines 89 - 105, The switch in writeCanonicalToDb (inside safeWrite) currently falls through for record.record_kind === "normalization_run" and returns true without persisting; update the dispatcher to handle that variant explicitly by either calling the appropriate insert function (e.g., insertNormalizationRun(db, record as CanonicalNormalizationRunRecord)) or throw a descriptive error so callers know the write failed; apply the same explicit handling to the other dispatcher block mentioned (the similar switch around lines 113-127) to avoid silent success for "normalization_run".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/local-dashboard/src/pages/SkillReport.tsx`:
- Around line 480-487: Extract the computed value instead of using the IIFE:
compute const missed = duration_stats?.missed_triggers ?? 0 near the top of the
SkillReport component (or just before the JSX return) and then use <CardTitle
className={`text-2xl font-semibold tabular-nums @[250px]/card:text-3xl ${missed
> 0 ? "text-amber-600" : ""}`}>{missed}</CardTitle> in the render; update
references to duration_stats and missed accordingly to remove the
immediately-invoked function expression.
In `@cli/selftune/evolution/evidence.ts`:
- Around line 28-33: readEvidenceTrail currently unsafely casts the loose DB
result from queryEvolutionEvidence to EvolutionEvidenceEntry[]; remove the cast
and instead ensure eval_set and validation are correctly typed by either (A)
changing queryEvolutionEvidence to parse JSON columns into EvalEntry[] and
EvolutionEvidenceValidation before returning (adjust its return type
accordingly) or (B) validate/transform the returned records inside
readEvidenceTrail using a type guard or runtime parser (e.g., check/parse
eval_set and validation fields and throw or skip invalid records) so that
readEvidenceTrail returns a properly typed EvolutionEvidenceEntry[] without
using "as". Reference functions/types: readEvidenceTrail,
queryEvolutionEvidence, EvolutionEvidenceEntry, EvalEntry,
EvolutionEvidenceValidation.
In `@cli/selftune/hooks/skill-eval.ts`:
- Around line 106-115: Update the Initialize.md hook table entry for the
PostToolUse hook to reflect that the matcher now handles both Read and Skill
events: edit the table row that references hooks/skill-eval.ts (near where
Initialize.md documents skill hooks) so the event column reads "PostToolUse
(Read/Skill)" and the description mentions tracking both skill triggers and
explicit Skill tool invocations; this aligns the docs with
settings_snippet.json's matcher "Read|Skill" and the code path in processToolUse
which checks payload.tool_name === "Skill".
In `@cli/selftune/localdb/direct-write.ts`:
- Around line 286-307: The upsert in insertSession does not repair the platform
or source_session_kind fields on conflict, leaving stubbed rows with
"unknown"/NULL values; update the ON CONFLICT DO UPDATE clause in insertSession
(and the other similar upsert around the 321-344 block) to include platform and
source_session_kind with COALESCE(sessions.platform, excluded.platform) and
COALESCE(sessions.source_session_kind, excluded.source_session_kind) so that
when a real session row is written (e.g., via writeSessionToDb) those stubbed
columns are replaced if the excluded values are non-null.
In `@cli/selftune/localdb/materialize.ts`:
- Around line 321-326: The code is using unsafe type assertions like (si as
Record<string, unknown>).query as string to access query, skill_path,
skill_scope, and source on si; instead extend the CanonicalSkillInvocationRecord
type (or create a union/ExtendedCanonicalSkillInvocationRecord) to include these
optional string fields and update the variable/type of si (or the function
signature that consumes it) so you can access si.query, si.skill_path,
si.skill_scope, and si.source without assertions; remove the ad-hoc casts and
ensure the new type marks these fields optional if they may be absent.
In `@cli/selftune/normalization.ts`:
- Around line 91-119: The recovery code currently calls openDb() to create a
fresh DB connection (openDb) which bypasses the shared handle used elsewhere
(getDb()/writeCanonicalToDb and _setTestDb); change the logic to use getDb() to
obtain the shared Database instance (the same one used by
writeCanonicalToDb/_setTestDb) instead of openDb(), remove the redundant
db.close() on that shared handle, and adapt the typing and query calls to use
the getDb() return type so prompt recovery (the SELECT queries and assignments
to recovered.next_prompt_index, recovered.last_prompt_id,
recovered.last_actionable_prompt_id) operate on the same DB the process is
mutating.
In `@cli/selftune/utils/transcript.ts`:
- Around line 70-73: The assignment to model uses unchecked casts on JSON-parsed
objects (msg.model as string / entry.model as string); replace these with
runtime typeof guards: check if typeof msg.model === "string" and use that
value, else check typeof entry.model === "string" before assigning to the local
model variable (msg, entry, and model are the symbols to update). Ensure you
only assign when the typeof check passes so runtime non-string values are not
coerced and strict typing/unknown handling is preserved.
In `@skill/Workflows/Evolve.md`:
- Around line 96-97: The markdown lint error MD031 is caused by missing blank
line between the heading "**Call 1:**" and the opening fenced code block
(```json); fix by inserting a single blank line immediately after the "**Call
1:**" line so the code fence starts on its own paragraph, ensuring the fenced
block (```json) is separated from the preceding text.
- Line 92: Resolve the conflicting cancel behavior in the Evolve workflow by
making the AskUserQuestion rule consistent: update the step that uses the
AskUserQuestion tool to treat an explicit "cancel" or user cancellation as a
hard stop (abort the workflow) and treat the shorthand "use defaults" (or
equivalent) as the signal to continue to step 1 using recommended defaults;
remove or rewrite the opposing rule that treats cancel as continuing so only the
new behavior remains, and ensure the documentation text around the
AskUserQuestion invocation and the step referenced at lines ~92 and ~122
reflects this single clarified rule.
In `@tests/localdb/read-queries.test.ts`:
- Around line 245-261: The test only covers well-formed JSON paths; add rows
with malformed JSON to exercise the recovery branches used by the parser-backed
read helpers (the ones exercised by querySessionTelemetry after
seedSessionTelemetry). Insert one additional seeded row per parser-backed field
(e.g., bad JSON for tool_calls, bash_commands, skills_triggered/skills_invoked)
before calling querySessionTelemetry so the test asserts that the helper
recovers (returns sensible defaults or parsed fallbacks) for each malformed
blob; update the expectations accordingly for the bad-row results while keeping
the existing happy-path assertions.
In `@tests/localdb/write.test.ts`:
- Around line 113-124: Extract the repeated beforeEach/afterEach DB setup into a
shared helper (e.g., withTestDb) and replace duplicated blocks in tests that
call openDb and _setTestDb; specifically, factor the memory DB creation
(openDb(":memory:")), the _setTestDb(db) call, and cleanup (_setTestDb(null);
db.close()) into that helper and invoke it from each describe/spec instead of
repeating the beforeEach/afterEach code paths (target the openDb and _setTestDb
usages in your test suites).
---
Outside diff comments:
In `@cli/selftune/hooks/session-stop.ts`:
- Around line 46-71: The current locking logic (openSync, writeFileSync,
readFileSync, closeSync around lockPath and LOCK_STALE_MS) overwrites a stale
lock in-place, creating a TOCTOU race; change it to remove the stale file and
retry the atomic creation instead: when you detect EEXIST and determine the lock
is stale (using the parsed lock.timestamp and LOCK_STALE_MS), call
unlinkSync(lockPath) (or equivalent) and then re-attempt openSync(lockPath,
"wx") to claim the lock; ensure you closeSync(fd) on success, handle and
propagate any errors from unlink/open appropriately, and bound the retry (single
retry or small loop) to avoid livelock.
---
Duplicate comments:
In `@cli/selftune/localdb/direct-write.ts`:
- Around line 89-105: The switch in writeCanonicalToDb (inside safeWrite)
currently falls through for record.record_kind === "normalization_run" and
returns true without persisting; update the dispatcher to handle that variant
explicitly by either calling the appropriate insert function (e.g.,
insertNormalizationRun(db, record as CanonicalNormalizationRunRecord)) or throw
a descriptive error so callers know the write failed; apply the same explicit
handling to the other dispatcher block mentioned (the similar switch around
lines 113-127) to avoid silent success for "normalization_run".
In `@cli/selftune/localdb/schema.ts`:
- Around line 10-73: The SQL schema constants (CREATE_SESSIONS, CREATE_PROMPTS,
CREATE_SKILL_INVOCATIONS, CREATE_EXECUTION_FACTS) are missing columns for
several canonical fields so writeCanonicalToDb() silently drops data; update
these CREATE_* statements to add the missing columns: in CREATE_SESSIONS add
agent_id, provider, end_reason; in CREATE_PROMPTS add prompt_hash,
parent_prompt_id, source_message_id; in CREATE_SKILL_INVOCATIONS add
skill_version_hash and tool_call_id; in CREATE_EXECUTION_FACTS add
bash_commands_redacted and end_reason (choose appropriate TEXT/INTEGER/JSON
types), and ensure any new FK or NOT NULL constraints mirror how
writeCanonicalToDb() writes those fields so persisted data is not discarded.
In `@cli/selftune/normalization.ts`:
- Around line 124-125: The fallback to canonical JSONL is now broken because
helpers stopped writing CANONICAL_LOG and ignore logPath; either fully remove
the fallback usage in derivePromptSessionStateFromCanonicalLog and related code
paths (delete references to CANONICAL_LOG/_canonicalLogPath/canonicalLogPath) or
restore the JSONL fallback by reintroducing appendJsonl to the imports and
update the writers (the functions that previously emitted canonical JSONL) to
honor logPath and write CANONICAL_LOG again so recovery can rebuild from
up-to-date data; pick one approach and make sure all references
(derivePromptSessionStateFromCanonicalLog, CANONICAL_LOG, _canonicalLogPath,
canonicalLogPath) are consistently updated.
In `@tests/hooks/prompt-log.test.ts`:
- Around line 163-168: Replace the CommonJS require with an ESM dynamic import
since the test is async: use await import("node:fs") to get readFileSync (or
better, use the fs/promises API) and then parse the file; update the assertion
block that references promptStatePath and state to use the imported readFileSync
(or await fs.readFile if switching to promises) so the test remains
ESM-consistent.
In `@tests/hooks/signal-detection.test.ts`:
- Around line 175-184: The test currently asserts no signals by reading
signals.jsonl (readJsonl/ImprovementSignalRecord/signalLogPath) but
processPrompt now persists signals to SQLite; update the test to query the
SQLite table (improvement_signals) after calling processPrompt and assert zero
rows instead of reading the file. Locate the test "does not append signal for
normal queries" and replace the readJsonl/assert with a DB query (e.g., via the
project's test DB helper or a direct SELECT COUNT(*) FROM improvement_signals
WHERE session_id = 'sess-int-2') and expect count === 0.
In `@tests/localdb/localdb.test.ts`:
- Around line 54-63: The tests named "creates queries table with expected
columns" and "creates improvement_signals table with expected columns" only
check that SELECT works (zero rows) but not the actual schema; update each test
to query the schema (e.g., use db.query("PRAGMA table_info('queries')").all() or
db.query("SELECT sql FROM sqlite_master WHERE name='queries'").all()) and assert
the returned columns match the expected column names/types (compare against an
expectedColumns array) instead of only asserting row count; do the same for
improvement_signals and use the existing db.query helper to locate these checks.
In `@tests/localdb/read-queries.test.ts`:
- Around line 568-578: The assertions use non-null assertions (research! and
browser!) after calling list.find and already checking toBeDefined(); remove the
`!` and use optional chaining (e.g., research?.total_checks,
research?.triggered_count, research?.pass_rate, research?.has_evidence and
browser?.total_checks, browser?.has_evidence) in those expect(...) calls so the
tests rely on the prior toBeDefined() checks and follow the project guideline
against `!` in tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e235a5a0-e6b0-4cd0-8f95-715c8fcb768e
📒 Files selected for processing (35)
ARCHITECTURE.mdCLAUDE.mdapps/local-dashboard/src/pages/SkillReport.tsxcli/selftune/dashboard-contract.tscli/selftune/evolution/evidence.tscli/selftune/hooks/session-stop.tscli/selftune/hooks/skill-eval.tscli/selftune/ingestors/claude-replay.tscli/selftune/localdb/db.tscli/selftune/localdb/direct-write.tscli/selftune/localdb/materialize.tscli/selftune/localdb/queries.tscli/selftune/localdb/schema.tscli/selftune/monitoring/watch.tscli/selftune/normalization.tscli/selftune/routes/skill-report.tscli/selftune/sync.tscli/selftune/utils/jsonl.tscli/selftune/utils/transcript.tsdocs/design-docs/sqlite-first-migration.mdskill/Workflows/Evals.mdskill/Workflows/Evolve.mdskill/Workflows/EvolveBody.mdskill/references/logs.mdtests/blog-proof/seo-audit-evolve.test.tstests/evolution/evolve-body.test.tstests/evolution/evolve.test.tstests/hooks/prompt-log.test.tstests/hooks/signal-detection.test.tstests/hooks/skill-eval.test.tstests/localdb/localdb.test.tstests/localdb/read-queries.test.tstests/localdb/write.test.tstests/monitoring/integration.test.tstests/monitoring/watch.test.ts
cli/selftune/evolution/evidence.ts
Outdated
Comment on lines
+28
to
+33
| export function readEvidenceTrail( | ||
| skillName?: string, | ||
| logPath: string = EVOLUTION_EVIDENCE_LOG, | ||
| _logPath?: string, | ||
| ): EvolutionEvidenceEntry[] { | ||
| const entries = readJsonl<EvolutionEvidenceEntry>(logPath); | ||
| if (!skillName) return entries; | ||
| return entries.filter((entry) => entry.skill_name === skillName); | ||
| const db = getDb(); | ||
| return queryEvolutionEvidence(db, skillName) as EvolutionEvidenceEntry[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the return type shape from queryEvolutionEvidence matches EvolutionEvidenceEntry
ast-grep --pattern $'export function queryEvolutionEvidence($_, $_): $_ {
$$$
}'Repository: selftune-dev/selftune
Length of output: 2438
🏁 Script executed:
#!/bin/bash
# Find EvolutionEvidenceEntry interface definition
rg -t ts "interface EvolutionEvidenceEntry|type EvolutionEvidenceEntry" -A 15Repository: selftune-dev/selftune
Length of output: 1847
🏁 Script executed:
#!/bin/bash
# Check EvolutionTarget, EvalEntry, and EvolutionEvidenceValidation definitions
rg -t ts "type EvolutionTarget|interface EvolutionTarget|interface EvalEntry|type EvalEntry|interface EvolutionEvidenceValidation|type EvolutionEvidenceValidation" -A 5Repository: selftune-dev/selftune
Length of output: 950
Remove the unsafe cast and properly type JSON fields in queryEvolutionEvidence.
The cast as EvolutionEvidenceEntry[] masks type incompatibility: queryEvolutionEvidence returns eval_set and validation as Record<string, unknown> (loose), but EvolutionEvidenceEntry expects them as EvalEntry[] and EvolutionEvidenceValidation (typed). Either strengthen the return type in the query function with proper JSON parsing, or add type guards in readEvidenceTrail before casting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/evolution/evidence.ts` around lines 28 - 33, readEvidenceTrail
currently unsafely casts the loose DB result from queryEvolutionEvidence to
EvolutionEvidenceEntry[]; remove the cast and instead ensure eval_set and
validation are correctly typed by either (A) changing queryEvolutionEvidence to
parse JSON columns into EvalEntry[] and EvolutionEvidenceValidation before
returning (adjust its return type accordingly) or (B) validate/transform the
returned records inside readEvidenceTrail using a type guard or runtime parser
(e.g., check/parse eval_set and validation fields and throw or skip invalid
records) so that readEvidenceTrail returns a properly typed
EvolutionEvidenceEntry[] without using "as". Reference functions/types:
readEvidenceTrail, queryEvolutionEvidence, EvolutionEvidenceEntry, EvalEntry,
EvolutionEvidenceValidation.
Comment on lines
+321
to
326
| si.agent_type ?? null, | ||
| (si as Record<string, unknown>).query as string ?? null, | ||
| (si as Record<string, unknown>).skill_path as string ?? null, | ||
| (si as Record<string, unknown>).skill_scope as string ?? null, | ||
| (si as Record<string, unknown>).source as string ?? null, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertions bypass type safety.
The (si as Record<string, unknown>).query as string pattern indicates these fields exist on the runtime object but not on CanonicalSkillInvocationRecord. Consider extending the type or using a union type for records that carry these extra fields.
- (si as Record<string, unknown>).query as string ?? null,
- (si as Record<string, unknown>).skill_path as string ?? null,
- (si as Record<string, unknown>).skill_scope as string ?? null,
- (si as Record<string, unknown>).source as string ?? null,
+ // TODO: Extend CanonicalSkillInvocationRecord or use a union type
+ (si as { query?: string }).query ?? null,
+ (si as { skill_path?: string }).skill_path ?? null,
+ (si as { skill_scope?: string }).skill_scope ?? null,
+ (si as { source?: string }).source ?? null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| si.agent_type ?? null, | |
| (si as Record<string, unknown>).query as string ?? null, | |
| (si as Record<string, unknown>).skill_path as string ?? null, | |
| (si as Record<string, unknown>).skill_scope as string ?? null, | |
| (si as Record<string, unknown>).source as string ?? null, | |
| ); | |
| si.agent_type ?? null, | |
| // TODO: Extend CanonicalSkillInvocationRecord or use a union type | |
| (si as { query?: string }).query ?? null, | |
| (si as { skill_path?: string }).skill_path ?? null, | |
| (si as { skill_scope?: string }).skill_scope ?? null, | |
| (si as { source?: string }).source ?? null, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/materialize.ts` around lines 321 - 326, The code is
using unsafe type assertions like (si as Record<string, unknown>).query as
string to access query, skill_path, skill_scope, and source on si; instead
extend the CanonicalSkillInvocationRecord type (or create a
union/ExtendedCanonicalSkillInvocationRecord) to include these optional string
fields and update the variable/type of si (or the function signature that
consumes it) so you can access si.query, si.skill_path, si.skill_scope, and
si.source without assertions; remove the ad-hoc casts and ensure the new type
marks these fields optional if they may be absent.
Comment on lines
+245
to
+261
| it("returns correct shape with parsed JSON fields", () => { | ||
| seedSessionTelemetry(db); | ||
|
|
||
| const results = querySessionTelemetry(db); | ||
| expect(results).toHaveLength(1); | ||
|
|
||
| const r = results[0]; | ||
| expect(r.session_id).toBe("sess-001"); | ||
| expect(r.tool_calls).toEqual({ Read: 3, Bash: 2 }); | ||
| expect(r.bash_commands).toEqual(["git status"]); | ||
| expect(r.skills_triggered).toEqual(["Research"]); | ||
| expect(r.skills_invoked).toEqual(["Research"]); | ||
| expect(r.total_tool_calls).toBe(5); | ||
| expect(r.assistant_turns).toBe(4); | ||
| expect(r.input_tokens).toBe(1000); | ||
| expect(r.output_tokens).toBe(500); | ||
| }); |
There was a problem hiding this comment.
Exercise the malformed-JSON branches too.
These cases only seed valid JSON, but the read helpers here deliberately recover from malformed blobs. Add one bad row for each parser-backed query so historical corrupt data stays on a tested path.
As per coding guidelines: "tests/**/*.ts": "Coverage of error paths, not just happy paths"
Also applies to: 352-361, 379-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/localdb/read-queries.test.ts` around lines 245 - 261, The test only
covers well-formed JSON paths; add rows with malformed JSON to exercise the
recovery branches used by the parser-backed read helpers (the ones exercised by
querySessionTelemetry after seedSessionTelemetry). Insert one additional seeded
row per parser-backed field (e.g., bad JSON for tool_calls, bash_commands,
skills_triggered/skills_invoked) before calling querySessionTelemetry so the
test asserts that the helper recovers (returns sensible defaults or parsed
fallbacks) for each malformed blob; update the expectations accordingly for the
bad-row results while keeping the existing happy-path assertions.
Comment on lines
+113
to
+124
| describe("writeCanonicalToDb", () => { | ||
| let db: ReturnType<typeof openDb>; | ||
|
|
||
| beforeEach(() => { | ||
| db = openDb(":memory:"); | ||
| _setTestDb(db); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| _setTestDb(null); | ||
| db.close(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting repeated DB setup/teardown to a shared helper.
Each describe block repeats the same beforeEach/afterEach pattern. For a 792-line test file, consider a shared test utility:
function withTestDb(fn: (db: Database) => void) {
const db = openDb(":memory:");
_setTestDb(db);
try { fn(db); } finally { _setTestDb(null); db.close(); }
}Not blocking — the current structure is correct and works.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/localdb/write.test.ts` around lines 113 - 124, Extract the repeated
beforeEach/afterEach DB setup into a shared helper (e.g., withTestDb) and
replace duplicated blocks in tests that call openDb and _setTestDb;
specifically, factor the memory DB creation (openDb(":memory:")), the
_setTestDb(db) call, and cleanup (_setTestDb(null); db.close()) into that helper
and invoke it from each describe/spec instead of repeating the
beforeEach/afterEach code paths (target the openDb and _setTestDb usages in your
test suites).
- Auto-fix 58 biome formatting errors across 50+ files - Add occurred_at, query, source, agent_type to CanonicalInvocation type - Fix SkillReport.tsx timestamp nullability (string | undefined → string | null) - Scope bun test to tests/ directory (excludes vitest dashboard tests) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- direct-write.ts: session stub upsert now repairs platform + source_session_kind - normalization.ts: use getDb() singleton for prompt recovery (not openDb()) - transcript.ts: typeof guards for model extraction instead of unsafe casts - SkillReport.tsx: extract IIFE to const for missed triggers - Evolve.md: fix conflicting cancel behavior + MD031 code fence spacing - Initialize.md: update hook table to reflect Read/Skill event handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ci.yml: run `bun test tests/` instead of `bun test` (excludes vitest dashboard tests) - packages/ui: add @types/react + @types/react-dom devDeps (fixes dashboard typecheck) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…act key The lockfile had both file: and workspace: entries for the same package, causing bun install to fail in CI with "Duplicate package path" error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test was hitting the real ~/.selftune/selftune.db via getDb() singleton, causing lock contention with parallel test processes in CI. Inject :memory: DB via _setTestDb() for test isolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
cli/selftune/hooks/evolution-guard.ts (1)
70-75:⚠️ Potential issue | 🟠 MajorLatest-action selection is reversed on SQLite-backed reads.
queryEvolutionAudit(... )returns DESC, soskillEntries[skillEntries.length - 1]selects the oldest action and can block/allow incorrectly.🔧 Proposed fix
- const lastEntry = skillEntries[skillEntries.length - 1]; - return lastEntry.action === "deployed"; + const latestEntry = skillEntries.reduce((latest, current) => { + const latestTs = new Date((latest as { timestamp?: string }).timestamp ?? 0).getTime(); + const currentTs = new Date((current as { timestamp?: string }).timestamp ?? 0).getTime(); + return currentTs > latestTs ? current : latest; + }); + return latestEntry.action === "deployed";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/evolution-guard.ts` around lines 70 - 75, queryEvolutionAudit returns results in DESC order, so the current logic that picks lastEntry via skillEntries[skillEntries.length - 1] is actually selecting the oldest action; update the check to inspect the most recent entry instead (e.g., use skillEntries[0] as lastEntry or explicitly sort skillEntries by timestamp descending before selecting). Modify the code that sets lastEntry and the final return (the variable referenced is lastEntry and the array is skillEntries) so the function correctly returns whether the newest action === "deployed".cli/selftune/localdb/queries.ts (1)
584-589:⚠️ Potential issue | 🟠 MajorValidate parsed JSON shape before returning
Record<string, unknown>.
safeParseJsoncurrently returns any JSON value, including arrays/primitives, while the function contract promises an object. This can corrupt downstream typed fields (e.g.,tool_calls,validation).🔧 Proposed fix
export function safeParseJson(json: string | null): Record<string, unknown> | null { if (!json) return null; try { - return JSON.parse(json); + const parsed: unknown = JSON.parse(json); + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + return parsed as Record<string, unknown>; + } + return null; } catch { return null; } }As per coding guidelines,
cli/**/*.tsshould keep strict typing and use type guards for unknown-shaped data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/queries.ts` around lines 584 - 589, safeParseJson currently may return non-object JSON (arrays/primitives) contrary to its Record<string, unknown> contract; update safeParseJson to parse JSON as before but then validate the parsed value is a non-null object and not an array (e.g., typeof parsed === "object" && parsed !== null && !Array.isArray(parsed") ) before returning it, otherwise return null; keep the try/catch behavior and ensure the function signature remains safeParseJson(json: string | null): Record<string, unknown> | null so downstream callers (like handling tool_calls and validation) only receive object-shaped values.cli/selftune/ingestors/claude-replay.ts (1)
157-230:⚠️ Potential issue | 🔴 CriticalDo not ignore DB write results during replay ingest.
Lines 166, 190, and 215 call fail-open writers but ignore their boolean results. Failed writes can still lead to transcript marker advancement later, which silently drops replay data.
🔧 Suggested fix
-export function writeSession( +export function writeSession( session: ParsedSession, dryRun = false, queryLogPath: string = QUERY_LOG, telemetryLogPath: string = TELEMETRY_LOG, skillLogPath: string = SKILL_LOG, canonicalLogPath: string = CANONICAL_LOG, -): void { +): boolean { + let ok = true; if (dryRun) { ... - return; + return true; } ... - writeQueryToDb(queryRecord); + ok = writeQueryToDb(queryRecord) && ok; ... - writeSessionTelemetryToDb(telemetry); + ok = writeSessionTelemetryToDb(telemetry) && ok; ... - writeSkillCheckToDb({ + ok = + writeSkillCheckToDb({ ... - }); + }) && ok; ... - appendCanonicalRecords(canonicalRecords, canonicalLogPath); + if (ok) appendCanonicalRecords(canonicalRecords, canonicalLogPath); + return ok; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/ingestors/claude-replay.ts` around lines 157 - 230, The DB write calls writeQueryToDb, writeSessionTelemetryToDb, and writeSkillCheckToDb are currently called inside empty catch blocks and their boolean results are ignored, which can hide failures during replay ingest; update the code to capture each function's return value (or await it if it returns a Promise), check for a falsy/false result and handle it by throwing or returning an error (or logging and aborting replay) so the caller can stop advancing transcript markers on failure; specifically change the blocks around writeQueryToDb(queryRecord), writeSessionTelemetryToDb(telemetry), and writeSkillCheckToDb({...}) to validate the returned boolean and propagate failure instead of silently swallowing exceptions (use deriveSkillInvocationId, isActionableQueryText, deriveInvocationMode as needed to locate the surrounding logic).cli/selftune/localdb/materialize.ts (1)
129-206:⚠️ Potential issue | 🔴 CriticalPersist EOF offsets during full reads, or next incremental run re-ingests entire logs.
Line 131 and similar
!sincebranches read full files but never append tonewOffsets. After a full rebuild, the next incremental run starts at offset0for every file and reprocesses all historical records.🔧 Suggested fix
+import { existsSync, statSync } from "node:fs"; ... +function fileSizeOrZero(path: string): number { + return existsSync(path) ? statSync(path).size : 0; +} ... if (!since) { filteredCanonical = readCanonicalRecords(canonicalPath); + newOffsets.push([canonicalPath, fileSizeOrZero(canonicalPath)]); } else { const { records, newOffset } = readJsonlFrom<CanonicalRecord>( canonicalPath, getOffset(canonicalPath), ); filteredCanonical = records.filter(isCanonicalRecord); newOffsets.push([canonicalPath, newOffset]); } ... if (!since) { filteredTelemetry = readJsonl<SessionTelemetryRecord>(telemetryPath); + newOffsets.push([telemetryPath, fileSizeOrZero(telemetryPath)]); } else { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/materialize.ts` around lines 129 - 206, The full-read branches (when !since) read entire files but never record EOF offsets, so subsequent incremental runs reprocess everything; after each full read (for canonicalPath, telemetryPath, auditPath, evidencePath, orchestratePath) call getOffset(path) and push the resulting offset into newOffsets (e.g., newOffsets.push([canonicalPath, getOffset(canonicalPath)]) ) so EOF offsets are persisted; update the same pattern wherever the code uses readJsonl(...) in the !since branch and ensure you reference newOffsets, canonicalPath, telemetryPath, auditPath, evidencePath and orchestratePath.cli/selftune/activation-rules.ts (1)
114-131:⚠️ Potential issue | 🟠 MajorOrdering mismatch: DB returns DESC, but code accesses last element.
queryEvolutionAuditreturns rowsORDER BY timestamp DESC(newest first), but line 129 accessesauditEntries[auditEntries.length - 1]— the oldest entry. For staleness detection, you need the newest entry (index 0).The JSONL fallback reads chronologically (oldest first), so
[length - 1]was correct there. The DB path breaks this assumption.Proposed fix
- const lastEntry = auditEntries[auditEntries.length - 1]; + // DB returns DESC (newest first), JSONL is chronological (oldest first) + const lastEntry = ctx.evolution_audit_log_path === EVOLUTION_AUDIT_LOG + ? auditEntries[0] + : auditEntries[auditEntries.length - 1];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/activation-rules.ts` around lines 114 - 131, The code assumes the newest audit entry is at auditEntries[auditEntries.length - 1], but queryEvolutionAudit() returns rows ORDER BY timestamp DESC (newest first) while readJsonl() returns oldest-first; update the logic around selecting lastEntry so it picks auditEntries[0] when using the DB branch (ctx.evolution_audit_log_path === EVOLUTION_AUDIT_LOG) and auditEntries[auditEntries.length - 1] for the JSONL branch, or normalize both branches by computing the newest entry via Math.max timestamp to ensure lastTimestamp is always the newest; adjust references to auditEntries and lastEntry accordingly (functions/vars: queryEvolutionAudit, readJsonl, auditEntries, lastEntry, lastTimestamp).cli/selftune/evolution/evolve.ts (1)
1019-1031:⚠️ Potential issue | 🟡 MinorPre-flight validation mixes DB and JSONL checks inconsistently.
Line 1021 checks DB for skill records (
querySkillUsageRecords(db).length > 0), but line 1022 checks JSONL file existence (existsSync(QUERY_LOG)). Since the actual eval-set construction at line 330 reads queries from the DB, this validation should also check the DB:- const hasQueryLog = existsSync(QUERY_LOG); + const hasQueryLog = queryQueryLog(dbCheck).length > 0;Otherwise, pre-flight passes when JSONL exists but DB is empty, then eval-set construction returns an empty array silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/evolution/evolve.ts` around lines 1019 - 1031, The pre-flight check mixes DB and JSONL validations causing false positives; instead of using existsSync(QUERY_LOG) check the DB for stored query records. In the block gated by !evalSetPath && !(values["sync-first"] ?? false), use getDb() and the DB query function (similar to querySkillUsageRecords) to verify there are query usage records (the same source used when building the eval set at line 330) and only pass if either skill usage or query usage exists; remove or replace the existsSync(QUERY_LOG) check and update the error message to reflect DB-backed log locations (referencing getDb(), querySkillUsageRecords, QUERY_LOG, SKILL_LOG, and evalSetPath).cli/selftune/hooks/skill-eval.ts (1)
132-145:⚠️ Potential issue | 🟠 MajorRead and Skill events now collide on the same invocation record.
processSkillToolUse()and the SKILL.md Read path both deriveskill_invocation_idfromcountSkillToolInvocations(...)-1. A normalSkill(foo)→Read(foo/SKILL.md)flow therefore produces two different records with the same ID, so the later write can overwrite the explicit invocation or create duplicate canonical entries with conflictinginvocation_mode/agent_type. Skip the Read-path write once a matching Skill event exists, or give contextual reads a separate identifier/record kind.Also applies to: 171-175, 283-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/skill-eval.ts` around lines 132 - 145, The Read-path is colliding with actual Skill tool invocations because both derive skill_invocation_id from countSkillToolInvocations(...)-1; update the Read-path logic (the block that constructs SkillUsageRecord using classifySkillPath, query, triggered, invocation_type "contextual") to first check for an existing invocation produced by processSkillToolUse by calling countSkillToolInvocations(transcriptPath, skillName) and, if invocationCount>0, skip writing the Read-path record (or alternatively assign a distinct identifier/record kind for contextual reads); ensure the same check is applied to the other affected sections noted (around the blocks at 171-175 and 283-324) so Read events do not overwrite or duplicate records created by processSkillToolUse.cli/selftune/evolution/rollback.ts (1)
23-28:⚠️ Potential issue | 🟠 Major
logPathis no longer backward compatible if it is silently ignored.The public
RollbackOptionstype still advertiseslogPath, but the implementation drops it everywhere. Existing callers that pass an isolated audit log now read/write the default DB instead, which can pick the wrong proposal or miss rollback history entirely. Either keep threadinglogPaththrough until the next breaking change, or reject it explicitly so callers know they need to migrate.Also applies to: 102-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/evolution/rollback.ts` around lines 23 - 28, The RollbackOptions type still exposes logPath but the implementation silently ignores it, causing callers that pass a custom audit log to operate on the default DB; update the rollback code to either fully thread logPath through all DB/open calls or explicitly reject/raise when logPath is provided. Concretely, keep RollbackOptions.logPath and either (A) propagate that value into every function that opens or reads the rollout/audit DB (where the rollback logic reads proposals) so custom logs are honored, or (B) add an early validation in the public rollback entrypoint that checks options.logPath and throws a clear error saying custom log paths are unsupported and require migration—choose one approach and apply consistently to all code paths that read/write rollback history (ensure functions that open the DB accept a logPath parameter or check it and fail).
♻️ Duplicate comments (16)
tests/hooks/signal-detection.test.ts (1)
188-198:⚠️ Potential issue | 🟠 MajorNegative test verifies a file that's no longer written to.
processPromptwrites signals to SQLite viawriteImprovementSignalToDb, not tosignalLogPath. This test passes vacuously becausesignals.jsonlis never created. Query the SQLite database instead to confirm no signal row exists.
,🐛 Proposed fix
test("does not append signal for normal queries", async () => { const payload: PromptSubmitPayload = { user_prompt: "help me refactor this module", session_id: "sess-int-2", }; await processPrompt(payload, logPath, canonicalLogPath, promptStatePath, signalLogPath); - const signals = readJsonl<ImprovementSignalRecord>(signalLogPath); - expect(signals).toHaveLength(0); + // Verify no signal was written to SQLite + const db = getDb(); + const row = db + .query("SELECT COUNT(*) as count FROM improvement_signals WHERE session_id = ?") + .get("sess-int-2") as { count: number } | null; + expect(row?.count ?? 0).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/signal-detection.test.ts` around lines 188 - 198, The test currently asserts on signals.jsonl (readJsonl) which is no longer written; instead change the assertion to query the same SQLite table that writeImprovementSignalToDb writes to and assert there are no rows for the test session_id. In the "does not append signal for normal queries" test (which calls processPrompt), remove the readJsonl<ImprovementSignalRecord>(signalLogPath) usage and replace it with a direct SQLite query against the improvement_signals table (or use the project's DB helper to open the DB used by writeImprovementSignalToDb) filtering by session_id "sess-int-2", then expect the returned rows length to be 0. Ensure you reference the same DB connection/utility used by writeImprovementSignalToDb so the test checks the real storage.cli/selftune/routes/orchestrate-runs.ts (1)
10-12:⚠️ Potential issue | 🟠 MajorReturn a controlled 503 when the query fails.
getOrchestrateRuns()can throw on DB/open/schema errors, and this handler currently lets that escape instead of returning JSON the dashboard can handle. Wrap the call intry/catchand return a 503 payload on failure.Suggested fix
export function handleOrchestrateRuns(db: Database, limit: number): Response { - const runs = getOrchestrateRuns(db, limit); - return Response.json({ runs }); + try { + const runs = getOrchestrateRuns(db, limit); + return Response.json({ runs }); + } catch { + return Response.json({ error: "Failed to load orchestrate runs" }, { status: 503 }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/routes/orchestrate-runs.ts` around lines 10 - 12, handleOrchestrateRuns currently lets getOrchestrateRuns throw; wrap the call to getOrchestrateRuns(db, limit) in a try/catch inside handleOrchestrateRuns, and on success return Response.json({ runs }) as before, but on failure return a Response.json({ error: "Failed to fetch orchestrate runs" }, { status: 503 }) so the dashboard receives a controlled 503 payload; optionally log the caught error before returning.cli/selftune/auto-update.ts (2)
71-75:⚠️ Potential issue | 🟠 MajorBack off after a failed cached update attempt.
When the cache says
latestVersionis newer, this branch rerunsperformUpdate()on every invocation until the cache expires. One permission/path failure will therefore stall every command behind the same 30s synchronous install attempt. Persist a failed-attempt timestamp/version and skip retries until the next refresh window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/auto-update.ts` around lines 71 - 75, The cached-update branch calls performUpdate repeatedly on every invocation until cache expiry; change it to persist a failed-update marker and back off: when cache.latestVersion > currentVersion, first check if cache.failedUpdateVersion === cache.latestVersion and Date.now() - (cache.failedAttemptAt || 0) < CHECK_INTERVAL_MS — if so, skip performUpdate; otherwise call performUpdate inside a try/catch, and on failure set cache.failedUpdateVersion = cache.latestVersion and cache.failedAttemptAt = Date.now(), while on success clear those fields; update the cache persistence logic where the cache object is saved so these new fields survive between invocations.
123-125:⚠️ Potential issue | 🟡 MinorDon't point this failure path at a raw npm command.
This message tells the human what shell command to run instead of giving the agent a CLI-oriented next step. Replace it with an agent-facing retry/recovery message and keep the package-manager detail out of the user-facing path.
Suggested fix
console.error( - `[selftune] Auto-update failed. Run manually: npm install -g selftune@${latestVersion}`, + "[selftune] Auto-update failed. Retry by rerunning selftune after fixing the install environment.", );As per coding guidelines,
cli/selftune/**/*.ts: Error messages should guide the agent, not the human (e.g., suggest the next CLI command, not 'check the docs').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/auto-update.ts` around lines 123 - 125, Replace the human-facing npm suggestion with an agent-facing retry/recovery message: in the block that currently calls console.error(`[selftune] Auto-update failed. Run manually: npm install -g selftune@${latestVersion}`) (referencing latestVersion), remove the raw npm command and instead log a concise agent-oriented instruction such as prompting the CLI's update flow or a retry action (e.g., suggest running the tool's built-in update command or retry flag) and include the failed version info if helpful; ensure the new message guides the agent to retry or invoke the CLI update workflow rather than showing a package-manager command.cli/selftune/quickstart.ts (1)
102-115:⚠️ Potential issue | 🟠 MajorDon't turn DB read failures into a fake empty workspace.
Line 102 can throw before the fallback, and both catch blocks replace read errors with
[], so a corrupt or unreadable DB gets reported as "no sessions" / missing audit history. Stop here and surface a recovery command likeselftune rebuild-dborselftune ingest claudeinstead of continuing with synthetic emptiness.As per coding guidelines,
cli/**/*.ts: Actionable error messages, not raw stack traces.Also applies to: 136-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/quickstart.ts` around lines 102 - 115, The current try/catch around getDb() and the three query* calls swallows DB read failures and returns empty arrays; change it so DB or query errors are surfaced with an actionable error and exit instead of returning synthetic []: call getDb() and each of querySessionTelemetry/querySkillUsageRecords/queryQueryLog in guarded blocks (or let their errors propagate), and on any error log a clear, user-facing message (no raw stack) that includes recovery commands like "selftune rebuild-db" or "selftune ingest claude" and then throw or process.exit(1); update the code paths that previously relied on empty arrays to assume these functions either succeed or the process has terminated.cli/selftune/evolution/evidence.ts (1)
28-30:⚠️ Potential issue | 🟠 MajorRemove the unsafe cast from DB rows to
EvolutionEvidenceEntry[].
queryEvolutionEvidencereturns loose shapes; casting directly can leak invalidstage/target/eval_set/validationvalues into typed consumers.🔧 Proposed fix
export function readEvidenceTrail(skillName?: string, _logPath?: string): EvolutionEvidenceEntry[] { const db = getDb(); - return queryEvolutionEvidence(db, skillName) as EvolutionEvidenceEntry[]; + const rows = queryEvolutionEvidence(db, skillName); + return rows.flatMap((row) => { + // map/validate row into EvolutionEvidenceEntry (or drop invalid rows) + // e.g. validate stage/target enums and structured JSON fields here. + return [row as EvolutionEvidenceEntry]; + }); }As per coding guidelines,
cli/**/*.tsshould preserve strict typing via validation/guards instead of unchecked casts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/evolution/evidence.ts` around lines 28 - 30, The current readEvidenceTrail function unsafely casts DB rows from queryEvolutionEvidence to EvolutionEvidenceEntry[]; replace the unchecked cast by validating/mapping each row returned by queryEvolutionEvidence (obtained via getDb()) into a properly typed EvolutionEvidenceEntry: implement a small guard/validator that checks required fields (stage, target, eval_set, validation, etc.) and transforms or filters out invalid rows, then return the validated array; reference the readEvidenceTrail and queryEvolutionEvidence functions and the EvolutionEvidenceEntry type when adding the mapping/guard logic.cli/selftune/contribute/bundle.ts (1)
213-237:⚠️ Potential issue | 🟠 MajorSelect source per log, not via one global
useJsonlswitch.Any single override currently forces all collections to JSONL, which changes behavior for unaffected datasets and can drop SQLite-backed records.
🔧 Proposed fix
- const useJsonl = - queryLogPath !== QUERY_LOG || - skillLogPath !== SKILL_LOG || - telemetryLogPath !== TELEMETRY_LOG || - evolutionAuditLogPath !== EVOLUTION_AUDIT_LOG; + const useJsonlSkill = skillLogPath !== SKILL_LOG; + const useJsonlQuery = queryLogPath !== QUERY_LOG; + const useJsonlTelemetry = telemetryLogPath !== TELEMETRY_LOG; + const useJsonlAudit = evolutionAuditLogPath !== EVOLUTION_AUDIT_LOG; + const needsDb = !useJsonlSkill || !useJsonlQuery || !useJsonlTelemetry || !useJsonlAudit; let allSkillRecords: SkillUsageRecord[]; let allQueryRecords: QueryLogRecord[]; let allTelemetryRecords: SessionTelemetryRecord[]; let allEvolutionRecords: EvolutionAuditEntry[]; - if (useJsonl) { - allSkillRecords = readJsonl<SkillUsageRecord>(skillLogPath); - allQueryRecords = readJsonl<QueryLogRecord>(queryLogPath); - allTelemetryRecords = readJsonl<SessionTelemetryRecord>(telemetryLogPath); - allEvolutionRecords = readJsonl<EvolutionAuditEntry>(evolutionAuditLogPath); - } else { - const db = getDb(); - allSkillRecords = querySkillUsageRecords(db) as SkillUsageRecord[]; - allQueryRecords = queryQueryLog(db) as QueryLogRecord[]; - allTelemetryRecords = querySessionTelemetry(db) as SessionTelemetryRecord[]; - allEvolutionRecords = queryEvolutionAudit(db) as EvolutionAuditEntry[]; - } + const db = needsDb ? getDb() : undefined; + allSkillRecords = useJsonlSkill + ? readJsonl<SkillUsageRecord>(skillLogPath) + : (querySkillUsageRecords(db!) as SkillUsageRecord[]); + allQueryRecords = useJsonlQuery + ? readJsonl<QueryLogRecord>(queryLogPath) + : (queryQueryLog(db!) as QueryLogRecord[]); + allTelemetryRecords = useJsonlTelemetry + ? readJsonl<SessionTelemetryRecord>(telemetryLogPath) + : (querySessionTelemetry(db!) as SessionTelemetryRecord[]); + allEvolutionRecords = useJsonlAudit + ? readJsonl<EvolutionAuditEntry>(evolutionAuditLogPath) + : (queryEvolutionAudit(db!) as EvolutionAuditEntry[]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/contribute/bundle.ts` around lines 213 - 237, The current global useJsonl flag forces all collections to be read from JSONL whenever any single path is overridden; change this to decide per-log whether to use JSONL by comparing each path to its default (e.g., skillLogPath !== SKILL_LOG, queryLogPath !== QUERY_LOG, telemetryLogPath !== TELEMETRY_LOG, evolutionAuditLogPath !== EVOLUTION_AUDIT_LOG) and then for each collection call either readJsonl<T>(...) or the SQLite query (via getDb() + querySkillUsageRecords, queryQueryLog, querySessionTelemetry, queryEvolutionAudit) accordingly so only overridden logs use JSONL. Ensure variables/functions referenced above are used to locate the code.cli/selftune/canonical-export.ts (1)
146-153:⚠️ Potential issue | 🟠 MajorHandle evidence DB read errors explicitly before building payload.
Failures in the inline DB IIFE currently bubble into
exitWithUsage(...), which misreports runtime DB problems as usage errors.🔧 Proposed fix
+ let evidenceEntries: EvolutionEvidenceEntry[] = []; + if (values["push-payload"]) { + try { + const db = getDb(); + evidenceEntries = queryEvolutionEvidence(db) as EvolutionEvidenceEntry[]; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`[ERROR] Failed to read evolution evidence from SQLite: ${message}`); + console.error("Run `selftune doctor` and retry `selftune export-canonical --push-payload`."); + process.exit(1); + } + } + const output = values["push-payload"] ? `${JSON.stringify( - buildPushPayloadV2( - records, - (() => { - const db = getDb(); - return queryEvolutionEvidence(db) as EvolutionEvidenceEntry[]; - })(), - ), + buildPushPayloadV2(records, evidenceEntries), null, values.pretty ? 2 : undefined, )}\n`As per coding guidelines,
cli/**/*.tsshould emit actionable runtime errors, andcli/selftune/**/*.tserrors should guide the agent toward the next command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/canonical-export.ts` around lines 146 - 153, The inline IIFE that calls getDb() and queryEvolutionEvidence() inside the buildPushPayloadV2 call can throw DB/read errors which are currently being caught by exitWithUsage; replace the inline IIFE with an explicit try/catch prior to calling buildPushPayloadV2: call getDb() and queryEvolutionEvidence(db) in a separate block, catch any DB errors and emit an actionable runtime error (not exitWithUsage) that describes the DB failure and instructs the user on the next step (e.g., retry or initialize DB), then pass the retrieved EvolutionEvidenceEntry[] into buildPushPayloadV2; update references to buildPushPayloadV2, getDb, and queryEvolutionEvidence accordingly.cli/selftune/localdb/materialize.ts (1)
342-345:⚠️ Potential issue | 🟠 MajorRemove unsafe field assertions for invocation extras.
Lines 342-345 cast unknown values directly to
string, which bypasses type safety and can persist invalid runtime values. Prefer a typed extension ofCanonicalSkillInvocationRecordorunknown+ guards.🔧 Suggested fix
+function optionalStringField(obj: unknown, key: string): string | null { + if (!obj || typeof obj !== "object") return null; + const value = (obj as Record<string, unknown>)[key]; + return typeof value === "string" ? value : null; +} ... - ((si as Record<string, unknown>).query as string) ?? null, - ((si as Record<string, unknown>).skill_path as string) ?? null, - ((si as Record<string, unknown>).skill_scope as string) ?? null, - ((si as Record<string, unknown>).source as string) ?? null, + optionalStringField(si, "query"), + optionalStringField(si, "skill_path"), + optionalStringField(si, "skill_scope"), + optionalStringField(si, "source"),#!/bin/bash rg -nP '\(si as Record<string, unknown>\)\.(query|skill_path|skill_scope|source)\s+as string' cli/selftune/localdb/materialize.tsAs per coding guidelines:
cli/**/*.ts: “Strict typing (no `any`, prefer `unknown` with type guards)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/materialize.ts` around lines 342 - 345, The code is unsafely asserting fields on the invocation extras (the `si` variable) by casting unknowns to `string`; replace these casts with a proper typed guard or accessor: define a typed extension/interface for `CanonicalSkillInvocationRecord` (or a runtime type guard `isCanonicalSkillInvocationRecord(si): boolean`) that verifies `query`, `skill_path`, `skill_scope`, and `source` are strings (or null/undefined if allowed), then use that guard before reading the fields (or a helper like `getStringField(si, 'query')`) and fall back to null when validation fails; update the occurrences that reference `((si as Record<string, unknown>).query as string) ?? null` (and the `skill_path`, `skill_scope`, `source` lines) to use the guard/accessor so no direct `as string` casts remain.tests/localdb/write.test.ts (1)
116-127: 🧹 Nitpick | 🔵 TrivialRepeated DB setup/teardown is acceptable but could be extracted.
Each
describeblock repeats the samebeforeEach/afterEachpattern. A sharedwithTestDb(fn)helper would reduce boilerplate, but the current structure is correct and works. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/localdb/write.test.ts` around lines 116 - 127, Multiple describe blocks duplicate the same test DB setup/teardown using openDb, _setTestDb, and db.close; extract this into a reusable helper (e.g., withTestDb) that accepts a callback and internally performs db = openDb(":memory:"), _setTestDb(db) before running the callback and then _setTestDb(null) and db.close() afterward, and replace each describe's beforeEach/afterEach pair (including the one around writeCanonicalToDb) with calls to this helper to remove boilerplate while preserving behavior.cli/selftune/routes/actions.ts (2)
45-46:⚠️ Potential issue | 🟠 MajorGuard request body fields at runtime instead of asserting them to strings.
These casts accept truthy non-strings at runtime, so a malformed JSON body can turn into
--skill [object Object]/--proposal-id 123and either misroute the CLI or fail insideBun.spawn.🛡️ Tighten field parsing
+function getRequiredString(value: unknown): string | undefined { + return typeof value === "string" && value.trim() !== "" ? value : undefined; +} + export async function handleAction( action: string, body: Record<string, unknown>, executeAction: ActionRunner = runAction, ): Promise<Response> { if (action === "watch" || action === "evolve") { - const skill = body.skill as string | undefined; - const skillPath = body.skillPath as string | undefined; + const skill = getRequiredString(body.skill); + const skillPath = getRequiredString(body.skillPath); if (!skill || !skillPath) { return Response.json( { success: false, error: "Missing required fields: skill, skillPath" }, @@ if (action === "rollback") { - const skill = body.skill as string | undefined; - const skillPath = body.skillPath as string | undefined; - const proposalId = body.proposalId as string | undefined; + const skill = getRequiredString(body.skill); + const skillPath = getRequiredString(body.skillPath); + const proposalId = getRequiredString(body.proposalId);As per coding guidelines:
cli/**/*.ts: CLI source code. Review for: Strict typing (no any, preferunknownwith type guards).Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/routes/actions.ts` around lines 45 - 46, The code currently casts body.skill and body.skillPath to string without runtime checks, allowing non-string values to slip through; update the parsing in actions.ts to validate these fields using typeof checks (e.g., ensure typeof body.skill === "string") before assigning to skill and skillPath (and similarly guard the other fields referenced around lines 59-61), and if a field is not a string set it to undefined or reject the request with a clear error; reference the variables skill, skillPath and the incoming body object to locate where to add these guards and error handling.
47-50:⚠️ Potential issue | 🟡 MinorMake the 400 responses tell the agent what to run next.
These errors state the validation failure, but they do not suggest the next command or valid action set. Add concrete follow-ups such as
selftune statusfor missing skill data and enumeratewatch | evolve | rollbackin the unknown-action response.As per coding guidelines:
cli/selftune/**/*.ts: Error messages should guide the agent, not the human (e.g., suggest the next CLI command, not 'check the docs').Also applies to: 63-65, 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/routes/actions.ts` around lines 47 - 50, Update the error Response.json messages to guide the agent on next CLI commands: when validating inputs around the variables skill and skillPath (the Response.json call returning 400 for missing required fields), change the message to instruct the agent to run "selftune status" (e.g., "Missing required fields: skill, skillPath — run `selftune status` to see available skills and try again"). Also update the unknown-action 400 response (the branch checking the action variable) to enumerate valid actions and the next command, e.g., "Unknown action: <action>. Valid actions: watch | evolve | rollback — run `selftune <action>` or `selftune status` to list options." Apply the same messaging pattern to the other similar 400 responses referenced around the Response.json calls at the other occurrences noted (lines ~63-65 and ~73).tests/localdb/read-queries.test.ts (1)
293-309:⚠️ Potential issue | 🟡 MinorCover the malformed-JSON recovery branches too.
These assertions only cover well-formed blobs, but the read layer is explicitly fail-open on corrupt JSON. Seed one bad row for telemetry, audit/evidence JSON, and
skill_actions_jsonso the fallback/default behavior stays covered.As per coding guidelines:
tests/**/*.ts: Coverage of error paths, not just happy paths.Also applies to: 426-435, 453-461, 563-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/localdb/read-queries.test.ts` around lines 293 - 309, Add a failing-JSON branch to the test by seeding one additional row with malformed blobs for the telemetry, audit/evidence, and skill_actions_json fields (use the existing seed helper or extend seedSessionTelemetry) and then call querySessionTelemetry to verify the read layer's fail-open behavior: assert that parsing errors return the expected fallback/default shapes (e.g., empty objects/arrays or zero counts) for tool_calls, bash_commands, skills_triggered/skills_invoked, and any audit/evidence fields while still preserving other fields like session_id, total_tool_calls, assistant_turns, input_tokens, and output_tokens; update assertions to explicitly cover both the well-formed result (as already present) and the malformed-JSON result returned by querySessionTelemetry.cli/selftune/orchestrate.ts (1)
98-104:⚠️ Potential issue | 🟠 MajorPersist signal consumption back to SQLite.
readPendingSignals()now pulls fromimprovement_signals, but the consume path still only rewritesSIGNAL_LOG. That leaves the SQLite rows unconsumed, so every orchestrate run can keep reusing the same signals. Update the DB rows here as the source of truth, and keep the JSONL rewrite only if legacy compatibility still matters.Also applies to: 123-159, 868-870
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/orchestrate.ts` around lines 98 - 104, readPendingSignals currently reads from improvement_signals via queryImprovementSignals but the downstream consume logic still only rewrites SIGNAL_LOG, leaving SQLite rows unmarked and causing duplicate consumption; update the consume path so that after reading/processing each ImprovementSignalRecord (from readPendingSignals / queryImprovementSignals) you persist its consumption back to the SQLite table (either mark a consumed flag or delete the row) using the same DB client from getDb(), and optionally keep the SIGNAL_LOG JSONL rewrite only for legacy compatibility; change references in the consume flow that currently call SIGNAL_LOG-only writing so they also call the new DB update/delete operation for ImprovementSignalRecord.cli/selftune/evolution/audit.ts (1)
23-35:⚠️ Potential issue | 🟠 MajorDon't drop legacy audit rows when exact matches exist.
This only searches
detailswhen the exactskill_namequery returns zero rows. In mixed datasets, legacy rows withskill_name = NULLdisappear as soon as one normalized row exists, sogetLastDeployedProposal()can return incomplete history. Union the exact and legacy matches, then dedupe/sort the combined set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/evolution/audit.ts` around lines 23 - 35, The current readAuditTrail implementation returns legacy rows (where skill_name is null and the name appears only in details) only when the exact skill_name query yields zero results, causing mixed datasets to lose legacy entries; change readAuditTrail to always fetch both sets: call queryEvolutionAudit(db, skillName) to get exact matches and call queryEvolutionAudit(db) then filter by details.toLowerCase().includes(needle) to get legacy matches, merge the two arrays, deduplicate entries (e.g. by a stable unique key such as an id/timestamp/JSON signature on EvolutionAuditEntry) and finally sort the combined set deterministically (e.g. by timestamp descending or created_at) before returning; refer to readAuditTrail, queryEvolutionAudit, EvolutionAuditEntry and ensure the merged result preserves both normalized and legacy rows for getLastDeployedProposal() and other callers.cli/selftune/dashboard-server.ts (1)
213-225:⚠️ Potential issue | 🟠 MajorSSE invalidation is still wired to the old JSONL files.
WATCHED_LOGSonly observesTELEMETRY_LOG,QUERY_LOG, andEVOLUTION_AUDIT_LOG, but this PR moves normal dashboard updates onto SQLite/canonical writes. Updates coming fromappendCanonicalRecord()or direct DB writers never reachbroadcastSSE("update"), so/api/v2/eventsstays quiet on the new hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 213 - 225, WATCHED_LOGS and onLogFileChange only fire for the old JSONL files so updates via appendCanonicalRecord()/DB writes don't trigger refreshV2DataImmediate() or broadcastSSE("update"); update the hot-path by hooking the DB/write layer to emit the same event (or call a shared notifier). Specifically, add a call into appendCanonicalRecord (or the DB writer entrypoint) to invoke refreshV2DataImmediate() and broadcastSSE("update") (or better, emit a centralized "v2-update" event that onLogFileChange and the DB writer both subscribe to) so canonical SQLite writes produce the same SSE invalidation as the watched JSONL paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 908f0491-73d5-4905-a0d0-50edaa40e2bb
📒 Files selected for processing (58)
apps/local-dashboard/src/hooks/useSSE.tsapps/local-dashboard/src/pages/SkillReport.tsxcli/selftune/activation-rules.tscli/selftune/auto-update.tscli/selftune/badge/badge.tscli/selftune/canonical-export.tscli/selftune/contribute/bundle.tscli/selftune/dashboard-contract.tscli/selftune/dashboard-server.tscli/selftune/eval/baseline.tscli/selftune/eval/hooks-to-evals.tscli/selftune/evolution/audit.tscli/selftune/evolution/evidence.tscli/selftune/evolution/evolve-body.tscli/selftune/evolution/evolve.tscli/selftune/evolution/rollback.tscli/selftune/export.tscli/selftune/hooks/evolution-guard.tscli/selftune/hooks/prompt-log.tscli/selftune/hooks/session-stop.tscli/selftune/hooks/skill-eval.tscli/selftune/index.tscli/selftune/ingestors/claude-replay.tscli/selftune/localdb/db.tscli/selftune/localdb/direct-write.tscli/selftune/localdb/materialize.tscli/selftune/localdb/queries.tscli/selftune/monitoring/watch.tscli/selftune/normalization.tscli/selftune/orchestrate.tscli/selftune/quickstart.tscli/selftune/routes/actions.tscli/selftune/routes/badge.tscli/selftune/routes/index.tscli/selftune/routes/orchestrate-runs.tscli/selftune/routes/overview.tscli/selftune/routes/report.tscli/selftune/routes/skill-report.tscli/selftune/status.tscli/selftune/sync.tscli/selftune/utils/transcript.tspackage.jsonskill/Workflows/Evolve.mdskill/Workflows/Initialize.mdtests/autonomy-proof.test.tstests/blog-proof/seo-audit-evolve.test.tstests/evolution/audit.test.tstests/evolution/evidence.test.tstests/evolution/evolve-body.test.tstests/evolution/evolve.test.tstests/hooks/prompt-log.test.tstests/hooks/session-stop.test.tstests/hooks/signal-detection.test.tstests/hooks/skill-eval.test.tstests/ingestors/codex-rollout.test.tstests/localdb/localdb.test.tstests/localdb/read-queries.test.tstests/localdb/write.test.ts
Comment on lines
+76
to
+81
| // Read log files from SQLite | ||
| const db = getDb(); | ||
| const telemetry = querySessionTelemetry(db) as SessionTelemetryRecord[]; | ||
| const skillRecords = querySkillUsageRecords(db) as SkillUsageRecord[]; | ||
| const queryRecords = queryQueryLog(db) as QueryLogRecord[]; | ||
| const auditEntries = queryEvolutionAudit(db) as EvolutionAuditEntry[]; |
There was a problem hiding this comment.
Catch DB load failures before generating the badge.
These eager SQLite reads can fail on open/schema/query errors, and this command currently has no controlled failure path around them. Wrap the block in try/catch, print a short recovery step, and exit 1 instead of letting badge generation abort without guidance.
As per coding guidelines, cli/**/*.ts: Actionable error messages, not raw stack traces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/badge/badge.ts` around lines 76 - 81, The eager DB reads using
getDb(), querySessionTelemetry(), querySkillUsageRecords(), queryQueryLog(), and
queryEvolutionAudit() must be wrapped in a try/catch so DB open/schema/query
failures are caught; on error, log a short actionable message (no raw stack)
that suggests recovery (e.g., "Failed to read telemetry DB — ensure the CLI DB
is initialized or run `selftune init`") and call process.exit(1) to stop badge
generation; update the block that currently invokes those functions so all calls
are inside the try and the catch prints the concise, user-facing message and
exits.
Comment on lines
+471
to
+479
| if ( | ||
| skillLogPath === SKILL_LOG && | ||
| queryLogPath === QUERY_LOG && | ||
| telemetryLogPath === TELEMETRY_LOG | ||
| ) { | ||
| const db = getDb(); | ||
| skillRecords = querySkillUsageRecords(db) as SkillUsageRecord[]; | ||
| queryRecords = queryQueryLog(db) as QueryLogRecord[]; | ||
| telemetryRecords = querySessionTelemetry(db) as SessionTelemetryRecord[]; |
There was a problem hiding this comment.
Fail the DB-backed default path with a guided error.
This branch now hard-depends on SQLite, but any open/query failure bubbles out of cliMain() before the user gets a concrete recovery step. Catch the DB reads here and exit with a short message like selftune rebuild-db or explicit --skill-log/--query-log/--telemetry-log guidance instead of falling through to the generic top-level error path.
As per coding guidelines, cli/**/*.ts: Actionable error messages, not raw stack traces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/eval/hooks-to-evals.ts` around lines 471 - 479, Wrap the
DB-backed default branch (the block calling getDb(), querySkillUsageRecords,
queryQueryLog, querySessionTelemetry when SKILL_LOG/QUERY_LOG/TELEMETRY_LOG are
used) in a try/catch; on any error catch it and print a short, actionable
message (for example: "Database-backed defaults failed: run `selftune
rebuild-db` or provide --skill-log/--query-log/--telemetry-log") and exit
non‑zero instead of letting the raw error bubble to cliMain; ensure the catch
references the same symbols (getDb, querySkillUsageRecords, queryQueryLog,
querySessionTelemetry, SKILL_LOG, QUERY_LOG, TELEMETRY_LOG) and does not emit
the full stack trace.
Comment on lines
+114
to
+116
| "question": "Advanced Options", | ||
| "options": ["Defaults (0.6 confidence, 3 iterations, single candidate) (recommended)", "Stricter (0.7 confidence, 5 iterations)", "Pareto mode (multiple candidates per iteration)"] | ||
| } |
There was a problem hiding this comment.
Map the new Advanced Options presets to concrete flags.
Stricter and Pareto mode are introduced here, but the mapping table still has no stricter preset and still refers to pareto as 6b. That leaves step 0 unable to deterministically build the command it says it will build. Add explicit mappings for the stricter preset, renumber pareto to question 4, and document --pareto in the options table.
As per coding guidelines, skill/**/*.md workflow docs must have clear step-by-step instructions and no ambiguous references.
Also applies to: 127-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skill/Workflows/Evolve.md` around lines 114 - 116, The "Advanced Options"
presets are not mapped to concrete flags: add explicit mappings for the
"Stricter (0.7 confidence, 5 iterations)" preset (e.g., map to --confidence 0.7
--iterations 5) and for "Pareto mode (multiple candidates per iteration)" (map
to --pareto and document its behavior), update the mapping table entry that
currently refers to pareto as "6b" to be question 4 (renumber pareto to question
4), and update any step 0 text that constructs the CLI command so it
deterministically composes flags based on the selected preset strings ("Defaults
(...)", "Stricter (...)", "Pareto mode (...)"); ensure the options table lists
--pareto and the stricter preset flags so the workflow can build exact commands.
| | `PreToolUse` (Write/Edit) | `hooks/skill-change-guard.ts` | Detect uncontrolled skill edits | | ||
| | `PreToolUse` (Write/Edit) | `hooks/evolution-guard.ts` | Block SKILL.md edits on monitored skills | | ||
| | `PostToolUse` (Read) | `hooks/skill-eval.ts` | Track skill triggers | | ||
| | `PostToolUse` (Read/Skill) | `hooks/skill-eval.ts` | Track skill triggers and Skill tool invocations | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify hook configuration consistency for PostToolUse Read/Skill tracking
# Check if skill-eval.ts handles both Read and Skill tool types
echo "=== Checking hooks/skill-eval.ts for Read and Skill handling ==="
rg -n -A 5 -B 2 'PostToolUse|Read|Skill' cli/selftune/hooks/skill-eval.ts
# Check settings_snippet.json for the hook configuration
echo -e "\n=== Checking skill/settings_snippet.json for PostToolUse configuration ==="
cat skill/settings_snippet.json | jq '.hooks[] | select(.lifecycle == "PostToolUse")'
# Verify the hook is registered with the correct tool filter
echo -e "\n=== Checking for tool filter in settings_snippet.json ==="
rg -n 'PostToolUse.*skill-eval' skill/settings_snippet.jsonRepository: selftune-dev/selftune
Length of output: 8460
🏁 Script executed:
#!/bin/bash
# Check skill/settings_snippet.json structure and PostToolUse configuration
echo "=== File structure and size ==="
wc -l skill/settings_snippet.json
echo -e "\n=== First 100 lines to understand structure ==="
head -100 skill/settings_snippet.json
echo -e "\n=== Search for PostToolUse in the file ==="
rg -n 'PostToolUse' skill/settings_snippet.json -A 5 -B 2
echo -e "\n=== Search for skill-eval reference ==="
rg -n 'skill-eval' skill/settings_snippet.json -A 3 -B 3Repository: selftune-dev/selftune
Length of output: 2255
Notation is accurate; consider using "Read, Skill" for consistency with settings.json matcher syntax.
Verified that hooks/skill-eval.ts handles both Read and Skill tool invocations via separate handlers (lines 113–118 and 267–272), and skill/settings_snippet.json correctly registers the hook with matcher "Read|Skill". The documentation change at line 101 is correct.
The "/" notation in "(Read/Skill)" is accurate but uses different syntax than settings_snippet.json's pipe operator "|". Consider using "(Read, Skill)" in the table for consistency, though the current form is unambiguous in context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skill/Workflows/Initialize.md` at line 101, Update the table row for
`PostToolUse` in skill/Workflows/Initialize.md to use the comma notation "(Read,
Skill)" instead of "(Read/Skill)" so it matches the matcher syntax used in
skill/settings_snippet.json (`"Read|Skill"`) and the handlers in
hooks/skill-eval.ts; specifically edit the `PostToolUse` row text to read
`PostToolUse (Read, Skill) | hooks/skill-eval.ts | Track skill triggers and
Skill tool invocations` to keep the documentation notation consistent with
`hooks/skill-eval.ts` handlers and the settings snippet.
Comment on lines
+80
to
92
| appendAuditEntry( | ||
| makeEntry({ proposal_id: "evo-001", details: "Proposal for pptx skill improvement" }), | ||
| ); | ||
| appendAuditEntry( | ||
| makeEntry({ proposal_id: "evo-002", details: "Proposal for csv-parser skill fix" }), | ||
| ); | ||
| appendAuditEntry(makeEntry({ proposal_id: "evo-003", details: "Another PPTX evolution step" })); | ||
|
|
||
| const pptxEntries = readAuditTrail("pptx", logPath); | ||
| const pptxEntries = readAuditTrail("pptx"); | ||
| expect(pptxEntries).toHaveLength(2); | ||
|
|
||
| const csvEntries = readAuditTrail("csv-parser", logPath); | ||
| const csvEntries = readAuditTrail("csv-parser"); | ||
| expect(csvEntries).toHaveLength(1); |
There was a problem hiding this comment.
Add a case that hits the DB-backed skill_name filter path.
These fixtures only identify the skill via details, so readAuditTrail("pptx") is exercising the backward-compatibility fallback rather than the new primary query path. The getLastDeployedProposal() cases below inherit the same blind spot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/evolution/audit.test.ts` around lines 80 - 92, The tests currently only
set skill via the details string so readAuditTrail("pptx") exercises the
fallback path; add at least one audit fixture that explicitly sets the
skill_name field (e.g., appendAuditEntry(makeEntry({ proposal_id: "evo-004",
skill_name: "pptx", details: "explicit skill_name entry" }))) so
readAuditTrail("pptx") uses the DB-backed skill_name filter; do the same for the
csv-parser case and update any getLastDeployedProposal() test fixtures to
include skill_name where appropriate so those tests exercise the primary query
path.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 31-32: Replace the inline cd in the "Dashboard typecheck" job step
with GitHub Actions' working-directory: set working-directory:
apps/local-dashboard and change the run command to just run: bunx tsc --noEmit;
update the step that currently contains run: cd apps/local-dashboard && bunx tsc
--noEmit so the step name ("Dashboard typecheck") remains the same but uses the
working-directory directive for clarity and readability.
- Line 43: The CI step currently uses the exact command string "run: bun test
tests/ --coverage" which restricts execution to the tests/ directory and skips
colocated tests; update that run step so bun runs tests across the repository
(remove the hardcoded "tests/" directory restriction or replace it with a
glob-based pattern that includes colocated test files such as *.test.tsx,
*.test.ts and package fixture tests) so files like Overview.test.tsx,
SkillReport.test.tsx and golden.test.ts are executed in CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e9fcc2a-7c83-4ebd-9947-9dad7517af70
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackages/ui/package.jsontests/normalization/normalization.test.ts
.github/workflows/ci.yml
Outdated
| - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 | ||
| - run: bun install | ||
| - run: bun test --coverage | ||
| - run: bun test tests/ --coverage |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find any test files outside the tests/ directory that would now be excluded
# Search for test files outside tests/ directory
echo "=== Test files outside tests/ directory ==="
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx --exclude tests --exclude node_modules --exclude dist
# Also check for any __tests__ directories outside tests/
echo ""
echo "=== __tests__ directories outside tests/ ==="
fd -t d '__tests__' --exclude tests --exclude node_modules --exclude distRepository: selftune-dev/selftune
Length of output: 307
The bun test tests/ command excludes test files outside tests/ that should be run.
The following test files would be skipped:
apps/local-dashboard/src/pages/Overview.test.tsxapps/local-dashboard/src/pages/SkillReport.test.tsxpackages/telemetry-contract/fixtures/golden.test.ts
Update the test command to include these patterns, or move colocated test files into the tests/ directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 43, The CI step currently uses the exact
command string "run: bun test tests/ --coverage" which restricts execution to
the tests/ directory and skips colocated tests; update that run step so bun runs
tests across the repository (remove the hardcoded "tests/" directory restriction
or replace it with a glob-based pattern that includes colocated test files such
as *.test.tsx, *.test.ts and package fixture tests) so files like
Overview.test.tsx, SkillReport.test.tsx and golden.test.ts are executed in CI.
…oundaries - normalization.ts: SQLite write before JSONL append (JSONL now best-effort) - skill-report.ts: include query + source in canonical_invocations response - dashboard-server.ts: use closeSingleton() instead of raw db.close() - prompt-log.ts: re-add JSONL dual-write for query log recovery path - export.ts: create output dir, remove 10k row limit on orchestrate export - actions.ts: make proposalId optional for rollback route - index.ts: DB-first with JSONL fallback for composability, export error boundary - session-stop.ts: remove unused _signalLogPath param + unsafe type cast - skill-eval.ts: fix Windows path detection in detectAgentType - signal-detection.test.ts: remove redundant db.close() - package.json: fix check script to use bun run test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ract tests - ci.yml: use working-directory directive instead of cd for dashboard typecheck - ci.yml + package.json: include packages/telemetry-contract/ in test scope (dashboard vitest tests excluded — they run via vitest, not bun test) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
tests/hooks/signal-detection.test.ts (1)
194-195:⚠️ Potential issue | 🟠 MajorNegative-path integration still asserts JSONL, not SQLite.
Line 194 and Line 195 validate
signals.jsonl, which can pass even if SQLite persistence/query behavior regresses. Assert againstimprovement_signalsin the test DB for the no-signal case.Suggested fix
-import type { ImprovementSignalRecord, PromptSubmitPayload } from "../../cli/selftune/types.js"; -import { readJsonl } from "../../cli/selftune/utils/jsonl.js"; +import type { PromptSubmitPayload } from "../../cli/selftune/types.js"; ... test("does not append signal for normal queries", async () => { const payload: PromptSubmitPayload = { user_prompt: "help me refactor this module", session_id: "sess-int-2", }; await processPrompt(payload, logPath, canonicalLogPath, promptStatePath, signalLogPath); - const signals = readJsonl<ImprovementSignalRecord>(signalLogPath); - expect(signals).toHaveLength(0); + const db = getDb(); + const row = db + .query("SELECT 1 AS found FROM improvement_signals WHERE session_id = ? LIMIT 1") + .get(payload.session_id) as { found: number } | null; + expect(row).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/signal-detection.test.ts` around lines 194 - 195, The test currently reads signals.jsonl via readJsonl<ImprovementSignalRecord>(signalLogPath) and asserts length 0, which misses validating SQLite persistence; instead query the test database's improvement_signals table (e.g., via the test DB client/helper used elsewhere in tests) and assert that the table has zero rows for the no-signal case. Replace the readJsonl/ImprovementSignalRecord/signalLogPath assertion with a DB query against improvement_signals and assert row count is 0 so the negative-path verifies SQLite behavior.cli/selftune/hooks/session-stop.ts (1)
166-182:⚠️ Potential issue | 🟠 MajorNon-URL remote fallback can still leak credential-like prefixes.
Line 181 treats non-URL remotes as safe and persists
rawRemoteunchanged. scp-style values liketoken@github.com:org/repo.gitwould be stored verbatim.Proposed fix
+ const sanitizeRemote = (value: string): string => { + try { + const parsed = new URL(value); + parsed.username = ""; + parsed.password = ""; + return `${parsed.protocol}//${parsed.host}${parsed.pathname}`; + } catch { + // scp-like: user@host:path -> host:path + const scpLike = value.match(/^[^@]+@([^:]+):(.+)$/); + if (scpLike) return `${scpLike[1]}:${scpLike[2]}`; + return value; + } + }; + if (rawRemote) { - try { - const parsed = new URL(rawRemote); - parsed.username = ""; - parsed.password = ""; - repoRemote = `${parsed.protocol}//${parsed.host}${parsed.pathname}`; - } catch { - repoRemote = rawRemote; // SSH or non-URL format, safe as-is - } + repoRemote = sanitizeRemote(rawRemote); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/session-stop.ts` around lines 166 - 182, The fallback branch that assigns repoRemote = rawRemote can leak credential-like prefixes (e.g., token@github.com:org/repo.git); update the catch branch handling around rawRemote/repoRemote so non-URL scp-style remotes are sanitized before persisting by stripping any leading "user@" or "user:pass@" component (e.g., remove the substring matching ^[^@]+@) and then assign the sanitized value to repoRemote; keep the URL-parsing branch unchanged (the try block that uses new URL(rawRemote) and clears parsed.username/parsed.password).cli/selftune/hooks/skill-eval.ts (1)
221-236:⚠️ Potential issue | 🟠 MajorFix
classifyInvocationType()before writing eval data.The explicit branches are case-sensitive against
trimmed, the slash-command check has no token boundary, and the implicit branch still builds aRegExpfrom variable input. That means<command-name>/Browser</command-name>is mislabeled asimplicit, and/selftune2is treated asexplicit.🔧 Minimal fix
function classifyInvocationType( query: string, skillName: string, ): "explicit" | "implicit" | "inferred" { const trimmed = query.trim(); + const trimmedLower = trimmed.toLowerCase(); const skillLower = skillName.toLowerCase(); // /selftune or /selftune args - if (trimmed.toLowerCase().startsWith(`/${skillLower}`)) return "explicit"; + const explicitPrefix = `/${skillLower}`; + if (trimmedLower.startsWith(explicitPrefix)) { + const nextChar = trimmedLower[explicitPrefix.length]; + if (nextChar === undefined || !/[a-z0-9_]/.test(nextChar)) return "explicit"; + } // <command-name>/selftune</command-name> pattern (skill already loaded) - if (trimmed.includes(`<command-name>/${skillLower}</command-name>`)) return "explicit"; - if (trimmed.includes(`<command-name>${skillLower}</command-name>`)) return "explicit"; + if (trimmedLower.includes(`<command-name>/${skillLower}</command-name>`)) return "explicit"; + if (trimmedLower.includes(`<command-name>${skillLower}</command-name>`)) return "explicit"; // User mentioned the skill name in their prompt (case-insensitive word boundary) - const mentionPattern = new RegExp( - `\\b${skillLower.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`, - "i", - ); - if (mentionPattern.test(trimmed)) return "implicit"; + let idx = trimmedLower.indexOf(skillLower); + while (idx !== -1) { + const before = trimmedLower[idx - 1]; + const after = trimmedLower[idx + skillLower.length]; + const boundaryBefore = idx === 0 || !/[a-z0-9_]/.test(before); + const boundaryAfter = + idx + skillLower.length === trimmedLower.length || !/[a-z0-9_]/.test(after); + if (boundaryBefore && boundaryAfter) return "implicit"; + idx = trimmedLower.indexOf(skillLower, idx + 1); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/skill-eval.ts` around lines 221 - 236, classifyInvocationType currently does case-sensitive checks and builds an unsafe RegExp; fix by normalizing the input and escaping skillName: use a lowercased version of trimmed (e.g., const lc = trimmed.toLowerCase()) for the startsWith check and the two includes checks that look for `<command-name>/${skillLower}</command-name>` and `<command-name>${skillLower}</command-name>`, change the slash-command test to require a token boundary after the skill (e.g., startsWith(`/${skillLower}`) AND next char is end-of-string or non-word) or use a regex like new RegExp(`^/${escapedSkill}(?:\\b|$)`, 'i'); escape skillName when building mentionPattern (use skillLower.replace(/[.*+?^${}()|[\\]\\\\]/g, "\\$&")) and keep the 'i' flag so mentionPattern correctly matches whole-word, case-insensitive mentions; update references to trimmed to use lc where appropriate and update mentionPattern construction to use escapedSkill.cli/selftune/export.ts (1)
39-40:⚠️ Potential issue | 🟠 Major
orchestrateexport is still truncated by the query helper default.
getOrchestrateRuns(db)defaults tolimit = 20incli/selftune/localdb/queries.ts, so this export silently drops older runs. Export should fetch all rows by default and make any limit explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/export.ts` around lines 39 - 40, The orchestrate export currently calls getOrchestrateRuns(db) which uses its default limit=20 and thus silences older runs; update the orchestrate.query export in cli/selftune/export.ts to call getOrchestrateRuns with an explicit argument that requests all rows (for example pass null/undefined if the helper treats that as no limit, or add/use a getOrchestrateRunsAll wrapper that returns all runs), or change getOrchestrateRuns invocation to accept and pass a specific large/explicit limit so the export no longer truncates results by the default 20; reference getOrchestrateRuns(db) in cli/selftune/localdb/queries.ts and the orchestrate.query entry in cli/selftune/export.ts when making the change.cli/selftune/routes/actions.ts (1)
45-46:⚠️ Potential issue | 🟠 MajorValidate request-body strings before building CLI args.
These
as stringcasts accept truthy non-string values, so objects or numbers can slip intoargsand only fail insideBun.spawnor a customActionRunner. Usetypeof value === "string"guards forskill,skillPath, andproposalId. As per coding guidelines,cli/**/*.ts: Strict typing (noany, preferunknownwith type guards).🛡️ Suggested change
- const skill = body.skill as string | undefined; - const skillPath = body.skillPath as string | undefined; + const skill = typeof body.skill === "string" ? body.skill : undefined; + const skillPath = typeof body.skillPath === "string" ? body.skillPath : undefined; @@ - const skill = body.skill as string | undefined; - const skillPath = body.skillPath as string | undefined; - const proposalId = body.proposalId as string | undefined; + const skill = typeof body.skill === "string" ? body.skill : undefined; + const skillPath = typeof body.skillPath === "string" ? body.skillPath : undefined; + const proposalId = typeof body.proposalId === "string" ? body.proposalId : undefined;Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/routes/actions.ts` around lines 45 - 46, Replace the unsafe "as string" casts for body.skill, body.skillPath and body.proposalId in actions.ts with runtime type checks: use typeof body.skill === "string" (and similarly for skillPath and proposalId) before assigning to skill/skillPath or pushing into args; only append those values to the CLI args array when they are strings to prevent objects/numbers from leaking into Bun.spawn or ActionRunner (update the code locations around the skill/skillPath declarations and the args-building logic where proposalId is used).cli/selftune/dashboard-server.ts (1)
177-185:⚠️ Potential issue | 🟠 MajorSSE invalidation is still disconnected from SQLite writes.
refreshV2Data*()are now no-ops, and the only broadcast trigger isfs.watchon the legacy JSONL files. Direct SQLite writes from the new source-of-truth path never hit those watchers, so/api/v2/eventsstops emitting updates for normal dashboard activity.Also applies to: 213-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 177 - 185, The SSE stream stopped receiving updates because refreshV2Data() and refreshV2DataImmediate() were turned into no-ops, so direct SQLite writes never trigger the same broadcast path used by fs.watch for /api/v2/events; restore these functions to call the existing broadcast/invalidation routine (the same code path that fs.watch invokes) or emit the SSE update directly (e.g., call the event emitter/notify function used by the file-change handler) so that writes through the new SQLite source-of-truth will push updates to /api/v2/events; locate and update refreshV2Data, refreshV2DataImmediate, and the file-change handler (fs.watch) to reuse the same notify/broadcast routine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 361-364: Wrap the call to req.json() in a try/catch before
invoking handleAction so malformed or non-object bodies produce a 400 response
instead of throwing; validate the parsed value is an object (Record<string,
unknown>) and if parsing or validation fails return withCors(new
Response(JSON.stringify({ error: "invalid_request", message: "Request body must
be valid JSON object" }), { status: 400 })) (or equivalent) and only call
handleAction(action, body, executeAction) when the body is valid. Ensure you
update the branch that currently calls handleAction directly (the block using
url.pathname.startsWith("/api/actions/") and req.method === "POST") to perform
this guarded parse/validation.
In `@cli/selftune/export.ts`:
- Around line 47-51: Replace the current warn-and-continue behavior when an
unknown table is encountered (inside the loop over selectedTables where
tables[tableName] is falsy) with a hard failure: when table is undefined, log an
actionable error message that includes the invalid tableName and suggests
`selftune export --help` (or throw an Error with that message), then exit the
process with code 1 (or let index.ts propagate the thrown Error so it returns
exit code 1). Update the block that currently uses console.warn to instead
perform this fail-fast behavior so callers of index.ts get a non-zero exit code
for typos like `audti`.
In `@cli/selftune/hooks/skill-eval.ts`:
- Line 15: The import list includes an unused symbol "join" which triggers lint
noUnusedImports; remove "join" from the import statement (the import line that
currently reads import { basename, dirname, join } from "node:path") so it only
imports the used symbols (basename, dirname) to resolve the lint failure.
- Around line 257-259: The code uses JSON.parse(readFileSync(metaPath, "utf-8"))
and returns meta.agentType directly which is untyped; narrow the parsed value
before returning a string by checking that meta is a non-null object and that
meta.agentType is a string (e.g. typeof meta === "object" && meta !== null &&
typeof meta.agentType === "string") and only then return meta.agentType,
otherwise return the fallback "subagent"; reference existsSync, metaPath,
JSON.parse, readFileSync, and meta.agentType when making the change.
In `@cli/selftune/index.ts`:
- Around line 73-76: The auto-update block in cli/selftune/index.ts (the dynamic
import of "./auto-update.js" and the call to autoUpdate()) must be made
best-effort: wrap the import and await autoUpdate() in a try/catch so any
import, network, or filesystem error is caught, log a warning that includes the
error details (use the existing logger or console.warn/error) and then continue
executing the requested command instead of letting the exception propagate and
abort the command.
In `@cli/selftune/normalization.ts`:
- Around line 93-117: The recovery path uses row.prompt_index unconditionally in
the getDb() block (the variable recovered) which fails when prompt_index is
null; update the SELECT to filter out null prompt_index (e.g., add "AND
prompt_index IS NOT NULL") or, if you must keep the latest row regardless of
prompt_index, detect row.prompt_index === null and derive the index from
prompt_id the same way the JSONL fallback does (or query for the last non-null
prompt_index separately) before assigning recovered.next_prompt_index =
row.prompt_index + 1; also ensure any other uses (like actionable lookup) remain
consistent.
- Around line 387-408: The wrappers appendCanonicalRecord and
appendCanonicalRecords currently ignore the boolean return from
writeCanonicalToDb/writeCanonicalBatchToDb; change each to check the returned
success flag and abort (or throw/log an error) when the DB write fails so we
don’t proceed to the JSONL append on a failed SQLite write—specifically, capture
the result of writeCanonicalToDb(record) in appendCanonicalRecord and of
writeCanonicalBatchToDb(records) in appendCanonicalRecords, and if the result is
false, stop execution (return or throw) and surface an error via processLogger
or thrown exception instead of performing the best-effort append.
In `@cli/selftune/routes/skill-report.ts`:
- Around line 155-169: The current query used to populate promptSamples (built
with skillSessionsCte) can emit the same prompts multiple times because LEFT
JOIN to skill_invocations produces one row per match; change the SQL to detect
matching invocations per prompt without duplicating rows—either replace the LEFT
JOIN with an EXISTS subquery (e.g. SELECT ... FROM prompts p WHERE EXISTS
(SELECT 1 FROM skill_invocations si WHERE si.matched_prompt_id = p.prompt_id AND
si.skill_name = ?)) and compute is_matched via CASE WHEN EXISTS(...) THEN 1 ELSE
0 END, or aggregate by p.prompt_id (GROUP BY p.prompt_id and use MAX(CASE WHEN
si.skill_name = ? THEN 1 ELSE 0 END) AS is_matched) so the LIMIT 50 returns
unique prompts; keep the same skillSessionsCte and bind the skillName parameter
accordingly for the EXISTS/aggregate check.
---
Duplicate comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 177-185: The SSE stream stopped receiving updates because
refreshV2Data() and refreshV2DataImmediate() were turned into no-ops, so direct
SQLite writes never trigger the same broadcast path used by fs.watch for
/api/v2/events; restore these functions to call the existing
broadcast/invalidation routine (the same code path that fs.watch invokes) or
emit the SSE update directly (e.g., call the event emitter/notify function used
by the file-change handler) so that writes through the new SQLite
source-of-truth will push updates to /api/v2/events; locate and update
refreshV2Data, refreshV2DataImmediate, and the file-change handler (fs.watch) to
reuse the same notify/broadcast routine.
In `@cli/selftune/export.ts`:
- Around line 39-40: The orchestrate export currently calls
getOrchestrateRuns(db) which uses its default limit=20 and thus silences older
runs; update the orchestrate.query export in cli/selftune/export.ts to call
getOrchestrateRuns with an explicit argument that requests all rows (for example
pass null/undefined if the helper treats that as no limit, or add/use a
getOrchestrateRunsAll wrapper that returns all runs), or change
getOrchestrateRuns invocation to accept and pass a specific large/explicit limit
so the export no longer truncates results by the default 20; reference
getOrchestrateRuns(db) in cli/selftune/localdb/queries.ts and the
orchestrate.query entry in cli/selftune/export.ts when making the change.
In `@cli/selftune/hooks/session-stop.ts`:
- Around line 166-182: The fallback branch that assigns repoRemote = rawRemote
can leak credential-like prefixes (e.g., token@github.com:org/repo.git); update
the catch branch handling around rawRemote/repoRemote so non-URL scp-style
remotes are sanitized before persisting by stripping any leading "user@" or
"user:pass@" component (e.g., remove the substring matching ^[^@]+@) and then
assign the sanitized value to repoRemote; keep the URL-parsing branch unchanged
(the try block that uses new URL(rawRemote) and clears
parsed.username/parsed.password).
In `@cli/selftune/hooks/skill-eval.ts`:
- Around line 221-236: classifyInvocationType currently does case-sensitive
checks and builds an unsafe RegExp; fix by normalizing the input and escaping
skillName: use a lowercased version of trimmed (e.g., const lc =
trimmed.toLowerCase()) for the startsWith check and the two includes checks that
look for `<command-name>/${skillLower}</command-name>` and
`<command-name>${skillLower}</command-name>`, change the slash-command test to
require a token boundary after the skill (e.g., startsWith(`/${skillLower}`) AND
next char is end-of-string or non-word) or use a regex like new
RegExp(`^/${escapedSkill}(?:\\b|$)`, 'i'); escape skillName when building
mentionPattern (use skillLower.replace(/[.*+?^${}()|[\\]\\\\]/g, "\\$&")) and
keep the 'i' flag so mentionPattern correctly matches whole-word,
case-insensitive mentions; update references to trimmed to use lc where
appropriate and update mentionPattern construction to use escapedSkill.
In `@cli/selftune/routes/actions.ts`:
- Around line 45-46: Replace the unsafe "as string" casts for body.skill,
body.skillPath and body.proposalId in actions.ts with runtime type checks: use
typeof body.skill === "string" (and similarly for skillPath and proposalId)
before assigning to skill/skillPath or pushing into args; only append those
values to the CLI args array when they are strings to prevent objects/numbers
from leaking into Bun.spawn or ActionRunner (update the code locations around
the skill/skillPath declarations and the args-building logic where proposalId is
used).
In `@tests/hooks/signal-detection.test.ts`:
- Around line 194-195: The test currently reads signals.jsonl via
readJsonl<ImprovementSignalRecord>(signalLogPath) and asserts length 0, which
misses validating SQLite persistence; instead query the test database's
improvement_signals table (e.g., via the test DB client/helper used elsewhere in
tests) and assert that the table has zero rows for the no-signal case. Replace
the readJsonl/ImprovementSignalRecord/signalLogPath assertion with a DB query
against improvement_signals and assert row count is 0 so the negative-path
verifies SQLite behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d100f1db-048b-4532-ba4a-5c83fc16a943
📒 Files selected for processing (11)
cli/selftune/dashboard-server.tscli/selftune/export.tscli/selftune/hooks/prompt-log.tscli/selftune/hooks/session-stop.tscli/selftune/hooks/skill-eval.tscli/selftune/index.tscli/selftune/normalization.tscli/selftune/routes/actions.tscli/selftune/routes/skill-report.tspackage.jsontests/hooks/signal-detection.test.ts
Comment on lines
+73
to
+76
| // Auto-update check (skip for hooks — they must be fast — and --help) | ||
| if (command && command !== "hook" && command !== "--help" && command !== "-h") { | ||
| const { autoUpdate } = await import("./auto-update.js"); | ||
| await autoUpdate(); |
There was a problem hiding this comment.
Auto-update must not block the requested command.
Any import, network, or filesystem failure thrown by autoUpdate() aborts every non-help/non-hook command before it reaches its own error handling. Make this best-effort: catch and log a warning, then continue with the original command.
🧯 Suggested change
// Auto-update check (skip for hooks — they must be fast — and --help)
if (command && command !== "hook" && command !== "--help" && command !== "-h") {
- const { autoUpdate } = await import("./auto-update.js");
- await autoUpdate();
+ try {
+ const { autoUpdate } = await import("./auto-update.js");
+ await autoUpdate();
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ console.warn(`[WARN] Auto-update skipped: ${message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Auto-update check (skip for hooks — they must be fast — and --help) | |
| if (command && command !== "hook" && command !== "--help" && command !== "-h") { | |
| const { autoUpdate } = await import("./auto-update.js"); | |
| await autoUpdate(); | |
| // Auto-update check (skip for hooks — they must be fast — and --help) | |
| if (command && command !== "hook" && command !== "--help" && command !== "-h") { | |
| try { | |
| const { autoUpdate } = await import("./auto-update.js"); | |
| await autoUpdate(); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| console.warn(`[WARN] Auto-update skipped: ${message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/index.ts` around lines 73 - 76, The auto-update block in
cli/selftune/index.ts (the dynamic import of "./auto-update.js" and the call to
autoUpdate()) must be made best-effort: wrap the import and await autoUpdate()
in a try/catch so any import, network, or filesystem error is caught, log a
warning that includes the error details (use the existing logger or
console.warn/error) and then continue executing the requested command instead of
letting the exception propagate and abort the command.
Comment on lines
+93
to
+117
| const { getDb } = require("./localdb/db.js") as { | ||
| getDb: () => import("bun:sqlite").Database; | ||
| }; | ||
| const db = getDb(); | ||
| const rows = db | ||
| .query( | ||
| "SELECT prompt_id, prompt_index, is_actionable FROM prompts WHERE session_id = ? ORDER BY prompt_index DESC LIMIT 1", | ||
| ) | ||
| .all(sessionId) as Array<{ | ||
| prompt_id: string; | ||
| prompt_index: number; | ||
| is_actionable: number; | ||
| }>; | ||
| if (rows.length > 0) { | ||
| const row = rows[0]; | ||
| recovered.next_prompt_index = row.prompt_index + 1; | ||
| recovered.last_prompt_id = row.prompt_id; | ||
| // Get last actionable | ||
| const actionable = db | ||
| .query( | ||
| "SELECT prompt_id, prompt_index FROM prompts WHERE session_id = ? AND is_actionable = 1 ORDER BY prompt_index DESC LIMIT 1", | ||
| ) | ||
| .get(sessionId) as { prompt_id: string; prompt_index: number } | null; | ||
| if (actionable) recovered.last_actionable_prompt_id = actionable.prompt_id; | ||
| return recovered; |
There was a problem hiding this comment.
SQLite prompt-state recovery still assumes prompt_index is always populated.
BuildPromptInput.prompt_index is optional, but this path does row.prompt_index + 1 unconditionally. If the newest prompts row for a session has a null index, recovery can jump to the wrong next ID after state-file loss. Filter out null prompt_index rows here, or mirror the JSONL fallback by deriving the index from prompt_id when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/normalization.ts` around lines 93 - 117, The recovery path uses
row.prompt_index unconditionally in the getDb() block (the variable recovered)
which fails when prompt_index is null; update the SELECT to filter out null
prompt_index (e.g., add "AND prompt_index IS NOT NULL") or, if you must keep the
latest row regardless of prompt_index, detect row.prompt_index === null and
derive the index from prompt_id the same way the JSONL fallback does (or query
for the last non-null prompt_index separately) before assigning
recovered.next_prompt_index = row.prompt_index + 1; also ensure any other uses
(like actionable lookup) remain consistent.
cli/selftune/normalization.ts
Outdated
Comment on lines
+387
to
+408
| export function appendCanonicalRecord(record: CanonicalRecord, logPath?: string): void { | ||
| writeCanonicalToDb(record); | ||
| // JSONL append — best-effort backup for prompt state recovery | ||
| try { | ||
| const path = logPath ?? CANONICAL_LOG; | ||
| const dir = dirname(path); | ||
| if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); | ||
| appendFileSync(path, `${JSON.stringify(record)}\n`, "utf-8"); | ||
| } catch { /* best-effort only */ } | ||
| } | ||
|
|
||
| export function appendCanonicalRecords( | ||
| records: CanonicalRecord[], | ||
| logPath: string = CANONICAL_LOG, | ||
| ): void { | ||
| for (const record of records) appendCanonicalRecord(record, logPath); | ||
| export function appendCanonicalRecords(records: CanonicalRecord[], logPath?: string): void { | ||
| writeCanonicalBatchToDb(records); | ||
| // JSONL append — best-effort backup for prompt state recovery | ||
| try { | ||
| const path = logPath ?? CANONICAL_LOG; | ||
| const dir = dirname(path); | ||
| if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); | ||
| for (const record of records) { | ||
| appendFileSync(path, `${JSON.stringify(record)}\n`, "utf-8"); | ||
| } | ||
| } catch { /* best-effort only */ } |
There was a problem hiding this comment.
Primary SQLite write failures are still silent here.
writeCanonicalToDb() / writeCanonicalBatchToDb() return a boolean, but both wrappers ignore that result and continue as if the write succeeded. When the SQLite write fails, callers still see a successful append even though the source of truth dropped the record.
🛡️ Suggested change
export function appendCanonicalRecord(record: CanonicalRecord, logPath?: string): void {
- writeCanonicalToDb(record);
+ if (!writeCanonicalToDb(record)) {
+ throw new Error(
+ "Failed to persist canonical telemetry to SQLite. Run 'selftune doctor' and retry.",
+ );
+ }
// JSONL append — best-effort backup for prompt state recovery
try {
const path = logPath ?? CANONICAL_LOG;
@@
export function appendCanonicalRecords(records: CanonicalRecord[], logPath?: string): void {
- writeCanonicalBatchToDb(records);
+ if (!writeCanonicalBatchToDb(records)) {
+ throw new Error(
+ "Failed to persist canonical telemetry batch to SQLite. Run 'selftune doctor' and retry.",
+ );
+ }
// JSONL append — best-effort backup for prompt state recovery
try {
const path = logPath ?? CANONICAL_LOG;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/normalization.ts` around lines 387 - 408, The wrappers
appendCanonicalRecord and appendCanonicalRecords currently ignore the boolean
return from writeCanonicalToDb/writeCanonicalBatchToDb; change each to check the
returned success flag and abort (or throw/log an error) when the DB write fails
so we don’t proceed to the JSONL append on a failed SQLite write—specifically,
capture the result of writeCanonicalToDb(record) in appendCanonicalRecord and of
writeCanonicalBatchToDb(records) in appendCanonicalRecords, and if the result is
false, stop execution (return or throw) and surface an error via processLogger
or thrown exception instead of performing the best-effort append.
Comment on lines
+155
to
+169
| const promptSamples = db | ||
| .query( | ||
| `${skillSessionsCte} | ||
| SELECT p.prompt_text, p.prompt_kind, p.is_actionable, p.occurred_at, p.session_id, | ||
| CASE WHEN si.matched_prompt_id IS NOT NULL THEN 1 ELSE 0 END AS is_matched | ||
| FROM prompts p | ||
| LEFT JOIN skill_invocations si ON si.matched_prompt_id = p.prompt_id | ||
| AND si.skill_name = ? | ||
| WHERE p.session_id IN (SELECT session_id FROM skill_sessions) | ||
| AND p.prompt_text IS NOT NULL | ||
| AND p.prompt_text != '' | ||
| ORDER BY is_matched DESC, p.occurred_at DESC | ||
| LIMIT 50`, | ||
| ) | ||
| .all(skillName, skillName) as Array<{ |
There was a problem hiding this comment.
The prompt sample join can duplicate rows.
One prompts row is emitted once per matching skill_invocations row, so repeated matches for the same prompt can consume the LIMIT 50 budget and hide unique samples. Use EXISTS or aggregate on p.prompt_id instead of a raw LEFT JOIN.
♻️ Suggested change
- SELECT p.prompt_text, p.prompt_kind, p.is_actionable, p.occurred_at, p.session_id,
- CASE WHEN si.matched_prompt_id IS NOT NULL THEN 1 ELSE 0 END AS is_matched
- FROM prompts p
- LEFT JOIN skill_invocations si ON si.matched_prompt_id = p.prompt_id
- AND si.skill_name = ?
+ SELECT p.prompt_text, p.prompt_kind, p.is_actionable, p.occurred_at, p.session_id,
+ EXISTS (
+ SELECT 1
+ FROM skill_invocations si
+ WHERE si.matched_prompt_id = p.prompt_id
+ AND si.skill_name = ?
+ ) AS is_matched
+ FROM prompts p
WHERE p.session_id IN (SELECT session_id FROM skill_sessions)
AND p.prompt_text IS NOT NULL
AND p.prompt_text != ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/routes/skill-report.ts` around lines 155 - 169, The current
query used to populate promptSamples (built with skillSessionsCte) can emit the
same prompts multiple times because LEFT JOIN to skill_invocations produces one
row per match; change the SQL to detect matching invocations per prompt without
duplicating rows—either replace the LEFT JOIN with an EXISTS subquery (e.g.
SELECT ... FROM prompts p WHERE EXISTS (SELECT 1 FROM skill_invocations si WHERE
si.matched_prompt_id = p.prompt_id AND si.skill_name = ?)) and compute
is_matched via CASE WHEN EXISTS(...) THEN 1 ELSE 0 END, or aggregate by
p.prompt_id (GROUP BY p.prompt_id and use MAX(CASE WHEN si.skill_name = ? THEN 1
ELSE 0 END) AS is_matched) so the LIMIT 50 returns unique prompts; keep the same
skillSessionsCte and bind the skillName parameter accordingly for the
EXISTS/aggregate check.
…ype guards - dashboard-server.ts: catch malformed JSON body → 400 instead of 500 - export.ts: throw on unknown table name instead of warn+continue - skill-eval.ts: remove unused `join` import, add typeof guards for agentType Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/ci.yml (1)
44-44:⚠️ Potential issue | 🟠 MajorCI tests are still path-scoped and can skip colocated tests.
Line 44 hardcodes
tests/andpackages/telemetry-contract/, so colocated tests (for example inapps/local-dashboard) are not covered by this job.♻️ Proposed fix
- - run: bun test tests/ packages/telemetry-contract/ --coverage + - run: bun test --coverage#!/bin/bash # Verify whether current CI scope excludes test files. # Expected: no output if everything is included by the scoped command. fd -t f -E node_modules -E dist -E coverage -e ts -e tsx '(test|spec)\.(ts|tsx)$' \ | sort \ | awk '!/^(tests\/|packages\/telemetry-contract\/)/ { print }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 44, The CI step currently hardcodes the test paths in the run command ("run: bun test tests/ packages/telemetry-contract/ --coverage"), which excludes colocated tests; update that command to run all tests (e.g., remove the explicit paths and use "bun test --coverage") or replace it with a discovery-based invocation (e.g., run a script that uses fd to find all *.(test|spec).(ts|tsx) files and passes them to bun) so colocated tests like apps/local-dashboard are included; ensure the workflow step that contains the current run string is updated accordingly and retains the --coverage flag.package.json (1)
59-59:⚠️ Potential issue | 🟠 Major
testscript is too narrow and misses colocated tests.Line 59 restricts discovery to two paths;
check(Line 65) now inherits that limitation and can report false green.♻️ Proposed fix
- "test": "bun test tests/ packages/telemetry-contract/", + "test": "bun test",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 59, The "test" npm script currently hardcodes two paths and misses colocated tests; update the "test" script (the "test" entry in package.json) to run a broad discovery (e.g., call the test runner without path-restricting args or use a glob that covers all packages and colocated tests) so all tests are executed, and ensure the "check" script that depends on "test" no longer inherits the narrow scope—make both refer to the updated, wide-discovery test command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/hooks/prompt-log.ts`:
- Around line 155-156: The call to writeImprovementSignalToDb(...) can return
false without throwing and its result is currently ignored, causing silent data
loss; update the code around writeImprovementSignalToDb and the function that
accepts _signalLogPath to check the boolean return and, on false, perform a
best-effort non-blocking fallback that appends the signal record as a JSONL line
to a file (use _signalLogPath if provided, otherwise default to a file under the
user's home ~/.claude/), ensuring any file errors are caught and logged but do
not throw; also remove or use the dead _signalLogPath parameter where
appropriate so the fallback path is plumbed in from the caller to the
write-improvement logic (refer to writeImprovementSignalToDb and the surrounding
function signature that currently has _signalLogPath?: string).
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Line 44: The CI step currently hardcodes the test paths in the run command
("run: bun test tests/ packages/telemetry-contract/ --coverage"), which excludes
colocated tests; update that command to run all tests (e.g., remove the explicit
paths and use "bun test --coverage") or replace it with a discovery-based
invocation (e.g., run a script that uses fd to find all *.(test|spec).(ts|tsx)
files and passes them to bun) so colocated tests like apps/local-dashboard are
included; ensure the workflow step that contains the current run string is
updated accordingly and retains the --coverage flag.
In `@package.json`:
- Line 59: The "test" npm script currently hardcodes two paths and misses
colocated tests; update the "test" script (the "test" entry in package.json) to
run a broad discovery (e.g., call the test runner without path-restricting args
or use a glob that covers all packages and colocated tests) so all tests are
executed, and ensure the "check" script that depends on "test" no longer
inherits the narrow scope—make both refer to the updated, wide-discovery test
command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 280eb53a-6a2d-4a65-898a-56431cf5d57c
📒 Files selected for processing (4)
.github/workflows/ci.ymlcli/selftune/hooks/prompt-log.tscli/selftune/normalization.tspackage.json
Comment on lines
+155
to
+156
| _signalLogPath?: string, | ||
| ): Promise<QueryLogRecord | null> { |
There was a problem hiding this comment.
Handle failed improvement-signal DB writes to prevent silent data loss.
Line 218 ignores the boolean result from writeImprovementSignalToDb(). That function fail-opens and can return false without throwing, so the catch won’t run and signal records are dropped silently. Also, _signalLogPath on Line 155 is currently dead.
🔧 Suggested fix (best-effort fallback, still non-blocking)
export async function processPrompt(
payload: PromptSubmitPayload,
logPath: string = QUERY_LOG,
canonicalLogPath: string = CANONICAL_LOG,
promptStatePath?: string,
- _signalLogPath?: string,
+ signalLogPath?: string,
): Promise<QueryLogRecord | null> {
@@
try {
const signal = detectImprovementSignal(query, record.session_id);
if (signal) {
const { writeImprovementSignalToDb } = await import("../localdb/direct-write.js");
- writeImprovementSignalToDb(signal);
+ const ok = writeImprovementSignalToDb(signal);
+ if (!ok && signalLogPath) {
+ try {
+ appendJsonl(signalLogPath, signal);
+ } catch {
+ /* hooks must never block */
+ }
+ }
}
} catch {
// silent — hooks must never block Claude
}Based on learnings, Log files are append-only JSONL format stored at ~/.claude/.
Also applies to: 217-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/hooks/prompt-log.ts` around lines 155 - 156, The call to
writeImprovementSignalToDb(...) can return false without throwing and its result
is currently ignored, causing silent data loss; update the code around
writeImprovementSignalToDb and the function that accepts _signalLogPath to check
the boolean return and, on false, perform a best-effort non-blocking fallback
that appends the signal record as a JSONL line to a file (use _signalLogPath if
provided, otherwise default to a file under the user's home ~/.claude/),
ensuring any file errors are caught and logged but do not throw; also remove or
use the dead _signalLogPath parameter where appropriate so the fallback path is
plumbed in from the caller to the write-improvement logic (refer to
writeImprovementSignalToDb and the surrounding function signature that currently
has _signalLogPath?: string).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Existing databases created before the skill_usage/skill_invocations consolidation lack query, skill_path, skill_scope, source columns. CREATE TABLE IF NOT EXISTS doesn't add columns to existing tables. Added ALTER TABLE migrations in schema.ts that run on every openDb() — SQLite throws "duplicate column" on re-runs which is caught silently. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Indexes on query/skill_scope columns were in CREATE_INDEXES (runs before migrations), causing "no such column: query" on existing databases. Moved to POST_MIGRATION_INDEXES that run after ALTER TABLE ADD COLUMN. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
cli/selftune/export.ts (1)
39-41:⚠️ Potential issue | 🟠 Major
orchestrateexports are still silently truncated.
getOrchestrateRuns(db)uses the query helper default limit, so this path exports only a bounded subset instead of full history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/export.ts` around lines 39 - 41, The orchestrate export currently uses getOrchestrateRuns(db) which relies on the query helper's default limit and therefore truncates history; update the export mapping for "orchestrate" to call getOrchestrateRuns with an explicit parameter (or use the helper's pagination API) that requests the full/unbounded result set (e.g., pass a no-limit or very large limit option or implement paging) so the orchestrate_run_log.jsonl contains the complete history; locate the export entry named "orchestrate" and the getOrchestrateRuns function to apply the change.cli/selftune/dashboard-server.ts (1)
213-225:⚠️ Potential issue | 🟠 MajorSSE refresh trigger is still JSONL-bound in a SQLite-first write path.
onLogFileChange()only runs from JSONL file watchers, so SQLite-direct updates can skip/api/v2/eventsbroadcasts and leave clients stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/dashboard-server.ts` around lines 213 - 225, The SSE refresh is only triggered by the JSONL file watcher via onLogFileChange(), so updates written directly to SQLite never call refreshV2DataImmediate() or broadcastSSE("update"); modify the update path so database-driven writes also trigger the same refresh/broadcast: either extract the debounce+refresh logic into a shared function (e.g., emitV2Update() called by onLogFileChange()) and call that function from the SQLite write/update hooks, or add a DB change listener that invokes refreshV2DataImmediate() and broadcastSSE("update") with the same debounce behavior as FS_DEBOUNCE_MS; ensure you reference onLogFileChange, WATCHED_LOGS, refreshV2DataImmediate, and broadcastSSE when making the change so both JSONL and SQLite flows use the same code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 300-307: The cancel handler currently uses the wrong parameter
name (cancel(controller)) and thus receives the cancellation reason instead of
the controller, causing sseClients.delete(...) to remove the wrong value and
leak controllers; fix by capturing the controller in an outer variable inside
the ReadableStream factory (e.g. let streamController), assign it in
start(controller) and add it to sseClients, then change cancel(_reason) to use
sseClients.delete(streamController) (and null out streamController after
deletion) so the exact controller added in start is removed on cancel; reference
ReadableStream, start(controller), cancel(...), and sseClients.
In `@cli/selftune/export.ts`:
- Around line 59-62: Replace the current warn-and-continue behavior for parsing
options.since: validate options.since by creating sinceDate and if
sinceDate.getTime() is NaN, write an actionable error message and exit with a
non-zero status (e.g., process.exit(1)) instead of console.warn; update the
block around sinceDate/options.since in cli/selftune/export.ts (the code that
constructs sinceDate) to produce a clear user-facing message like "Invalid
--since date: <value> — please provide YYYY-MM-DD" and call process.exit(1) so
the CLI fails fast on bad input.
In `@cli/selftune/localdb/schema.ts`:
- Around line 239-244: MIGRATIONS is missing an ALTER for the agent_type column
that exists in CREATE_SKILL_INVOCATIONS; add an ALTER TABLE skill_invocations
ADD COLUMN agent_type TEXT (or the appropriate type) to the MIGRATIONS array so
existing databases receive the agent_type column during upgrade and writes that
include agent_type won't fail.
---
Duplicate comments:
In `@cli/selftune/dashboard-server.ts`:
- Around line 213-225: The SSE refresh is only triggered by the JSONL file
watcher via onLogFileChange(), so updates written directly to SQLite never call
refreshV2DataImmediate() or broadcastSSE("update"); modify the update path so
database-driven writes also trigger the same refresh/broadcast: either extract
the debounce+refresh logic into a shared function (e.g., emitV2Update() called
by onLogFileChange()) and call that function from the SQLite write/update hooks,
or add a DB change listener that invokes refreshV2DataImmediate() and
broadcastSSE("update") with the same debounce behavior as FS_DEBOUNCE_MS; ensure
you reference onLogFileChange, WATCHED_LOGS, refreshV2DataImmediate, and
broadcastSSE when making the change so both JSONL and SQLite flows use the same
code path.
In `@cli/selftune/export.ts`:
- Around line 39-41: The orchestrate export currently uses
getOrchestrateRuns(db) which relies on the query helper's default limit and
therefore truncates history; update the export mapping for "orchestrate" to call
getOrchestrateRuns with an explicit parameter (or use the helper's pagination
API) that requests the full/unbounded result set (e.g., pass a no-limit or very
large limit option or implement paging) so the orchestrate_run_log.jsonl
contains the complete history; locate the export entry named "orchestrate" and
the getOrchestrateRuns function to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36893a0c-df36-40e1-8153-65843d5a9484
📒 Files selected for processing (5)
cli/selftune/dashboard-server.tscli/selftune/export.tscli/selftune/hooks/skill-eval.tscli/selftune/localdb/db.tscli/selftune/localdb/schema.ts
Comment on lines
+300
to
+307
| const stream = new ReadableStream({ | ||
| start(controller) { | ||
| sseClients.add(controller); | ||
| controller.enqueue(new TextEncoder().encode(": connected\n\n")); | ||
| }, | ||
| cancel(controller) { | ||
| sseClients.delete(controller); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the WHATWG/Web Streams API, what arguments does the cancel()method of an underlying source fornew ReadableStream({...}) receive?
💡 Result:
In the WHATWG Streams Standard, an underlying source’s cancel() is called with one optional argument, the cancellation reason:
- Signature:
cancel(optional any reason) -> Promise<undefined> - It receives the same
reasonvalue passed by the consumer tostream.cancel(reason)orreader.cancel(reason)(orundefinedif none was provided). [1]
Sources: [1] (streams.spec.whatwg.org)
Citations:
🏁 Script executed:
# First, check the full context around lines 300-307
head -320 cli/selftune/dashboard-server.ts | tail -40Repository: selftune-dev/selftune
Length of output: 1369
🏁 Script executed:
# Check if ReadableStream is imported and from where
rg "ReadableStream|from.*stream" cli/selftune/dashboard-server.ts -B2 -A2Repository: selftune-dev/selftune
Length of output: 517
🏁 Script executed:
# Check Bun's ReadableStream support and Web Streams API documentation
# Verify the cancel() signature in actual usage context
rg "cancel\(" cli/selftune/dashboard-server.ts -B3 -A3Repository: selftune-dev/selftune
Length of output: 293
🏁 Script executed:
# Check the type annotation for sseClients to understand expected behavior
rg "sseClients.*=" cli/selftune/dashboard-server.ts -B2 -A2Repository: selftune-dev/selftune
Length of output: 263
SSE cleanup leaks controller objects due to incorrect cancel() signature.
The cancel() method receives a cancellation reason as its argument, not the controller. The current code passes controller as a parameter name but actually receives the reason value, so sseClients.delete(controller) deletes the wrong object. This causes all controller objects to remain in the Set indefinitely, leaking memory as clients disconnect.
Proposed fix
if (url.pathname === "/api/v2/events" && req.method === "GET") {
+ let streamController: ReadableStreamDefaultController<Uint8Array> | null = null;
const stream = new ReadableStream({
start(controller) {
- sseClients.add(controller);
- controller.enqueue(new TextEncoder().encode(": connected\n\n"));
+ streamController = controller;
+ sseClients.add(controller);
+ controller.enqueue(new TextEncoder().encode(": connected\n\n"));
},
- cancel(controller) {
- sseClients.delete(controller);
+ cancel() {
+ if (streamController) sseClients.delete(streamController);
+ streamController = null;
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/dashboard-server.ts` around lines 300 - 307, The cancel handler
currently uses the wrong parameter name (cancel(controller)) and thus receives
the cancellation reason instead of the controller, causing
sseClients.delete(...) to remove the wrong value and leak controllers; fix by
capturing the controller in an outer variable inside the ReadableStream factory
(e.g. let streamController), assign it in start(controller) and add it to
sseClients, then change cancel(_reason) to use
sseClients.delete(streamController) (and null out streamController after
deletion) so the exact controller added in start is removed on cancel; reference
ReadableStream, start(controller), cancel(...), and sseClients.
Comment on lines
+59
to
+62
| const sinceDate = new Date(options.since); | ||
| if (Number.isNaN(sinceDate.getTime())) { | ||
| console.warn(`Invalid --since date: ${options.since}, skipping filter`); | ||
| } else { |
There was a problem hiding this comment.
Fail fast on invalid --since instead of warning and continuing.
Continuing with unfiltered data on invalid input is risky and hides user error; throw an actionable error so CLI exits non-zero.
Proposed fix
if (options.since) {
const sinceDate = new Date(options.since);
if (Number.isNaN(sinceDate.getTime())) {
- console.warn(`Invalid --since date: ${options.since}, skipping filter`);
+ throw new Error(
+ `Invalid --since value: ${options.since}. Retry with an ISO-8601 timestamp, e.g. 2026-03-01T00:00:00Z.`,
+ );
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sinceDate = new Date(options.since); | |
| if (Number.isNaN(sinceDate.getTime())) { | |
| console.warn(`Invalid --since date: ${options.since}, skipping filter`); | |
| } else { | |
| const sinceDate = new Date(options.since); | |
| if (Number.isNaN(sinceDate.getTime())) { | |
| throw new Error( | |
| `Invalid --since value: ${options.since}. Retry with an ISO-8601 timestamp, e.g. 2026-03-01T00:00:00Z.`, | |
| ); | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/export.ts` around lines 59 - 62, Replace the current
warn-and-continue behavior for parsing options.since: validate options.since by
creating sinceDate and if sinceDate.getTime() is NaN, write an actionable error
message and exit with a non-zero status (e.g., process.exit(1)) instead of
console.warn; update the block around sinceDate/options.since in
cli/selftune/export.ts (the code that constructs sinceDate) to produce a clear
user-facing message like "Invalid --since date: <value> — please provide
YYYY-MM-DD" and call process.exit(1) so the CLI fails fast on bad input.
Comment on lines
+239
to
+244
| export const MIGRATIONS = [ | ||
| // skill_invocations consolidation (skill_usage columns merged in) | ||
| `ALTER TABLE skill_invocations ADD COLUMN query TEXT`, | ||
| `ALTER TABLE skill_invocations ADD COLUMN skill_path TEXT`, | ||
| `ALTER TABLE skill_invocations ADD COLUMN skill_scope TEXT`, | ||
| `ALTER TABLE skill_invocations ADD COLUMN source TEXT`, |
There was a problem hiding this comment.
Add migration for agent_type to keep upgrades compatible.
agent_type is present in CREATE_SKILL_INVOCATIONS but missing in MIGRATIONS. Existing DBs can stay without this column and fail writes that include it.
Proposed fix
export const MIGRATIONS = [
// skill_invocations consolidation (skill_usage columns merged in)
+ `ALTER TABLE skill_invocations ADD COLUMN agent_type TEXT`,
`ALTER TABLE skill_invocations ADD COLUMN query TEXT`,
`ALTER TABLE skill_invocations ADD COLUMN skill_path TEXT`,
`ALTER TABLE skill_invocations ADD COLUMN skill_scope TEXT`,
`ALTER TABLE skill_invocations ADD COLUMN source TEXT`,
];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/schema.ts` around lines 239 - 244, MIGRATIONS is missing
an ALTER for the agent_type column that exists in CREATE_SKILL_INVOCATIONS; add
an ALTER TABLE skill_invocations ADD COLUMN agent_type TEXT (or the appropriate
type) to the MIGRATIONS array so existing databases receive the agent_type
column during upgrade and writes that include agent_type won't fail.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cli/selftune/localdb/schema.ts (1)
236-242:⚠️ Potential issue | 🟠 MajorMissing migration for
agent_typecolumn.
agent_typeappears inCREATE_SKILL_INVOCATIONS(line 50) but is absent fromMIGRATIONS. Existing databases upgrading won't have this column, causing write failures whenagent_typeis included in INSERTs.Proposed fix
export const MIGRATIONS = [ // skill_invocations consolidation (skill_usage columns merged in) + `ALTER TABLE skill_invocations ADD COLUMN agent_type TEXT`, `ALTER TABLE skill_invocations ADD COLUMN query TEXT`, `ALTER TABLE skill_invocations ADD COLUMN skill_path TEXT`, `ALTER TABLE skill_invocations ADD COLUMN skill_scope TEXT`, `ALTER TABLE skill_invocations ADD COLUMN source TEXT`, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/schema.ts` around lines 236 - 242, The MIGRATIONS array for the skill_invocations table is missing an ALTER for the agent_type column referenced by CREATE_SKILL_INVOCATIONS, causing upgrades to fail when INSERTs include agent_type; update the MIGRATIONS constant to add an ALTER TABLE skill_invocations ADD COLUMN agent_type TEXT (matching the type used in CREATE_SKILL_INVOCATIONS) so existing databases get the new column during migration.cli/selftune/localdb/db.ts (1)
47-54:⚠️ Potential issue | 🟠 MajorSwallowing all migration errors masks real failures.
The catch block ignores every exception, not just "duplicate column name" errors. A typo in a migration or a disk-full condition would fail silently, leaving the schema in a partially upgraded state.
Proposed fix
for (const migration of MIGRATIONS) { try { db.run(migration); - } catch { - // Column already exists — expected on subsequent runs + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + if (!message.includes("duplicate column name")) throw error; + // Column already exists — expected on subsequent runs } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/localdb/db.ts` around lines 47 - 54, The migration loop is currently swallowing all exceptions which masks real failures; update the loop that iterates over MIGRATIONS and the db.run(migration) call to only ignore expected "column already exists"/duplicate-column errors (by checking the error message/code) and for any other error log the full error and rethrow (or propagate) so failing migrations don't silently continue; ensure you reference MIGRATIONS and db.run when adding error filtering/handling and include processLogger or throw to surface unexpected failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli/selftune/localdb/db.ts`:
- Around line 47-54: The migration loop is currently swallowing all exceptions
which masks real failures; update the loop that iterates over MIGRATIONS and the
db.run(migration) call to only ignore expected "column already
exists"/duplicate-column errors (by checking the error message/code) and for any
other error log the full error and rethrow (or propagate) so failing migrations
don't silently continue; ensure you reference MIGRATIONS and db.run when adding
error filtering/handling and include processLogger or throw to surface
unexpected failures.
In `@cli/selftune/localdb/schema.ts`:
- Around line 236-242: The MIGRATIONS array for the skill_invocations table is
missing an ALTER for the agent_type column referenced by
CREATE_SKILL_INVOCATIONS, causing upgrades to fail when INSERTs include
agent_type; update the MIGRATIONS constant to add an ALTER TABLE
skill_invocations ADD COLUMN agent_type TEXT (matching the type used in
CREATE_SKILL_INVOCATIONS) so existing databases get the new column during
migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ac833de-575b-4d72-876d-a6d37d6dcfb4
📒 Files selected for processing (2)
cli/selftune/localdb/db.tscli/selftune/localdb/schema.ts
The catch-all was masking real failures (disk full, permissions, etc.). Now checks for "duplicate column" in the error message and rethrows anything else. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/localdb/db.ts`:
- Around line 66-69: The JSDoc comment describing retrieval from the "_meta"
table is no longer adjacent to the function it documents; move that comment
block so it sits directly above the getMeta function declaration (replace the
misplaced JSDoc that currently sits before the singleton section), ensuring the
getMeta symbol is immediately preceded by the "_meta" JSDoc and delete the
old/commented duplicate left above the singleton block.
- Around line 47-55: When a migration in the MIGRATIONS loop (db.run calls)
throws a non-"duplicate column" error, wrap and rethrow a new Error that
includes an actionable agent instruction (e.g. "Run: selftune rebuild-db") plus
the original error message for context; locate the try/catch around the for
(const migration of MIGRATIONS) block in db.ts and replace the bare throw err
with throwing a new Error that composes a short directive ("Action: selftune
rebuild-db") and the original error message (use the msg variable) so the
surfaced error is actionable for the agent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6acba3c4-a90e-448b-bf51-2d341df551ee
📒 Files selected for processing (1)
cli/selftune/localdb/db.ts
- db.ts: move orphaned getMeta JSDoc to above the function - db.ts: migration errors now include "Run: selftune rebuild-db" guidance - bun.lock: regenerated clean (no duplicate telemetry-contract entries) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/localdb/db.ts`:
- Around line 84-89: closeSingleton currently calls _singletonDb.close() before
nulling _singletonDb, risking leaving a closed/invalid handle in place if
close() throws; change closeSingleton to remove the global reference first
(store _singletonDb to a local variable, set _singletonDb = null) and then call
close() inside a try/catch on that local variable so exceptions don't leave the
singleton non-null; reference functions/vars: closeSingleton, _singletonDb,
Database.close, getDb, and _setTestDb.
- Around line 47-60: openDb currently constructs new Database(dbPath) and runs
PRAGMA/DDL/migration/index steps without ensuring the DB is closed on error;
wrap the allocation and all subsequent setup (the new Database(dbPath) call plus
the PRAGMA/DDL loop over MIGRATIONS and the POST_MIGRATION_INDEXES loop) in a
try/catch/finally so that any thrown error causes db.close() in finally if db
was created; preserve the existing migration logic for duplicate-column and
rethrow other migration errors with the same actionable message, and for the
POST_MIGRATION_INDEXES loop catch db.run errors and rethrow a new Error like
"Schema index creation failed: <msg>. Run: selftune rebuild-db" (use the same
msg extraction code you use for migrations) so agent-facing guidance is
provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c36c7c07-b40b-482a-a7e0-8774e894a9fe
📒 Files selected for processing (1)
cli/selftune/localdb/db.ts
Comment on lines
+47
to
+60
| // Run migrations (ALTER TABLE ADD COLUMN — safe to re-run, fails silently if column exists) | ||
| for (const migration of MIGRATIONS) { | ||
| try { | ||
| db.run(migration); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (msg.includes("duplicate column")) continue; // expected on subsequent runs | ||
| throw new Error(`Schema migration failed: ${msg}. Run: selftune rebuild-db`); | ||
| } | ||
| } | ||
|
|
||
| // Create indexes that depend on migration columns | ||
| for (const idx of POST_MIGRATION_INDEXES) { | ||
| db.run(idx); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/selftune/localdb/db.ts | head -100Repository: selftune-dev/selftune
Length of output: 3829
🏁 Script executed:
# Check if there are other places where db.run() is called without error handling
rg "db\.run\(" cli/selftune/localdb/db.ts -A 2 -B 2Repository: selftune-dev/selftune
Length of output: 730
🏁 Script executed:
# Check if bun:sqlite Database.close() can throw
# Search for how it's used in tests or elsewhere
rg "\.close\(\)" cli/selftune/localdb/ -B 2 -A 2Repository: selftune-dev/selftune
Length of output: 579
🏁 Script executed:
# Check if there are tests or documentation about Database behavior
fd "schema.ts" cli/selftune/localdb/ | head -5Repository: selftune-dev/selftune
Length of output: 95
🏁 Script executed:
# Check the schema file to understand MIGRATIONS and POST_MIGRATION_INDEXES structure
head -50 cli/selftune/localdb/schema.tsRepository: selftune-dev/selftune
Length of output: 1516
🏁 Script executed:
# Search for MIGRATIONS and POST_MIGRATION_INDEXES definitions
rg "MIGRATIONS\s*=|POST_MIGRATION_INDEXES\s*=" cli/selftune/localdb/schema.ts -A 10Repository: selftune-dev/selftune
Length of output: 1001
Wrap openDb() in try-catch for cleanup, and make index errors actionable.
Once new Database(dbPath) succeeds, any error in PRAGMA/DDL/migration/index setup exits openDb() without closing the connection. Additionally, POST_MIGRATION_INDEXES at line 60 bubbles raw driver errors without the actionable guidance you added for migrations.
Suggested fix
const db = new Database(dbPath);
-
- // Enable WAL mode for better concurrent access
- db.run("PRAGMA journal_mode = WAL");
- db.run("PRAGMA foreign_keys = ON");
-
- // Run all DDL statements
- for (const ddl of ALL_DDL) {
- db.run(ddl);
- }
-
- // Run migrations (ALTER TABLE ADD COLUMN — safe to re-run, fails silently if column exists)
- for (const migration of MIGRATIONS) {
- try {
- db.run(migration);
- } catch (err) {
- const msg = err instanceof Error ? err.message : String(err);
- if (msg.includes("duplicate column")) continue; // expected on subsequent runs
- throw new Error(`Schema migration failed: ${msg}. Run: selftune rebuild-db`);
- }
- }
-
- // Create indexes that depend on migration columns
- for (const idx of POST_MIGRATION_INDEXES) {
- db.run(idx);
- }
-
- return db;
+ try {
+ db.run("PRAGMA journal_mode = WAL");
+ db.run("PRAGMA foreign_keys = ON");
+
+ for (const ddl of ALL_DDL) {
+ db.run(ddl);
+ }
+
+ for (const migration of MIGRATIONS) {
+ try {
+ db.run(migration);
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err);
+ if (msg.includes("duplicate column")) continue;
+ throw new Error(
+ `Schema migration failed. Run: selftune rebuild-db. Details: ${msg}`,
+ );
+ }
+ }
+
+ for (const idx of POST_MIGRATION_INDEXES) {
+ try {
+ db.run(idx);
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err);
+ throw new Error(
+ `Post-migration index creation failed. Run: selftune rebuild-db. Details: ${msg}`,
+ );
+ }
+ }
+
+ return db;
+ } catch (err) {
+ try {
+ db.close();
+ } catch {
+ // best-effort cleanup
+ }
+ throw err;
+ }Per coding guidelines: error messages must guide the agent, not the human, with actionable commands like selftune rebuild-db rather than raw stack traces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/db.ts` around lines 47 - 60, openDb currently constructs
new Database(dbPath) and runs PRAGMA/DDL/migration/index steps without ensuring
the DB is closed on error; wrap the allocation and all subsequent setup (the new
Database(dbPath) call plus the PRAGMA/DDL loop over MIGRATIONS and the
POST_MIGRATION_INDEXES loop) in a try/catch/finally so that any thrown error
causes db.close() in finally if db was created; preserve the existing migration
logic for duplicate-column and rethrow other migration errors with the same
actionable message, and for the POST_MIGRATION_INDEXES loop catch db.run errors
and rethrow a new Error like "Schema index creation failed: <msg>. Run: selftune
rebuild-db" (use the same msg extraction code you use for migrations) so
agent-facing guidance is provided.
Comment on lines
+84
to
+89
| export function closeSingleton(): void { | ||
| if (_singletonDb) { | ||
| _singletonDb.close(); | ||
| _singletonDb = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/selftune/localdb/db.tsRepository: selftune-dev/selftune
Length of output: 4466
Reset the singleton before calling close().
_setTestDb() already guards Database.close() with a try/catch, signaling it can throw. If closeSingleton() fails on close(), _singletonDb remains non-null, causing the next getDb() call to return the same possibly-closed handle instead of reopening.
Suggested fix
export function closeSingleton(): void {
- if (_singletonDb) {
- _singletonDb.close();
- _singletonDb = null;
- }
+ const db = _singletonDb;
+ _singletonDb = null;
+ db?.close();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/localdb/db.ts` around lines 84 - 89, closeSingleton currently
calls _singletonDb.close() before nulling _singletonDb, risking leaving a
closed/invalid handle in place if close() throws; change closeSingleton to
remove the global reference first (store _singletonDb to a local variable, set
_singletonDb = null) and then call close() inside a try/catch on that local
variable so exceptions don't leave the singleton non-null; reference
functions/vars: closeSingleton, _singletonDb, Database.close, getDb, and
_setTestDb.
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
Major refactor of the dashboard architecture with route extraction, SQLite-first migration, and auto-update mechanism. This simplifies dashboard-server.ts by moving route handlers to a dedicated routes/ directory, completes the SQLite materialization pipeline, and adds automatic update checking.
Key Changes
Testing
All tests updated and passing. New localdb tests verify read/write correctness.