refactor: SQLite-first cutover for all local read paths#66
Merged
WellDunDun merged 12 commits intomainfrom Mar 20, 2026
Merged
Conversation
.agents/ and skills-lock.json are dev tooling installed via `npx skills add`. They were being shipped to users via `npx skills add selftune-dev/selftune`, causing Reins to appear as part of the selftune skill. Now gitignored so only the intended selftune skill in skill/ is visible to consumers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the manual API key provisioning path (--alpha-key flag) from the CLI. Alpha enrollment now exclusively uses the device-code flow: browser opens automatically, user approves, credentials are provisioned and stored without any manual copy-paste. Also removes zod schemas from telemetry-contract — the cloud repo (gwangju-v1) already owns its own validation schemas in @selftune/shared. The OSS CLI only needs types and hand-written validators (zero deps). - Remove --alpha-key flag from init CLI and InitOptions interface - Remove direct-key code branch from runInit() - Update agent-guidance to reference device-code flow - Update flush.ts error messages - Update Initialize.md, Doctor.md, SKILL.md, alpha-remote-data-contract.md - Delete telemetry-contract/src/schemas.ts (zod dependency) - Remove zod from telemetry-contract package.json - Rewrite tests to mock device-code flow instead of using alphaKey - Add SELFTUNE_NO_BROWSER env var to suppress browser open in tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add canonical envelope columns (normalizer_version, capture_mode, raw_source_ref) to all 4 SQLite tables with migrations for existing DBs - Store envelope fields during direct-write inserts so SQLite records pass isCanonicalRecord() validation - Rewrite queryCanonicalRecordsForStaging() to reconstruct full contract-compliant records with session-level envelope fallback - Replace dashboard JSONL file watchers with WAL-based invalidation - Mark orchestrate signal consumption as SQLite-only (no JSONL rewrite) - Label remaining JSONL reads as test/custom-path fallbacks only - Update ARCHITECTURE.md, Dashboard.md, Doctor.md to reflect shipped state - Prune exec plans: 5 completed, 7 deferred, 4 remain active Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMakes SQLite the primary runtime read-store (WAL-driven SSE), removes local Alpha API key CLI support in favor of device-code/browser enrollment, extends local DB schema for canonical metadata, deletes Reins skill docs/lock, relaxes telemetry schema validations, and updates CLI, tests, and docs to the SQLite-first flows. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (selftune)
participant Auth as Alpha Auth Server
participant Browser as Browser/User
participant DB as Local DB (SQLite)
participant SSE as Dashboard SSE Server
CLI->>Auth: POST /device-code (request device + user codes)
Auth-->>CLI: {device_code, user_code, verification_uri}
CLI->>Browser: open verification_uri
Browser->>Auth: user approves
CLI->>Auth: poll /device-code/poll
Auth-->>CLI: approved {api_key, cloud_user_id, org_id}
CLI->>DB: persist api_key + cloud_user_id (write)
CLI->>DB: optionally stage canonical records
DB->>DB: WAL entries appended
SSE->>DB: fs.watchFile(wal) notices change
SSE->>Clients: broadcast SSE "update"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/selftune/hooks/evolution-guard.ts (1)
48-73: 🧹 Nitpick | 🔵 TrivialSQLite-first implementation is correct.
The removal of the try/catch fallback for the default path aligns with the SQLite-first cutover. One minor observation:
queryEvolutionAudit(db, skillName)already filters by skill name (per the relevant code snippet showing the SQL query), so the subsequent.filter((e) => e.skill_name === skillName)on line 68 is redundant. Not blocking, but could simplify.♻️ Optional: remove redundant filter
if (auditLogPath === EVOLUTION_AUDIT_LOG) { const { getDb } = await import("../localdb/db.js"); const { queryEvolutionAudit } = await import("../localdb/queries.js"); const db = getDb(); - entries = queryEvolutionAudit(db, skillName) as Array<{ + const skillEntries = queryEvolutionAudit(db, skillName) as Array<{ skill_name?: string; action: string; }>; + if (skillEntries.length === 0) return false; + const lastEntry = skillEntries[skillEntries.length - 1]; + return lastEntry.action === "deployed"; } else { // test/custom-path fallback entries = readJsonl<{ skill_name?: string; action: string }>(auditLogPath); } - - // 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]; - return lastEntry.action === "deployed"; + // JSONL path still needs filtering + const skillEntries = entries.filter((e) => e.skill_name === skillName); + if (skillEntries.length === 0) return false; + const lastEntry = skillEntries[skillEntries.length - 1]; + 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 48 - 73, The skill_name filter after querying the DB is redundant because queryEvolutionAudit(db, skillName) already returns only matching rows; in checkActiveMonitoring replace the post-query filtering (the entries.filter(...) that creates skillEntries) by using the entries array directly: check entries.length === 0, set lastEntry = entries[entries.length - 1], and return lastEntry.action === "deployed"; keep the JSONL fallback path unchanged (readJsonl still needs filtering for custom paths if those logs include other skills).tests/signal-orchestrate.test.ts (1)
211-254:⚠️ Potential issue | 🟡 MinorAdd DB cleanup or use isolated in-memory database.
This test writes to the singleton SQLite DB via
writeImprovementSignalToDb()but theafterEachhook only removes the temp directory—not the database. The sessionss1,s2,s3remain in the DB and will contaminate subsequent tests.Either:
- Call
_setTestDb(openDb(":memory:"))in abeforeEachor at test start, then reset inafterEach- Add explicit DB cleanup (clear the
improvement_signalstable for these session IDs) inafterEach- Use
expect().toHaveLength()with optional chaining on query results instead of non-null assertions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/signal-orchestrate.test.ts` around lines 211 - 254, This test mutates the shared SQLite DB and must isolate state; before running markSignalsConsumed call _setTestDb(openDb(":memory:")) (or in a beforeEach) to switch the singleton to an in-memory DB, then seed signals with writeImprovementSignalToDb and run the test; in afterEach reset/close the test DB (or call _setTestDb(null) / reopen the default) to avoid leaking rows in the improvement_signals table between tests. Use the _setTestDb and openDb helpers to create/reset the DB and ensure cleanup in afterEach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Around line 218-219: Update ARCHITECTURE.md to consistently reflect the
SQLite-first runtime and WAL-based invalidation (remove or rewrite any
JSONL-centric descriptions); search for and replace references to JSONL file
watchers/orchestrate behavior with SQLite/local DB terms and mention WAL-based
SSE invalidation; also, when you change any logging or schema references, update
the corresponding symbols/files: constants.ts, types.ts,
skill/references/logs.md, localdb/schema.ts, materialize.ts, direct-write.ts,
and queries.ts to keep code/docs in sync per the noted checklist.
In `@cli/selftune/activation-rules.ts`:
- Around line 118-125: The code currently uses the last array element of
auditEntries (from queryEvolutionAudit or readJsonl) as the "last" evolution
timestamp, but queryEvolutionAudit yields rows in descending timestamp order so
the last element is the oldest; replace any use of
auditEntries[auditEntries.length - 1] (and the similar usage later around the
stale-evolution rule) with logic that computes the newest timestamp explicitly
(e.g., take auditEntries[0] if you know the source is descending, or more
robustly compute the max timestamp via a reduce over auditEntries) so both
branches (when ctx.evolution_audit_log_path === EVOLUTION_AUDIT_LOG and when
using readJsonl) use the actual most recent timestamp.
In `@cli/selftune/agent-guidance.ts`:
- Around line 41-45: The guidance currently includes a concrete "--alpha-email
<email>" flag even when no real email is known; update buildGuidance calls that
use buildAlphaInitCommand (e.g., the "alpha_cloud_link_required" case and the
other two occurrences noted) so buildAlphaInitCommand only returns a
next_command containing the --alpha-email flag when options?.email is a
non-empty/valid string; otherwise return the runnable command without the
--alpha-email flag (so runInit can execute the suggested command safely).
In `@cli/selftune/alpha-upload/stage-canonical.ts`:
- Around line 153-156: queryCanonicalRecordsForStaging(db) currently causes
O(n²) behavior because code in cli/selftune/localdb/queries.ts does per-record
lookups with sessions.find(...) when enriching
prompts/invocations/execution_facts; fix by building a sessions map once
(sessionById) before the loops and use sessionById.get(sessionId) (or
sessionById[sessionId]) inside the prompt/invocation/execution_fact enrichment
loops instead of sessions.find, and update the enrichRecord/path that consumes
those lookups so queryCanonicalRecordsForStaging(db) returns the same
CanonicalRecord[] but with O(n) lookup performance.
In `@cli/selftune/dashboard-contract.ts`:
- Line 202: Update the runtime-footer type guard that checks record.watcher_mode
in the component (runtime-footer.tsx) to accept the new "wal" value: locate the
conditional that currently reads (record.watcher_mode === "jsonl" ||
record.watcher_mode === "none") and either add record.watcher_mode === "wal" to
that OR-list or remove the manual guard entirely so you rely on the updated
dashboard-contract type for validity; ensure any downstream logic that depended
on excluding other values still behaves correctly after allowing "wal".
In `@cli/selftune/dashboard-server.ts`:
- Around line 253-254: The unwatch currently removes all watchers because
unwatchFile(walPath) is called without the specific listener; change the stop
logic in startDashboardServer so it calls unwatchFile(walPath, onWALChange)
(i.e., pass the same onWALChange callback that was passed to watchFile) and only
flip walWatcherActive for this instance when you successfully unwatch; reference
the onWALChange callback, walWatcherActive flag, and startDashboardServer so the
watcher removal is instance-scoped and does not remove other dashboard server
watchers.
In `@cli/selftune/init.ts`:
- Around line 511-516: The fast-path in runInit() prevents the opts.alpha /
opts.noAlpha branches from running on already-initialized installs; modify
runInit() (or its call site) so the device-code branch controlled by opts.alpha
is evaluated before returning the existing-config fast path, or add an explicit
parameter/flag to runInit(e.g., forceAlphaCheck or allowAlphaOverride) that
disables the early return when opts.alpha or opts.noAlpha are present; ensure
the check for opts.alpha (and opts.noAlpha) in init.ts executes prior to any
early exit so the device-code flow (requestDeviceCode, grant handling) runs and
the subsequent cliMain() success event accurately reflects the performed action.
- Around line 546-553: The validatedAlphaIdentity construction is overwriting
stored alpha metadata with undefined when --alpha-email/--alpha-name are not
provided; update the assignment so email and display_name fallback to
existingAlphaBeforeOverwrite values instead of becoming undefined (e.g., email:
opts.alphaEmail ?? existingAlphaBeforeOverwrite?.email and display_name:
opts.alphaName ?? existingAlphaBeforeOverwrite?.display_name), keep user_id
logic using existingAlphaBeforeOverwrite?.user_id ?? generateUserId(), and
ensure consent_timestamp and enrolled handling remain unchanged so re-auth does
not clear prior metadata.
In `@cli/selftune/localdb/queries.ts`:
- Around line 591-730: In queryCanonicalRecordsForStaging, replace repeated
sessions.find(...) lookups with a prebuilt Map keyed by session_id: construct a
Map from the sessions array (e.g., const sessionById = new Map(sessions.map(s =>
[s.session_id, s])) ) right after sessions is populated, then use
sessionById.get(p.session_id) in the prompts loop and similarly use
sessionById.get(si.session_id) and sessionById.get(ef.session_id) in the
invocations and facts loops to avoid O(n) scans.
In `@cli/selftune/orchestrate.ts`:
- Around line 131-133: The loop over signals currently calls
updateSignalConsumed(signal.session_id, signal.query, signal.signal_type, runId)
and ignores its return value/errors, allowing failed writes to be silently
dropped; change this to await the updateSignalConsumed call, check its success
(or catch exceptions), and handle failures explicitly—e.g., log an error with
identifying values (session_id, query, signal_type, runId), optionally retry a
few times, and if retries fail either collect and surface the failed signal IDs
or throw to fail the run; ensure references to updateSignalConsumed, signals,
and runId are used in the error handling so failures are observable and not
silently ignored.
In `@skill/Workflows/Dashboard.md`:
- Around line 14-18: Update the "Live Updates (SSE)" section to remove
references to JSONL file watchers and explicitly state that live updates are
driven by SQLite WAL-based invalidation (Server-Sent Events) only; replace any
language about watching JSONL files with a description of WAL-triggered SSE
updates, keep the note that TanStack Query polling (60s) is the fallback, and
retain the `selftune export` mention solely as a debugging/offline JSONL export
tool rather than a mechanism for live updates.
In `@tests/alpha-upload/staging.test.ts`:
- Around line 711-718: The test currently uses non-null assertions (result! and
c.evolution_evidence!) which violates the tests guideline; instead, after the
initial existence checks for result and c.evolution_evidence add an explicit
guard (e.g., if (!result) return fail(...) or throw) so subsequent lines can
safely reference result.payload and c.evolution_evidence without using !; update
the assertions around payload (p) and any uses at lines ~735-737 to rely on that
guard and normal property access rather than optional chaining everywhere,
ensuring you reference the existing variables result, p, and
c.evolution_evidence when applying the guard.
In `@tests/signal-orchestrate.test.ts`:
- Around line 212-215: Replace the CommonJS requires for
writeImprovementSignalToDb, queryImprovementSignals, and getDb with top-level ES
module imports: add import statements for writeImprovementSignalToDb from
"../cli/selftune/localdb/direct-write.js", queryImprovementSignals from
"../cli/selftune/localdb/queries.js", and getDb from
"../cli/selftune/localdb/db.js" at the top of the test file and remove the
require(...) lines; keep using the imported symbols (writeImprovementSignalToDb,
queryImprovementSignals, getDb) unchanged in the test code.
- Around line 256-264: Two identical tests assert that markSignalsConsumed([],
runId) is a no-op; remove the duplicate to avoid redundant coverage. Delete one
of the tests (either the "handles empty pending signals" or "handles empty
pending signals (no-op)" block) in tests/signal-orchestrate.test.ts so only a
single test calls expect(() => markSignalsConsumed([], "run_123" or
"run_456")).not.toThrow(); keep the descriptive test name you prefer and ensure
markSignalsConsumed is still imported/used by the remaining test.
---
Outside diff comments:
In `@cli/selftune/hooks/evolution-guard.ts`:
- Around line 48-73: The skill_name filter after querying the DB is redundant
because queryEvolutionAudit(db, skillName) already returns only matching rows;
in checkActiveMonitoring replace the post-query filtering (the
entries.filter(...) that creates skillEntries) by using the entries array
directly: check entries.length === 0, set lastEntry = entries[entries.length -
1], and return lastEntry.action === "deployed"; keep the JSONL fallback path
unchanged (readJsonl still needs filtering for custom paths if those logs
include other skills).
In `@tests/signal-orchestrate.test.ts`:
- Around line 211-254: This test mutates the shared SQLite DB and must isolate
state; before running markSignalsConsumed call _setTestDb(openDb(":memory:"))
(or in a beforeEach) to switch the singleton to an in-memory DB, then seed
signals with writeImprovementSignalToDb and run the test; in afterEach
reset/close the test DB (or call _setTestDb(null) / reopen the default) to avoid
leaking rows in the improvement_signals table between tests. Use the _setTestDb
and openDb helpers to create/reset the DB and ensure cleanup in afterEach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d28db92-ed50-42e9-a273-16c5edbf5aac
📒 Files selected for processing (63)
.agents/skills/reins/HarnessMethodology.md.agents/skills/reins/SKILL.md.agents/skills/reins/Workflows/Audit.md.agents/skills/reins/Workflows/Doctor.md.agents/skills/reins/Workflows/Evolve.md.agents/skills/reins/Workflows/Scaffold.md.gitignoreAGENTS.mdARCHITECTURE.mdcli/selftune/activation-rules.tscli/selftune/agent-guidance.tscli/selftune/alpha-identity.tscli/selftune/alpha-upload/flush.tscli/selftune/alpha-upload/stage-canonical.tscli/selftune/contribute/bundle.tscli/selftune/dashboard-contract.tscli/selftune/dashboard-server.tscli/selftune/eval/hooks-to-evals.tscli/selftune/grading/auto-grade.tscli/selftune/grading/grade-session.tscli/selftune/hooks/auto-activate.tscli/selftune/hooks/evolution-guard.tscli/selftune/init.tscli/selftune/localdb/direct-write.tscli/selftune/localdb/queries.tscli/selftune/localdb/schema.tscli/selftune/monitoring/watch.tscli/selftune/observability.tscli/selftune/orchestrate.tscli/selftune/repair/skill-usage.tscli/selftune/sync.tsdocs/design-docs/alpha-remote-data-contract.mddocs/design-docs/live-dashboard-sse.mddocs/design-docs/sqlite-first-migration.mddocs/exec-plans/completed/dashboard-data-integrity-recovery.mddocs/exec-plans/completed/dashboard-signal-integration.mddocs/exec-plans/completed/local-sqlite-materialization.mddocs/exec-plans/completed/output-quality-loop-prereqs.mddocs/exec-plans/completed/telemetry-normalization.mddocs/exec-plans/deferred/advanced-skill-patterns-adoption.mddocs/exec-plans/deferred/cloud-auth-unification-for-alpha.mddocs/exec-plans/deferred/grader-prompt-evals.mddocs/exec-plans/deferred/mcp-tool-descriptions.mddocs/exec-plans/deferred/multi-agent-sandbox.mddocs/exec-plans/deferred/phase-d-marginal-case-review-spike.mddocs/exec-plans/deferred/user-owned-session-data-and-org-visible-outcomes.mdpackage.jsonpackages/telemetry-contract/package.jsonpackages/telemetry-contract/src/index.tspackages/telemetry-contract/src/schemas.tsskill/SKILL.mdskill/Workflows/Dashboard.mdskill/Workflows/Doctor.mdskill/Workflows/Initialize.mdskill/references/logs.mdskills-lock.jsontests/alpha-upload/staging.test.tstests/alpha-upload/status.test.tstests/init/alpha-consent.test.tstests/init/alpha-onboarding-e2e.test.tstests/observability.test.tstests/signal-orchestrate.test.tstests/trust-floor/health.test.ts
💤 Files with no reviewable changes (10)
- packages/telemetry-contract/src/index.ts
- skills-lock.json
- packages/telemetry-contract/package.json
- .agents/skills/reins/Workflows/Doctor.md
- .agents/skills/reins/Workflows/Scaffold.md
- .agents/skills/reins/HarnessMethodology.md
- .agents/skills/reins/Workflows/Evolve.md
- .agents/skills/reins/SKILL.md
- .agents/skills/reins/Workflows/Audit.md
- packages/telemetry-contract/src/schemas.ts
Comment on lines
+218
to
+219
| The dashboard uses WAL-based invalidation for SSE live updates — JSONL file | ||
| watchers have been removed from the dashboard server. |
There was a problem hiding this comment.
Architecture doc is still internally inconsistent on runtime data paths.
Line 218–Line 219 correctly states JSONL dashboard watchers were removed, but other sections still describe JSONL-centric orchestrate/runtime behavior. Please align those remaining sections so the document consistently reflects SQLite-first reads/writes.
Based on learnings: "When changing JSONL log schema or adding a new log file, update constants.ts, types.ts, skill/references/logs.md, and all database-related files (localdb/schema.ts, materialize.ts, direct-write.ts, queries.ts), plus ARCHITECTURE.md data architecture".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ARCHITECTURE.md` around lines 218 - 219, Update ARCHITECTURE.md to
consistently reflect the SQLite-first runtime and WAL-based invalidation (remove
or rewrite any JSONL-centric descriptions); search for and replace references to
JSONL file watchers/orchestrate behavior with SQLite/local DB terms and mention
WAL-based SSE invalidation; also, when you change any logging or schema
references, update the corresponding symbols/files: constants.ts, types.ts,
skill/references/logs.md, localdb/schema.ts, materialize.ts, direct-write.ts,
and queries.ts to keep code/docs in sync per the noted checklist.
tests/alpha-upload/staging.test.ts
Outdated
Comment on lines
+711
to
+718
| const p = result!.payload; | ||
| // Envelope fields | ||
| expect(p.schema_version).toBe("2.0"); | ||
| expect(typeof p.client_version).toBe("string"); | ||
| expect(p.push_id).toMatch( | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/, | ||
| ); | ||
| expect(typeof p.normalizer_version).toBe("string"); |
There was a problem hiding this comment.
Avoid non-null assertions in this Bun test.
result! and c.evolution_evidence! are exactly the pattern the test guideline asks us not to use here. Add an explicit guard after the existence checks so the rest of the assertions narrow naturally.
Suggested fix
- const p = result!.payload;
+ if (!result) throw new Error("expected staged payload");
+ const p = result.payload;
@@
- const ev = c.evolution_evidence![0];
+ const ev = c.evolution_evidence?.[0];
+ expect(ev).toBeDefined();As per coding guidelines, tests/**/*.ts should use optional chaining (result?.) and avoid non-null assertions (result!).
Also applies to: 735-737
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/alpha-upload/staging.test.ts` around lines 711 - 718, The test
currently uses non-null assertions (result! and c.evolution_evidence!) which
violates the tests guideline; instead, after the initial existence checks for
result and c.evolution_evidence add an explicit guard (e.g., if (!result) return
fail(...) or throw) so subsequent lines can safely reference result.payload and
c.evolution_evidence without using !; update the assertions around payload (p)
and any uses at lines ~735-737 to rely on that guard and normal property access
rather than optional chaining everywhere, ensuring you reference the existing
variables result, p, and c.evolution_evidence when applying the guard.
- Fix stale-evolution rule using oldest instead of newest audit entry (queryEvolutionAudit returns DESC order, so use [0] not [length-1]) - Fix agent guidance emitting placeholder email in next_command - Add "wal" to runtime-footer type guard in dashboard SPA - Pass specific listener to unwatchFile() to avoid removing all watchers - Preserve existing alpha identity metadata on re-auth without flags - Let alpha mutations bypass existing-config fast path in runInit() - Replace O(n²) sessions.find() with Map lookup in staging query - Log failed signal consumed writes in orchestrate - Fix Dashboard.md SSE section still referencing JSONL watchers - Remove non-null assertions in staging test - Convert require() to ESM imports in signal-orchestrate test - Remove duplicate empty-signals test case Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused readJsonl and queryImprovementSignals imports - Sort imports alphabetically per biome organizeImports rule - Format function signatures and long lines per biome formatter - Use template literal for WAL path concatenation - Update formatAlphaStatus tests for new guidance without placeholder email - Remove trailing blank line from duplicate test removal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Writes canonical records directly into SQLite tables (simulating the runtime hook path), then calls stageCanonicalRecords(db) with default args to exercise the SQLite read path. Verifies all 4 record kinds are staged and envelope fields survive the round-trip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…taging test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/signal-orchestrate.test.ts (1)
214-240:⚠️ Potential issue | 🟠 MajorIsolate this SQLite-backed test from shared DB state.
getDb()is a process-wide singleton, andwriteImprovementSignalToDb()usesINSERT OR IGNORE. With fixed keys (s1/s2/s3plus fixed timestamps), this setup can silently reuse existing rows unlessimprovement_signalsis cleared elsewhere, which makes the assertions order-dependent. Seed unique keys per test or clear those rows in setup/teardown. As per coding guidelines,tests/**/*.ts: "Isolated tests with proper setup/teardown".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/signal-orchestrate.test.ts` around lines 214 - 240, The test is reusing process-wide SQLite state so fixed keys/timestamps can collide; before seeding call getDb() and either delete existing rows from the improvement_signals table for the session_ids used or generate unique session_id/timestamp values per test, then use writeImprovementSignalToDb(...) to insert and markSignalsConsumed(...) to consume; specifically update the test to call getDb().exec("DELETE FROM improvement_signals WHERE session_id IN (...)") or produce unique session_ids (not "s1"/"s2"/"s3") so the assertions on improvement_signals reflect only this test's rows.
♻️ Duplicate comments (3)
cli/selftune/activation-rules.ts (1)
135-136:⚠️ Potential issue | 🟠 MajorCompute the newest audit timestamp explicitly here.
auditEntries[0]is only correct for the SQLite branch becausequeryEvolutionAudit()sortsDESC. The JSONL fallback keeps file order, so test/custom-path overrides can still read the wrong entry and falsely triggerstale-evolution. Reduce the entries to the max finite parsed timestamp before computingageMs.Suggested fix
- const lastEntry = auditEntries[0]; // queryEvolutionAudit returns DESC order - const lastTimestamp = new Date(lastEntry.timestamp).getTime(); + const lastTimestamp = auditEntries.reduce((max, entry) => { + const ts = Date.parse(entry.timestamp); + return Number.isFinite(ts) && ts > max ? ts : max; + }, Number.NEGATIVE_INFINITY); + if (!Number.isFinite(lastTimestamp)) return null;🤖 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 135 - 136, The code currently assumes auditEntries[0] is the newest entry; instead iterate/reduce auditEntries to parse each entry.timestamp into a finite number and take the maximum to produce lastTimestamp (then compute ageMs). Update the logic around variables auditEntries, lastEntry/lastTimestamp and where ageMs is computed (the consumer of queryEvolutionAudit()) to use the reduced max timestamp so JSONL (file-order) and SQLite both yield the correct newest audit time.cli/selftune/agent-guidance.ts (1)
8-10:⚠️ Potential issue | 🟠 MajorKeep
next_commandfree of raw email values.
next_commandis a machine-executable shell string, and this change now splices stored user input into it without quoting or escaping. A malformed email can turn the guidance into an invalid command or inject extra shell tokens. Since--alpha-emailis optional, the safer fix is to omit it fromnext_command.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; the CLI is the agent's API." Also, "Error messages should guide the agent, not the human (e.g., suggest the next CLI command, not 'check the docs')."Suggested fix
function buildAlphaInitCommand(options?: { - email?: string; force?: boolean; }): string { const parts = ["selftune", "init", "--alpha"]; - if (options?.email?.trim()) { - parts.push("--alpha-email", options.email); - } if (options?.force) { parts.push("--force"); } return parts.join(" "); }- buildAlphaInitCommand({ email: options?.email }), + buildAlphaInitCommand(), ... - buildAlphaInitCommand({ email: options?.email, force: true }), + buildAlphaInitCommand({ force: true }), ... - buildAlphaInitCommand({ email: options?.email, force: true }), + buildAlphaInitCommand({ force: true }),Also applies to: 42-43, 50-51, 58-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/agent-guidance.ts` around lines 8 - 10, The constructed shell token list 'parts' (used to form next_command) must not include raw user email values; remove the push of "--alpha-email" and options.email into parts and ensure any use of options.email is omitted from next_command generation (leave the email flag out entirely when building parts/next_command), while preserving other flags; update code paths that push email (references to parts, options?.email, and next_command) so next_command never contains the unescaped user-provided email.cli/selftune/init.ts (1)
472-475:⚠️ Potential issue | 🟠 MajorReject or apply standalone alpha metadata flags consistently.
--alpha-emailand--alpha-namenow bypass the existing-config fast path, butrunInit()only uses them inside theopts.alphabranch. That makesselftune init --alpha-name Boba silent no-op rewrite, andcliMain()still disagrees withrunInit()on empty-string values. Either require--alphawhen those flags are present or merge them into the restoredAlphaIdentity, and keep the mutation predicate identical in both places.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; the CLI is the agent's API."
Also applies to: 650-655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/init.ts` around lines 472 - 475, The fast-path predicate in cliMain() uses hasAlphaMutation (opts.alpha || opts.noAlpha || opts.alphaEmail !== undefined || opts.alphaName !== undefined) but runInit() only applies alphaEmail/alphaName when opts.alpha is true, causing silent no-ops; fix by making behavior consistent: either (A) require opts.alpha when opts.alphaEmail or opts.alphaName are set (validate and error early in cliMain()/parse stage), or (B) if restoring an AlphaIdentity in runInit(), merge opts.alphaEmail/opts.alphaName into the restored AlphaIdentity even when opts.alpha is false so the mutation predicate matches runInit(); update the logic around configPath existence, hasAlphaMutation, and the restoration code in runInit()/AlphaIdentity handling to use the same predicate and ensure empty-string values are honored consistently.
🤖 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/activation-rules.ts`:
- Around line 29-35: Wrap all SQLite access points (the getDb() and query*()
usages that branch on ctx.query_log_path === QUERY_LOG) in try/catch blocks so
DB errors don't propagate; on failure return the same safe fallback as the JSONL
path (either null for single-session reads or an empty array for lists) instead
of throwing. Specifically modify the blocks around the existing query reads (the
sequence using getDb() and queryQueryLog), the similar reads referenced at lines
42-50, and the reads around lines 118-125 to catch any exceptions from getDb()
or the query* functions and return null/[] accordingly so activation rules fail
open. Ensure you keep existing variable shapes (e.g., queries: Array<{
session_id: string; query: string }>) and only change error handling.
In `@cli/selftune/localdb/queries.ts`:
- Around line 644-647: The nullish-coalescing chain used to compute
raw_source_ref is misformatted; update the expression in the construction that
assigns raw_source_ref so it conforms to Biome styling (place each
operand/operator on its own properly indented line or wrap the chain in
parentheses) — locate the assignment that calls safeParseJson(p.raw_source_ref
as string | null) ?? safeParseJson(sessionEnvelope?.raw_source_ref as string |
null) ?? undefined and reformat it (alongside the nearby source_session_kind
line) so Biome formatter accepts it, ensuring references to safeParseJson,
raw_source_ref, and sessionEnvelope remain unchanged.
- Around line 696-727: While rebuilding execution_fact records in the loop that
iterates over facts (from the SELECT in queries.ts), include and preserve the
original execution_fact id by adding an execution_fact_id field set to ef.id
when pushing into records; update the records.push call (the object created for
"execution_fact") to emit execution_fact_id: ef.id so downstream consumers
retain the canonical ID instead of generating a new one.
In `@cli/selftune/orchestrate.ts`:
- Around line 131-138: The loop's check relies on
updateSignalConsumed(session_id, query, signal_type, runId) but that function
currently discards the .run() result inside safeWrite (callback returns void),
so zero-row UPDATEs are treated as success; change updateSignalConsumed to
return the actual affected-row count (or a boolean derived from it) by making
the safeWrite callback return the .run() result and then propagate
result.changes (or result.changes > 0) back to the caller; update callers (the
for loop expecting ok) to handle the new return type if needed so a zero-change
update yields false and triggers the error log.
In `@tests/alpha-upload/staging.test.ts`:
- Around line 716-718: Reformat the Jest assertion
expect(p.push_id).toMatch(...) to match Biome's formatting rules: adjust
whitespace and line breaks so the call and its regex argument follow Biome style
(e.g., place the .toMatch call and its regex argument on a single line or
otherwise match your project's Biome output); update the assertion around
expect, .toMatch and the regex literal so the formatter no longer reports drift
in the test containing expect(p.push_id).toMatch.
In `@tests/signal-orchestrate.test.ts`:
- Around line 14-17: Remove the two unused imports readJsonl and
queryImprovementSignals from the import block so only the used symbols
(writeImprovementSignalToDb and getDb) are imported; locate the import statement
that currently lists readJsonl, writeImprovementSignalToDb,
queryImprovementSignals, and getDb and edit it to drop readJsonl and
queryImprovementSignals to satisfy the linter/biome errors.
---
Outside diff comments:
In `@tests/signal-orchestrate.test.ts`:
- Around line 214-240: The test is reusing process-wide SQLite state so fixed
keys/timestamps can collide; before seeding call getDb() and either delete
existing rows from the improvement_signals table for the session_ids used or
generate unique session_id/timestamp values per test, then use
writeImprovementSignalToDb(...) to insert and markSignalsConsumed(...) to
consume; specifically update the test to call getDb().exec("DELETE FROM
improvement_signals WHERE session_id IN (...)") or produce unique session_ids
(not "s1"/"s2"/"s3") so the assertions on improvement_signals reflect only this
test's rows.
---
Duplicate comments:
In `@cli/selftune/activation-rules.ts`:
- Around line 135-136: The code currently assumes auditEntries[0] is the newest
entry; instead iterate/reduce auditEntries to parse each entry.timestamp into a
finite number and take the maximum to produce lastTimestamp (then compute
ageMs). Update the logic around variables auditEntries, lastEntry/lastTimestamp
and where ageMs is computed (the consumer of queryEvolutionAudit()) to use the
reduced max timestamp so JSONL (file-order) and SQLite both yield the correct
newest audit time.
In `@cli/selftune/agent-guidance.ts`:
- Around line 8-10: The constructed shell token list 'parts' (used to form
next_command) must not include raw user email values; remove the push of
"--alpha-email" and options.email into parts and ensure any use of options.email
is omitted from next_command generation (leave the email flag out entirely when
building parts/next_command), while preserving other flags; update code paths
that push email (references to parts, options?.email, and next_command) so
next_command never contains the unescaped user-provided email.
In `@cli/selftune/init.ts`:
- Around line 472-475: The fast-path predicate in cliMain() uses
hasAlphaMutation (opts.alpha || opts.noAlpha || opts.alphaEmail !== undefined ||
opts.alphaName !== undefined) but runInit() only applies alphaEmail/alphaName
when opts.alpha is true, causing silent no-ops; fix by making behavior
consistent: either (A) require opts.alpha when opts.alphaEmail or opts.alphaName
are set (validate and error early in cliMain()/parse stage), or (B) if restoring
an AlphaIdentity in runInit(), merge opts.alphaEmail/opts.alphaName into the
restored AlphaIdentity even when opts.alpha is false so the mutation predicate
matches runInit(); update the logic around configPath existence,
hasAlphaMutation, and the restoration code in runInit()/AlphaIdentity handling
to use the same predicate and ensure empty-string values are honored
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 895e507a-53e5-4a22-96e3-f9d08f18bf06
📒 Files selected for processing (10)
apps/local-dashboard/src/components/runtime-footer.tsxcli/selftune/activation-rules.tscli/selftune/agent-guidance.tscli/selftune/dashboard-server.tscli/selftune/init.tscli/selftune/localdb/queries.tscli/selftune/orchestrate.tsskill/Workflows/Dashboard.mdtests/alpha-upload/staging.test.tstests/signal-orchestrate.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/signal-orchestrate.test.ts (1)
216-240:⚠️ Potential issue | 🟡 MinorMake this DB test isolated from global SQLite state.
This test writes fixed
session_idvalues (s1,s2,s3) into a singleton DB, so prior rows can make outcomes order-dependent. Use per-test unique IDs (or explicit row cleanup) before assertions.♻️ Proposed refactor
test("marks matching signals as consumed", () => { + const suffix = `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const s1 = `s1-${suffix}`; + const s2 = `s2-${suffix}`; + const s3 = `s3-${suffix}`; + const signals = [ - makeSignal({ timestamp: "2025-01-01T00:00:00Z", session_id: "s1", mentioned_skill: "A" }), - makeSignal({ timestamp: "2025-01-01T00:01:00Z", session_id: "s2", mentioned_skill: "B" }), + makeSignal({ timestamp: "2025-01-01T00:00:00Z", session_id: s1, mentioned_skill: "A" }), + makeSignal({ timestamp: "2025-01-01T00:01:00Z", session_id: s2, mentioned_skill: "B" }), makeSignal({ timestamp: "2025-01-01T00:02:00Z", - session_id: "s3", + session_id: s3, consumed: true, consumed_at: "2025-01-01T00:05:00Z", consumed_by_run: "old_run", }), ]; @@ const updated = db .query( - "SELECT * FROM improvement_signals WHERE session_id IN ('s1','s2','s3') ORDER BY timestamp ASC", + "SELECT * FROM improvement_signals WHERE session_id IN (?, ?, ?) ORDER BY timestamp ASC", ) - .all(); + .all(s1, s2, s3);As per coding guidelines:
tests/**/*.tsshould use isolated tests with proper setup/teardown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/signal-orchestrate.test.ts` around lines 216 - 240, The test mutates the global singleton DB by inserting signals with fixed session_ids ("s1","s2","s3"), making results order-dependent; change the test to isolate DB state by either generating unique session_ids per test (e.g., via a UUID or timestamp when calling makeSignal) or by cleaning up rows before assertions (delete from improvement_signals where session_id IN (...) ) so prior runs don't affect results; locate the inserts via makeSignal and writeImprovementSignalToDb, ensure markSignalsConsumed is called only on the intended pendingSignals, and use getDb to run any required cleanup or to create a fresh in-memory DB for the test before querying improvement_signals.cli/selftune/observability.ts (1)
199-208: 🧹 Nitpick | 🔵 TrivialHealth check performs no actual verification.
checkDashboardIntegrityHealth()now returns a static"pass"without verifying the database or WAL file exists. Consider adding minimal validation:Suggested enhancement
export function checkDashboardIntegrityHealth(): HealthCheck[] { + const walPath = `${DB_PATH}-wal`; const check: HealthCheck = { name: "dashboard_freshness_mode", path: DB_PATH, - status: "pass", - message: "Dashboard reads SQLite and watches WAL for live updates", + status: existsSync(DB_PATH) ? "pass" : "warn", + message: existsSync(DB_PATH) + ? `Dashboard reads SQLite and watches WAL for live updates` + : "SQLite database not found; run selftune init", }; return [check]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/observability.ts` around lines 199 - 208, checkDashboardIntegrityHealth currently returns a static "pass" without verifying the underlying files; update it to perform minimal validation by checking that the SQLite DB file (DB_PATH) exists and, if WAL mode is expected, that the corresponding WAL file (DB_PATH + "-wal") exists and is readable; set the HealthCheck.status to "pass" only when these checks succeed, otherwise set "fail" (or "warn") and populate HealthCheck.message with a concise error description; use the existing function name checkDashboardIntegrityHealth and the DB_PATH constant to locate and implement the checks.tests/alpha-upload/status.test.ts (1)
241-246:⚠️ Potential issue | 🟡 MinorMake the
not enrolledcommand assertion exact.This still passes if the guidance regresses to
selftune init --alpha --force, because the substringselftune init --alphais present there too. Assert the full line, or at least add a negative check for--force, so this state stays distinct from the recovery cases.✅ Proposed fix
const output = formatAlphaStatus(null); expect(output).toContain("not enrolled"); - expect(output).toContain("Next command"); - expect(output).toContain("selftune init --alpha"); + expect(output).toContain("Next command: selftune init --alpha"); + expect(output).not.toContain("selftune init --alpha --force"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/alpha-upload/status.test.ts` around lines 241 - 246, The test "returns 'not enrolled' line when not enrolled" uses formatAlphaStatus(null) but only checks for a substring so it would still pass if the guidance becomes "selftune init --alpha --force"; update the assertions in that test to assert the full expected line (e.g., exact match for the guidance line returned by formatAlphaStatus) or add a negative assertion that the output does not contain "--force": reference the test name and the formatAlphaStatus call to locate the assertion and replace the loose expect(output).toContain("selftune init --alpha") with an equality/line-match assertion or add expect(output).not.toContain("--force") to keep the not-enrolled state distinct from recovery cases.
♻️ Duplicate comments (2)
cli/selftune/orchestrate.ts (1)
129-133:⚠️ Potential issue | 🟡 Minor
markSignalsConsumedcannot detect zero-row updates.At Line 129,
okis expected to represent “signal marked consumed”, butupdateSignalConsumed()currently returnssafeWrite(...)success even whenUPDATE ...affects 0 rows, so the error branch at Line 130-133 is effectively unreachable for no-op updates.♻️ Proposed fix (in
cli/selftune/localdb/direct-write.ts)export function updateSignalConsumed( sessionId: string, query: string, signalType: string, runId: string, ): boolean { - return safeWrite("signal-consumed", (db) => { - getStmt( + return safeWrite("signal-consumed", (db) => { + const result = getStmt( db, "signal-consumed", ` UPDATE improvement_signals SET consumed = 1, consumed_at = ?, consumed_by_run = ? WHERE session_id = ? AND query = ? AND signal_type = ? AND consumed = 0 `, - ).run(new Date().toISOString(), runId, sessionId, query, signalType); + ).run(new Date().toISOString(), runId, sessionId, query, signalType); + return result.changes > 0; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/orchestrate.ts` around lines 129 - 133, updateSignalConsumed currently treats safeWrite success as "consumed" even when the UPDATE affects 0 rows, so change updateSignalConsumed (in cli/selftune/localdb/direct-write.ts) to check the underlying write result (e.g., the returned result.changes / rowsAffected value from safeWrite or the DB run() response) and return false when 0 rows were updated; ensure orchestrate.ts's call to updateSignalConsumed (used when marking signals consumed) uses that boolean to trigger the error branch appropriately.cli/selftune/localdb/queries.ts (1)
696-730:⚠️ Potential issue | 🟠 MajorKeep
execution_fact_idwhen rebuilding canonical execution facts.This query reads
id, but the reconstructed record never emitsexecution_fact_id. On the SQLite-first staging path,stageCanonicalRecords()then has to synthesize a new ID instead of reusing the row's canonical identity, which breaks stable dedupe/cursor behavior for execution facts.🧩 Proposed fix
- `SELECT id, session_id, occurred_at, prompt_id, tool_calls_json, total_tool_calls, + `SELECT id AS execution_fact_id, session_id, occurred_at, prompt_id, tool_calls_json, total_tool_calls, assistant_turns, errors_encountered, input_tokens, output_tokens, duration_ms, completion_status, schema_version, platform, normalized_at, normalizer_version, capture_mode, raw_source_ref FROM execution_facts ORDER BY occurred_at`, @@ raw_source_ref: safeParseJson(ef.raw_source_ref as string | null) ?? safeParseJson(sessionEnvelope?.raw_source_ref as string | null) ?? undefined, source_session_kind: sessionEnvelope?.source_session_kind ?? undefined, session_id: ef.session_id, + execution_fact_id: ef.execution_fact_id, occurred_at: ef.occurred_at, prompt_id: ef.prompt_id ?? undefined,🤖 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 696 - 730, The reconstructed execution_fact record omits the original row id, so preserve the canonical execution_fact_id by adding an execution_fact_id property to the object pushed into records (use ef.id as the value); update the object created in the loop (where records.push({ ... })) to include execution_fact_id: ef.id ?? undefined so downstream stageCanonicalRecords() can reuse the original ID for stable dedupe/cursors.
🤖 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/agent-guidance.ts`:
- Around line 4-7: The buildAlphaInitCommand function is currently appending
options.email raw into the generated command which can inject extra tokens;
instead trim the input and validate it is a single safe email token (e.g., no
whitespace or shell metacharacters and matches a basic email pattern) before
adding it to parts; if it fails validation or contains unsafe characters, do not
push the --alpha-email flag at all (omit it) so the composed command cannot be
altered by malicious/invalid input (reference: buildAlphaInitCommand and
options.email).
In `@cli/selftune/init.ts`:
- Around line 472-475: The fast-path currently treats
opts.alphaEmail/opts.alphaName as mutations via hasAlphaMutation but the flags
are only applied when opts.alpha is true, so add an early validation before the
fast-path (near where hasAlphaMutation is computed) that rejects or exits if
(opts.alphaEmail !== undefined || opts.alphaName !== undefined) && !opts.alpha
with a clear error message; update both places (the earlier fast-path check
around hasAlphaMutation and the later block around lines 652-656) to perform
this same validation so standalone --alpha-email / --alpha-name cannot silently
bypass cached config.
- Around line 547-559: The code assigns result.api_key and other fields directly
into validatedAlphaIdentity after pollDeviceCode; add the same format/validation
used by checkAlphaReadiness to verify the approval payload (at least validate
api_key format and required fields cloud_user_id/org_id) before persisting or
emitting readiness. In practice, call the existing validator (or inline the same
checks) on result and abort/throw (and do not set validatedAlphaIdentity or emit
alpha_upload_ready) if the payload is malformed so broken credentials are never
written; update the block around pollDeviceCode, validatedAlphaIdentity and any
subsequent readiness emission to gate on that validation.
In `@tests/alpha-upload/staging.test.ts`:
- Around line 876-878: The test uses a loose assertion on
stageCanonicalRecords(db); change the assertion to require an exact count so
accidental duplicates fail the test: replace
expect(staged).toBeGreaterThanOrEqual(4) with expect(staged).toBe(4) (reference:
stageCanonicalRecords and the staged variable in this fixture) and update the
inline comment to state that exactly four canonical source rows are expected.
---
Outside diff comments:
In `@cli/selftune/observability.ts`:
- Around line 199-208: checkDashboardIntegrityHealth currently returns a static
"pass" without verifying the underlying files; update it to perform minimal
validation by checking that the SQLite DB file (DB_PATH) exists and, if WAL mode
is expected, that the corresponding WAL file (DB_PATH + "-wal") exists and is
readable; set the HealthCheck.status to "pass" only when these checks succeed,
otherwise set "fail" (or "warn") and populate HealthCheck.message with a concise
error description; use the existing function name checkDashboardIntegrityHealth
and the DB_PATH constant to locate and implement the checks.
In `@tests/alpha-upload/status.test.ts`:
- Around line 241-246: The test "returns 'not enrolled' line when not enrolled"
uses formatAlphaStatus(null) but only checks for a substring so it would still
pass if the guidance becomes "selftune init --alpha --force"; update the
assertions in that test to assert the full expected line (e.g., exact match for
the guidance line returned by formatAlphaStatus) or add a negative assertion
that the output does not contain "--force": reference the test name and the
formatAlphaStatus call to locate the assertion and replace the loose
expect(output).toContain("selftune init --alpha") with an equality/line-match
assertion or add expect(output).not.toContain("--force") to keep the
not-enrolled state distinct from recovery cases.
In `@tests/signal-orchestrate.test.ts`:
- Around line 216-240: The test mutates the global singleton DB by inserting
signals with fixed session_ids ("s1","s2","s3"), making results order-dependent;
change the test to isolate DB state by either generating unique session_ids per
test (e.g., via a UUID or timestamp when calling makeSignal) or by cleaning up
rows before assertions (delete from improvement_signals where session_id IN
(...) ) so prior runs don't affect results; locate the inserts via makeSignal
and writeImprovementSignalToDb, ensure markSignalsConsumed is called only on the
intended pendingSignals, and use getDb to run any required cleanup or to create
a fresh in-memory DB for the test before querying improvement_signals.
---
Duplicate comments:
In `@cli/selftune/localdb/queries.ts`:
- Around line 696-730: The reconstructed execution_fact record omits the
original row id, so preserve the canonical execution_fact_id by adding an
execution_fact_id property to the object pushed into records (use ef.id as the
value); update the object created in the loop (where records.push({ ... })) to
include execution_fact_id: ef.id ?? undefined so downstream
stageCanonicalRecords() can reuse the original ID for stable dedupe/cursors.
In `@cli/selftune/orchestrate.ts`:
- Around line 129-133: updateSignalConsumed currently treats safeWrite success
as "consumed" even when the UPDATE affects 0 rows, so change
updateSignalConsumed (in cli/selftune/localdb/direct-write.ts) to check the
underlying write result (e.g., the returned result.changes / rowsAffected value
from safeWrite or the DB run() response) and return false when 0 rows were
updated; ensure orchestrate.ts's call to updateSignalConsumed (used when marking
signals consumed) uses that boolean to trigger the error branch appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c68a0ed-0536-4712-a164-f0e294ab9c54
📒 Files selected for processing (9)
cli/selftune/agent-guidance.tscli/selftune/dashboard-server.tscli/selftune/init.tscli/selftune/localdb/queries.tscli/selftune/observability.tscli/selftune/orchestrate.tstests/alpha-upload/staging.test.tstests/alpha-upload/status.test.tstests/signal-orchestrate.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/selftune/localdb/direct-write.ts (2)
388-405:⚠️ Potential issue | 🟠 MajorConsume the specific signal row, not every matching query tuple.
This
UPDATEignorestimestamp, so one call will mark every unconsumed(session_id, query, signal_type)row as consumed. That can silently eat duplicate signals or a same-query signal that arrived after the run read its snapshot. Threadtimestamp(or a stable row id) throughmarkSignalsConsumed()and include it in the predicate.Suggested fix
-export function updateSignalConsumed( - sessionId: string, +export function updateSignalConsumed( + timestamp: string, + sessionId: string, query: string, signalType: string, runId: string, ): boolean { const result = safeWriteResult("signal-consumed", (db) => getStmt( db, "signal-consumed", ` UPDATE improvement_signals SET consumed = 1, consumed_at = ?, consumed_by_run = ? - WHERE session_id = ? AND query = ? AND signal_type = ? AND consumed = 0 + WHERE timestamp = ? AND session_id = ? AND query = ? AND signal_type = ? AND consumed = 0 `, - ).run(new Date().toISOString(), runId, sessionId, query, signalType), + ).run(new Date().toISOString(), runId, timestamp, sessionId, query, signalType), ); return result?.changes > 0; }// cli/selftune/orchestrate.ts const ok = updateSignalConsumed( signal.timestamp, signal.session_id, signal.query, signal.signal_type, runId, );🤖 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 388 - 405, The UPDATE currently marks all rows matching (session_id, query, signal_type) because timestamp isn't part of the WHERE; change the API and callers to consume a single signal row by adding a timestamp (or stable row id) parameter to updateSignalConsumed and using it in the WHERE clause (e.g. WHERE session_id = ? AND query = ? AND signal_type = ? AND timestamp = ? AND consumed = 0) so only that exact row is updated; also thread the new timestamp argument through markSignalsConsumed (orchestrate.ts caller) when calling updateSignalConsumed and pass it into the prepared statement so duplicates or later-arriving same-query signals are not accidentally consumed.
410-456:⚠️ Potential issue | 🟠 MajorSQLite-first staging is still not at canonical-field parity.
queryCanonicalRecordsForStaging()now rebuilds uploads from SQLite, but these writers still persist only a subset of the canonical records. Fields this codebase already builds today—e.g. session fields likeexternal_session_id/repo_root/commit_sha/permission_mode/approval_policy/sandbox_policy/provider/end_reason, prompt fields likeprompt_hash/parent_prompt_id/source_message_id, skill-invocation fields likeskill_version_hash/tool_call_id, and execution-fact fields likebash_commands_redacted/end_reason—never reach SQLite here, so they disappear from default-path uploads after this cutover.Also applies to: 459-483, 486-539, 542-572
🤖 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 410 - 456, The insertSession writer (function insertSession) is missing many canonical session fields (e.g. external_session_id, repo_root, commit_sha, permission_mode, approval_policy, sandbox_policy, provider, end_reason) so those values are never persisted to SQLite; update the INSERT column list and VALUES binding in insertSession to include these missing fields (and add them to the ON CONFLICT DO UPDATE clause with appropriate COALESCE/excluded logic), ensuring any complex objects are JSON.stringified like raw_source_ref; apply the same change pattern to the other writers referenced around lines 459-483, 486-539 and 542-572 (prompt, skill-invocation, and execution-fact writers) to add missing canonical fields such as prompt_hash, parent_prompt_id, source_message_id, skill_version_hash, tool_call_id, bash_commands_redacted, end_reason so canonical parity is preserved.
♻️ Duplicate comments (1)
cli/selftune/activation-rules.ts (1)
128-148:⚠️ Potential issue | 🟡 MinorDon’t assume descending order in the JSONL fallback.
queryEvolutionAudit()is DESC, butreadJsonl()preserves file order. For custom/testevolution_audit_log_pathvalues,auditEntries[0]is usually the oldest record, so this rule can mark evolution as stale even when a newer JSONL entry exists. Compute the newest timestamp explicitly instead of indexing the first element.Suggested fix
- const lastEntry = auditEntries[0]; // queryEvolutionAudit returns DESC order - const lastTimestamp = new Date(lastEntry.timestamp).getTime(); + const lastTimestamp = Math.max( + ...auditEntries.map((entry) => Date.parse(entry.timestamp)).filter((ts) => Number.isFinite(ts)), + ); + if (!Number.isFinite(lastTimestamp)) return null;🤖 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 128 - 148, When falling back to readJsonl for ctx.evolution_audit_log_path, do not assume auditEntries[0] is the newest; instead compute the newest timestamp across auditEntries (e.g., map each entry.timestamp to new Date(...).getTime() and take the max) and use that as lastTimestamp; update the logic around lastEntry/lastTimestamp (which currently assumes queryEvolutionAudit returns DESC) so both branches (queryEvolutionAudit and readJsonl) produce a correct newest timestamp before comparing staleness, and keep existing empty-array handling and calls to checkFalseNegatives unchanged.
🤖 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/queries.ts`:
- Around line 660-692: The rebuilt skill_invocation records omit the skill_path
persisted by insertSkillInvocation; modify the SQL query in the invocations
retrieval (the const invocations = db.query(...).all()) to select skill_path
from the skill_invocations table and include skill_path in the emitted record
object (records.push) so the reconstructed record sets skill_path (falling back
to sessionEnvelope if appropriate) — update the record key alongside other
fields like skill_invocation_id, session_id, raw_source_ref, etc., so staged
skill_invocation entries retain the canonical skill_path.
In `@packages/telemetry-contract/src/schemas.ts`:
- Around line 168-170: The schema change relaxes push_id to z.string().min(1)
but the test staging.test.ts still asserts a UUID format; pick one resolution:
either revert the schema by changing push_id back to z.string().uuid() to
satisfy the existing UUID regex in staging.test.ts, or update the test to
validate a non-empty string instead of a UUID (remove the UUID regex/assertion
and assert presence/non-empty for push_id); locate the push_id declaration in
the telemetry schema and the UUID assertion in staging.test.ts (the test that
checks push_id at the alpha-upload path) and apply the corresponding change so
schema and test agree.
---
Outside diff comments:
In `@cli/selftune/localdb/direct-write.ts`:
- Around line 388-405: The UPDATE currently marks all rows matching (session_id,
query, signal_type) because timestamp isn't part of the WHERE; change the API
and callers to consume a single signal row by adding a timestamp (or stable row
id) parameter to updateSignalConsumed and using it in the WHERE clause (e.g.
WHERE session_id = ? AND query = ? AND signal_type = ? AND timestamp = ? AND
consumed = 0) so only that exact row is updated; also thread the new timestamp
argument through markSignalsConsumed (orchestrate.ts caller) when calling
updateSignalConsumed and pass it into the prepared statement so duplicates or
later-arriving same-query signals are not accidentally consumed.
- Around line 410-456: The insertSession writer (function insertSession) is
missing many canonical session fields (e.g. external_session_id, repo_root,
commit_sha, permission_mode, approval_policy, sandbox_policy, provider,
end_reason) so those values are never persisted to SQLite; update the INSERT
column list and VALUES binding in insertSession to include these missing fields
(and add them to the ON CONFLICT DO UPDATE clause with appropriate
COALESCE/excluded logic), ensuring any complex objects are JSON.stringified like
raw_source_ref; apply the same change pattern to the other writers referenced
around lines 459-483, 486-539 and 542-572 (prompt, skill-invocation, and
execution-fact writers) to add missing canonical fields such as prompt_hash,
parent_prompt_id, source_message_id, skill_version_hash, tool_call_id,
bash_commands_redacted, end_reason so canonical parity is preserved.
---
Duplicate comments:
In `@cli/selftune/activation-rules.ts`:
- Around line 128-148: When falling back to readJsonl for
ctx.evolution_audit_log_path, do not assume auditEntries[0] is the newest;
instead compute the newest timestamp across auditEntries (e.g., map each
entry.timestamp to new Date(...).getTime() and take the max) and use that as
lastTimestamp; update the logic around lastEntry/lastTimestamp (which currently
assumes queryEvolutionAudit returns DESC) so both branches (queryEvolutionAudit
and readJsonl) produce a correct newest timestamp before comparing staleness,
and keep existing empty-array handling and calls to checkFalseNegatives
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a537b75a-b533-4b74-90f1-f2a11dd3c147
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
apps/local-dashboard/package.jsoncli/selftune/activation-rules.tscli/selftune/localdb/direct-write.tscli/selftune/localdb/queries.tscli/selftune/normalization.tscli/selftune/observability.tscli/selftune/orchestrate.tscli/selftune/types.tspackages/telemetry-contract/package.jsonpackages/telemetry-contract/src/schemas.tstests/autonomy-proof.test.tstests/hooks/auto-activate.test.tstests/localdb/read-queries.test.tstests/localdb/write.test.tstests/orchestrate.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/init.ts`:
- Around line 461-465: The function validateAlphaMetadataFlags has a single-line
parameter list causing Biome formatting failures; change its signature to a
multiline parameter list (each parameter on its own line) while preserving types
and the existing body so the formatter accepts it, i.e., update the
validateAlphaMetadataFlags(alpha: boolean | undefined, email?: string, name?:
string): void { ... } declaration to a multiline form.
In `@scripts/link-claude-workspace.sh`:
- Around line 27-33: The current block that checks current_target, skill_src,
and claude_skill_path uses rm -rf "$claude_skill_path" when backup_path already
exists, which can silently delete data; change the logic in that conditional
(referencing current_target, skill_src, claude_skill_path, backup_path) to never
rm -rf the existing claude_skill_path but instead create a new unique backup
(e.g., append a timestamp or incrementing suffix to backup_path) and move
claude_skill_path into that new backup location, or skip replacing if a safe
backup cannot be made, ensuring the original backup is preserved.
In `@tests/localdb/read-queries.test.ts`:
- Around line 863-867: The predicate passed into
queryCanonicalRecordsForStaging(db).find(...) is manually wrapped in a way that
conflicts with Biome's formatter; reformat the arrow function predicate for the
variable invocation so it adheres to Biome styling (let the formatter determine
line breaks) — locate the .find(...) call in tests/localdb/read-queries.test.ts
(variable invocation and the arrow predicate) and put the predicate expression
into the canonical Biome-friendly shape (either a single-line boolean expression
or each && clause on its own aligned line) so CI formatting no longer fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5096b898-28b4-4cc5-9dad-b3d896b9202b
📒 Files selected for processing (9)
cli/selftune/agent-guidance.tscli/selftune/init.tscli/selftune/localdb/queries.tspackage.jsonscripts/link-claude-workspace.shtests/agent-guidance.test.tstests/alpha-upload/staging.test.tstests/init/alpha-consent.test.tstests/localdb/read-queries.test.ts
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
normalizer_version,capture_mode,raw_source_ref) to all 4 SQLite tables so upload staging reads from SQLite instead of JSONLfs.watchFileon the WAL)orchestrate.tssignal consumption to use SQLite directly (no JSONL rewrite)Test plan
bun test tests/alpha-upload/staging.test.ts— 25 pass, 0 fail (validates SQLite staging path)bun test tests/observability.test.ts— validates doctor reports "pass" for dashboard freshnessbun testfull suite — 1661 pass, 12 pre-existing fail, 0 new failuresselftune dashboardand verify WAL-based live updates workselftune doctorand confirmdashboard_freshness_modeshows "pass"🤖 Generated with Claude Code