chore(omniroute): migration version collision fix + governance hardening (17 commits)

[KooshaPari]

User description

Substantial branch with 17 commits ahead of main:

Key changes:

• refactor(db): migrate console.* to pino in src/lib/db/
• fix(security): 4 audit findings + migrate encryption.ts to pino
• fix(governance): log empty catches, replace silent try/catch with explicit log.error
• fix(security): log crypto-relevant silent catches
• ci: use cache-pinned Trunk action
• ci: pin cross-platform Rust toolchain
• fix(desktop): target fork-owned Electron releases
• fix: use Node 24-compatible Keyv SQLite adapter
• chore(governance): rebase mergify config request-review fixes onto main

Net: 69 files changed, +376/-1953 (refactoring cleanup).

15 commits behind main (diverged). Needs rebase before merge.

---

CodeAnt-AI Description

Harden error handling, cloud-sync verification, and quota-store compatibility

What Changed

• Signed cloud-sync responses are rejected when no verification secret is configured instead of being accepted without verification; unsigned responses retain legacy compatibility.
• Keyv quota storage now matches the shared quota-store contract, returns structured pool usage with per-key consumption and burn-rate data, and preserves legacy plan and pool helpers through a separate extension.
• Previously silent failures across database operations, credential loading, encryption, background refresh, version management, OAuth, and WebSocket startup now produce structured operator-visible errors while preserving intentional fallbacks.
• Database migration, backup, cleanup, checkpoint, and schema activity now uses structured logs with relevant context.
• Keyv SQLite support is updated to the newer adapter release, and desktop updates now target the fork-owned release repository.
• CI and container publishing workflows use pinned action and toolchain revisions.

Impact

✅ Rejects unverifiable signed cloud-sync payloads
✅ Detectable database, credential, and background-task failures
✅ Structured quota usage for Keyv-backed pools

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

• New Features

  • Improved Keyv quota pool usage reporting with consumption, fair-share, deficit, borrowing, and burn-rate details.
  • Added support for plan usage, provider plans, and pool management through an extended quota interface.
  • Added stronger cloud-sync signature validation when a signing secret is configured.

• Bug Fixes

  • Corrected quota-store behavior and improved database error handling and recovery reporting.
  • Process failures now correctly report unavailable processes.

• Chores

  • Upgraded the SQLite Keyv adapter.
  • Hardened build and security workflows with immutable action versions.
  • Added local database files to ignore rules.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

[codeant-ai]

🤖 CodeAnt AI — Review Status

Status	Commit	Started (UTC)	Finished (UTC)
✅ Reviewed your PR	f6284ab	Aug 08, 2026 · 02:33	02:38

[codeant-ai]

---

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key: "review"

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

📝 Walkthrough

Walkthrough

The change aligns KeyvQuotaStore with QuotaStore, adds Keyv extras and contract tests, expands structured logging across runtime and database modules, pins GitHub Actions, replaces merge automation, upgrades @keyv/sqlite, and adds design specifications.

Changes

Quota store alignment

Layer / File(s)	Summary
Quota store contract and implementation src/lib/quota/*	KeyvQuotaStore now returns structured pool snapshots and exposes the canonical quota-store methods. Legacy methods move to KeyvQuotaStoreExtras.
Quota validation and fixtures tests/unit/quota/*, tests/e2e/quota-store.e2e.ts	Tests verify interface conformance, canonical methods, extras behavior, and isolated database-backed quota operations.

Structured diagnostics

Layer / File(s)	Summary
Runtime and database logging src/lib/**, open-sse/**, src/server*	Direct console output and silent error paths now use scoped structured loggers. Existing fallback and non-throwing behavior is preserved unless otherwise noted.

Repository controls

Layer / File(s)	Summary
Automation and packaging .github/workflows/*, .mergify.yml, .gitignore, package.json, electron/package.json	Workflow actions are pinned to commits, merge automation uses a release queue, local AgilePlus artifacts are ignored, and package configuration is updated.

Design specifications

Layer / File(s)	Summary
Encryption and Keyv design documents plans/*, .agileplus/*	Draft specifications define encryption hardening, Keyv default-driver behavior, and the Keyv quota-store contract rewrite.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• KooshaPari/OmniRoute#502: Both changes update pinned Trunk Action revisions.
• KooshaPari/OmniRoute#513: This PR contains the same Keyv, workflow, logging, and merge-queue changes.

Suggested labels: typescript

Suggested reviewers: diegosouzapw

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description summarizes the changes but omits the required Related Issues, Validation, Tests Added Or Updated, Coverage Notes, Reviewer Notes, and 71-pillar self-check sections.	Add the missing template sections and document validation results, changed test files, coverage impact, reviewer risks, related issues, and 71-pillar scope.
Docstring Coverage	⚠️ Warning	Docstring coverage is 34.04% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title identifies governance hardening, which is a real part of the changes, but it does not clearly represent the broader logging, security, quota, and CI work.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch agent/migration-version-collision-fix

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/migration-version-collision-fix

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codeant-ai]

Suggestion: The test invokes recordPlanUsage, upsertProviderPlan, listProviderPlans, setPools, and getPool on KeyvQuotaStore, but those methods were removed from that class and now exist only on KeyvQuotaStoreExtras. This test therefore fails its TypeScript checks (and cannot run as written); instantiate or obtain KeyvQuotaStoreExtras and call the methods on that object. [api mismatch]

Severity Level: Major ⚠️

- ❌ Quota unit tests fail TypeScript compilation.
- ⚠️ CI validation cannot run this new test suite successfully.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** tests/unit/quota/keyvQuotaStoreExtras.test.ts
**Line:** 32:38
**Comment:**
	*Api Mismatch: The test invokes `recordPlanUsage`, `upsertProviderPlan`, `listProviderPlans`, `setPools`, and `getPool` on `KeyvQuotaStore`, but those methods were removed from that class and now exist only on `KeyvQuotaStoreExtras`. This test therefore fails its TypeScript checks (and cannot run as written); instantiate or obtain `KeyvQuotaStoreExtras` and call the methods on that object.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[codeant-ai]

Suggestion: When WAL checkpointing fails, backup() logs the error but still copies the main database file and resolves successfully. Any pages remaining only in the WAL may be absent from the copied backup, yet callers will treat the operation as a completed backup. Propagate the checkpoint failure or use a backup procedure that includes the WAL state before reporting success. [api mismatch]

Severity Level: Critical 🚨

- ❌ Backups can omit committed WAL-resident database pages.
- ❌ Restoring an incomplete backup can lose database state.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/lib/db/adapters/nodeSqliteShared.ts
**Line:** 151:156
**Comment:**
	*Api Mismatch: When WAL checkpointing fails, `backup()` logs the error but still copies the main database file and resolves successfully. Any pages remaining only in the WAL may be absent from the copied backup, yet callers will treat the operation as a completed backup. Propagate the checkpoint failure or use a backup procedure that includes the WAL state before reporting success.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[codeant-ai]

Suggestion: Swallowing a persistence error during gracefulClose allows the adapter to close successfully even though the latest in-memory changes were never written to disk. Callers receive no failure signal and the process can lose data; propagate the error or make close report failure after a failed persist. [incomplete implementation]

Severity Level: Major ⚠️

- ❌ SQLite fallback writes can be lost during shutdown.
- ⚠️ Graceful shutdown reports success after persistence failure.
- ⚠️ Database recovery requires reconstructing unsaved in-memory changes.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/lib/db/adapters/sqljsAdapter.ts
**Line:** 165:166
**Comment:**
	*Incomplete Implementation: Swallowing a persistence error during `gracefulClose` allows the adapter to close successfully even though the latest in-memory changes were never written to disk. Callers receive no failure signal and the process can lose data; propagate the error or make close report failure after a failed persist.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[codeant-ai]

Suggestion: Static imports are evaluated before the test module body runs, so core.ts resolves and caches DATA_DIR before this assignment executes. Consequently getDbInstance() still uses the process's original database directory rather than TEST_DATA_DIR, making the test non-isolated and potentially reading or modifying a developer's real database. Set the environment before importing the database modules, or use dynamic imports/setup configuration. [logic error]

Severity Level: Major ⚠️

- ❌ Quota tests can write to the shared local database.
- ⚠️ Parallel Vitest workers can contaminate database state.
- ⚠️ Cleanup removes an unused temporary directory.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** tests/e2e/quota-store.e2e.ts
**Line:** 22:22
**Comment:**
	*Logic Error: Static imports are evaluated before the test module body runs, so `core.ts` resolves and caches `DATA_DIR` before this assignment executes. Consequently `getDbInstance()` still uses the process's original database directory rather than `TEST_DATA_DIR`, making the test non-isolated and potentially reading or modifying a developer's real database. Set the environment before importing the database modules, or use dynamic imports/setup configuration.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[coderabbitai]

Actionable comments posted: 1

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/lib/cloudSync.ts (1)

46-63: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not trust unsigned cloud responses by default.

When OMNIROUTE_CLOUD_SYNC_SECRET is unset, syncToCloud still accepts unsigned responses, parses provider data, and calls updateLocalTokens. That path can update local provider metadata. Require the secret, or put legacy unverified mode behind an explicit opt-in flag. Add tests for signed, unsigned, and missing-secret responses under tests/.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/cloudSync.ts` around lines 46 - 63, Update verifyCloudSignature and
the syncToCloud flow so unsigned responses are rejected when
OMNIROUTE_CLOUD_SYNC_SECRET is unset, unless an explicit legacy-unverified
opt-in flag is enabled. Preserve fail-closed rejection for signed responses
without a secret, and add tests under tests/ covering signed, unsigned, and
missing-secret responses.

Source: Coding guidelines

src/lib/db/stateReset.ts (1)

24-30: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Abort restore when module-state reset fails.

restoreDbBackup calls resetAllDbModuleState() before unlinking and replacing the DB. If a resetter throws, the current implementation logs the failure and continues, so stale module state can remain tied to the closed connection. Return an aggregate reset failure from resetAllDbModuleState() and abort restoreDbBackup before file replacement; add a regression test with a throwing resetter.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/db/stateReset.ts` around lines 24 - 30, Update resetAllDbModuleState
to collect resetter failures and return an aggregate failure instead of silently
continuing after logging. In restoreDbBackup, check that result immediately
after resetting module state and abort before unlinking or replacing the
database when reset fails. Add a regression test registering a throwing resetter
and verifying restore stops before file replacement.

🟡 Minor comments (15)

plans/quota-keystore-type-drift-spec.md-60-65 (1)

60-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep dispose() assigned to KeyvQuotaStore in both specifications.

The layout text says that KeyvQuotaStoreExtras contains dispose, but the design and implementation keep cleanup on KeyvQuotaStore.

• plans/quota-keystore-type-drift-spec.md#L60-L65: Change “5 dead methods + dispose” to “5 dead methods”.
• .agileplus/quota-keystore-type-drift/spec.md#L60-L65: Change “5 dead methods + dispose” to “5 dead methods”.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/quota-keystore-type-drift-spec.md` around lines 60 - 65, Update the
KeyvQuotaStoreExtras descriptions in plans/quota-keystore-type-drift-spec.md
lines 60-65 and .agileplus/quota-keystore-type-drift/spec.md lines 60-65 to say
“5 dead methods” instead of “5 dead methods + dispose”; keep dispose assigned to
KeyvQuotaStore in both specifications.

src/lib/quota/keyvQuotaStore.ts-145-165 (1)

145-165: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Calculate fair share with the persisted allocation total.

poolUsageWithDimensions() stores totalWeight, but fairShare divides by 100. Create, update, and upsertAllocations persist allocations without enforcing a 100 total; the zero-weight normalization still persists 100 / allocations.length per key. Use alloc.weight / totalWeight for fairShare, or normalize weights to 100 on every persistence path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/quota/keyvQuotaStore.ts` around lines 145 - 165, Update fair-share
calculation in poolUsageWithDimensions to divide alloc.weight by the persisted
totalWeight instead of the hardcoded 100. Preserve the zero-total behavior that
produces a zero fair share, and ensure the calculation remains consistent with
allocations persisted by create, update, and upsertAllocations.

plans/encryption-failclosed-spec.md-417-441 (1)

417-441: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Avoid partial in-place mutation on failure.

If apiKey encrypts successfully and accessToken throws, the function returns null but leaves the input object partially encrypted. Encrypt a copy and publish it only after all fields succeed, or restore every original value in the catch block.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` around lines 417 - 441, Update
encryptConnectionFields so encryption occurs on a copy of conn rather than
mutating the input object; apply all encrypted values to the copy, return it
only after every field succeeds, and return null on EncryptionRuntimeError while
leaving the original connection unchanged.

plans/encryption-failclosed-spec.md-314-318 (1)

314-318: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use one canonical AC-2 test location.

AC-2 names tests/unit/encryption.spec.ts, while Step 9 names tests/unit/db/encryption-failclosed.test.ts. The checklist also requires tests/unit/encryption.spec.ts to remain unmodified.

Choose one path and update the acceptance criteria, test plan, and checklist.

Also applies to: 851-851

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` around lines 314 - 318, The encryption
fail-closed specification lists conflicting test locations for AC-2 and
incorrectly requires encryption.spec.ts to remain unchanged. Choose one
canonical test file, then update AC-2, Step 9, and the checklist references
consistently while preserving the requirement that the selected test covers the
crypto failure, cause chain, remediation hint, and audit logging.

plans/encryption-failclosed-spec.md-388-405 (1)

388-405: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Make the log fields match AC-5.

AC-5 requires a masked keyId and a timestamp. Step 2 emits keyBytes and does not emit keyId. Option A also references an undefined maskKeyId() helper.

Define one stable, non-secret key identifier. Verify that the logger supplies timestamps, or include the timestamp explicitly.

Also applies to: 318-318

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` around lines 388 - 405, Update the
encryption failure logging around the catch block to emit the AC-5 fields: a
stable masked keyId derived from the configured key without exposing key
material, plus a timestamp; remove keyBytes. Define or reuse one valid keyId
helper consistently, including the related logging at the other affected
location, and confirm timestamps are supplied by the logger before omitting an
explicit timestamp.

plans/encryption-failclosed-spec.md-330-331 (1)

330-331: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Run every test named by AC-18.

AC-18 includes tests/integration/db/providers.test.ts, but the verification command omits it. Section 8.2 also says “if present,” while AC-18 treats the test as required.

Add the omitted suite or remove it from the acceptance criterion.

Also applies to: 676-677

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` around lines 330 - 331, Update the AC-18
verification command to run both required integration suites,
tests/integration/db/encrypt.test.mjs and
tests/integration/db/providers.test.ts. Keep AC-18’s requirement that both tests
pass, and remove or revise any conflicting “if present” wording in Section 8.2.

plans/encryption-failclosed-spec.md-612-612 (1)

612-612: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Escape the union type in the Markdown table.

The | in T | null creates an extra table cell. Escape the separator or write the type without a table delimiter.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` at line 612, Update the R-4 Markdown
table entry describing encryptConnectionFields so the T | null union does not
create an extra table column; escape the pipe character or express the type
without a table delimiter, while preserving the existing meaning.

Source: Linters/SAST tools

plans/encryption-failclosed-spec.md-63-65 (1)

63-65: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Rewrite the key-length failure scenarios.

scryptSync(secret, STATIC_SALT, KEY_LENGTH) always produces a 32-byte key buffer. A short or non-base64 environment key does not make createCipheriv reject the derived key, and cached key rotation does not change the derived buffer length. If malformed or weak keys must fail at startup, add explicit validation and test that policy separately; do not use these cases as evidence for State B. Also applies to R-1 at lines 609-610.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` around lines 63 - 65, Rewrite the
key-length failure scenarios to remove claims that short, non-base64, or rotated
environment keys cause derived-key length failures, since scryptSync always
produces a 32-byte buffer. If startup rejection of malformed or weak keys is
required, describe it as explicit validation and separate policy testing, and
update the corresponding R-1 discussion as well.

plans/keyv-as-embedded-default-spec.md-240-245 (1)

240-245: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Do not claim timestamp-based idempotency without implementing it.

The script documentation says it uses lastUpdatedAt to skip rows, but no timestamp is read, stored, or compared. Define the actual idempotency mechanism and test repeated and interrupted migrations.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/keyv-as-embedded-default-spec.md` around lines 240 - 245, Update the
one-shot migration specification to define and implement a concrete idempotency
mechanism instead of claiming lastUpdatedAt-based skipping without support. Add
the necessary timestamp read, persistence, and comparison across
SqliteQuotaStore and KeyvQuotaStore, and cover both repeated and interrupted
migrations with tests.

plans/keyv-as-embedded-default-spec.md-370-373 (1)

370-373: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fix the AC-15 verification command.

The proposed .env.example change adds one QUOTA_KEYV_BACKEND line, but the command requires grep to return more than one match. Assert at least one match or check the exact expected lines.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/keyv-as-embedded-default-spec.md` around lines 370 - 373, Update
AC-15’s verification command to accept the single documented QUOTA_KEYV_BACKEND
entry, using an at-least-one-match check or assertions for the exact expected
environment lines instead of requiring more than one grep match.

plans/keyv-as-embedded-default-spec.md-491-492 (1)

491-492: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the typecheck command fail on TypeScript errors.

The pipeline ends with wc -l, so it exits successfully even when tsc fails. Use set -o pipefail and preserve the direct tsc exit status, or compare the error count and return nonzero.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/keyv-as-embedded-default-spec.md` around lines 491 - 492, Update the
AC-12 typecheck command so TypeScript errors produce a nonzero exit status
instead of being masked by wc -l; enable pipefail while preserving the existing
filtering, or explicitly compare the resulting error count and fail when it is
nonzero.

plans/keyv-as-embedded-default-spec.md-522-529 (1)

522-529: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Run the smoke test with tsx.

src/lib/quota/storeFactory.ts imports @/... aliases, and plain node -e .ts does not load the alias/ts support used by the earlier --import tsx commands. Use ts/tsx instead so the smoke test is not a setup/load failure.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/keyv-as-embedded-default-spec.md` around lines 522 - 529, Update the
smoke-test command invoking getQuotaStore and resetQuotaStoreSingleton to run
through tsx instead of plain node, preserving the existing
DISABLE_SQLITE_AUTO_BACKUP environment setting and validation output so
TypeScript and `@/`... aliases load correctly.

src/lib/machineToken.ts-12-19 (1)

12-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the machine-token fallback description.

machineIdSync = () => "" supplies an empty machine ID. activeSalt still comes from OMNIROUTE_CLI_SALT or BUILTIN_DEFAULT_SALT. The log incorrectly says that the HMAC salt becomes empty.

Describe this as an empty machine ID or HMAC key fallback.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/machineToken.ts` around lines 12 - 19, Update the fallback message in
the machineIdSync error handler to state that the machine ID or HMAC key falls
back to an empty value, without claiming that activeSalt becomes empty. Preserve
the existing fallbackLogged guard and structured error logging.

src/lib/versionManager/binaryManager.ts-203-207 (1)

203-207: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Downgrade expected missing-path errors.

getCurrentBinaryPath returns null when the symlink is absent. getInstalledVersions returns an empty list when the bin directory is absent. fs.realpath and fs.readdir normally throw ENOENT in these states, so the new error logs will report normal first-run checks as failures.

Suppress or downgrade ENOENT; keep error-level logging for unexpected failures.

Also applies to: 225-229

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/versionManager/binaryManager.ts` around lines 203 - 207, Update the
error handling in getCurrentBinaryPath and getInstalledVersions so filesystem
ENOENT errors from fs.realpath and fs.readdir are suppressed or logged below
error level, while unexpected failures continue using error-level logging;
preserve the existing null and empty-list return behavior for missing paths.

src/lib/db/cleanup.ts-188-193 (1)

188-193: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the actual SQLite table names in cleanup logs.

The SQL targets mcp_tool_audit, a2a_task_events, and memories at Lines 184, 215, and 246. The changed logs report mcp_audit_log, a2a_events, and memory_entries.

Update the table fields and messages to match the SQL names. Otherwise, operational searches will report the wrong table.

Corrected table labels

-      { deleted: result.deleted, retentionDays, table: "mcp_audit_log" },
+      { deleted: result.deleted, retentionDays, table: "mcp_tool_audit" },

-      { deleted: result.deleted, retentionDays, table: "a2a_events" },
+      { deleted: result.deleted, retentionDays, table: "a2a_task_events" },

-      { deleted: result.deleted, retentionDays, table: "memory_entries" },
+      { deleted: result.deleted, retentionDays, table: "memories" },

Also applies to: 219-224, 250-255

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/db/cleanup.ts` around lines 188 - 193, Update the cleanup log entries
in the relevant cleanup functions to use the actual SQL table names:
mcp_tool_audit, a2a_task_events, and memories. Replace both the table fields and
success/error message labels, preserving the existing deletion details and error
handling.

🧹 Nitpick comments (3)

open-sse/services/tierResolver.ts (1)

117-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid reusing the logger's module field.

createLogger() attaches module to the child logger. This log call also writes module with a configuration path. The event can lose the open-sse:tier-resolver identity or emit duplicate fields.

Rename the field to configModulePath.

Proposed fix

-        { err, module: "../../src/lib/db/tierConfig" },
+        { err, configModulePath: "../../src/lib/db/tierConfig" },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@open-sse/services/tierResolver.ts` around lines 117 - 121, Rename the
structured log field in the catch block of the tier resolver’s tierConfig
loading flow from module to configModulePath, while preserving its existing
configuration path value and error logging behavior.

src/lib/db/settings.ts (1)

201-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Preserve the original Error object in the structured log.

The err field currently contains only error.message or String(error). Pino cannot record the original error type or stack. Pass the caught value as err and keep the normalized text in a separate errorMessage field if required.

As per coding guidelines, caught errors must be logged with pino context.

Proposed fix

-      { err: error instanceof Error ? error.message : String(error) },
+      {
+        err: error,
+        errorMessage: error instanceof Error ? error.message : String(error),
+      },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/db/settings.ts` around lines 201 - 205, Update the catch block in the
runtime settings update flow to pass the caught error object directly as the
structured log’s err field, preserving its type and stack. If normalized text is
still needed, add it separately as errorMessage rather than replacing err; keep
the existing warning message and pino context.

Source: Coding guidelines

src/lib/cloudSync.ts (1)

4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add explicit types to the newly added module loggers.

Each listed declaration relies on inference. Use ReturnType<typeof createLogger> or the project's explicit logger type.

As per coding guidelines, TypeScript code must use explicit types rather than relying on inference.

• src/lib/cloudSync.ts#L4-L6: annotate log.
• open-sse/config/credentialLoader.ts#L19-L21: annotate log.
• open-sse/services/autoRefreshDaemon.ts#L14-L16: annotate log.
• open-sse/services/tierResolver.ts#L6-L8: annotate log.
• src/lib/machineToken.ts#L2-L4: annotate log.
• src/lib/oauth/providers/antigravity.ts#L8-L10: annotate log.
• src/lib/resilience/anomalyHook.ts#L16-L20: annotate log.
• src/lib/versionManager/binaryManager.ts#L10-L14: annotate log.
• src/lib/db/settings.ts#L14-L18: annotate log.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/cloudSync.ts` around lines 4 - 6, Annotate each newly declared log
variable with the explicit logger type, preferably ReturnType<typeof
createLogger>, while preserving the existing createLogger calls:
src/lib/cloudSync.ts lines 4-6, open-sse/config/credentialLoader.ts lines 19-21,
open-sse/services/autoRefreshDaemon.ts lines 14-16,
open-sse/services/tierResolver.ts lines 6-8, src/lib/machineToken.ts lines 2-4,
src/lib/oauth/providers/antigravity.ts lines 8-10,
src/lib/resilience/anomalyHook.ts lines 16-20,
src/lib/versionManager/binaryManager.ts lines 10-14, and src/lib/db/settings.ts
lines 14-18. No other logger behavior needs to change.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plans/encryption-failclosed-spec.md`:
- Around line 421-429: Update encryptConnectionFields so every populated
credential field—apiKey, accessToken, refreshToken, and idToken—must receive a
non-null encrypted value from encrypt(); remove the plaintext fallbacks. If any
encryption result is null, reject the connection and do not return or persist
the partially encrypted object, including when getStaticKey() returns null while
encryption is enabled.

---

Outside diff comments:
In `@src/lib/cloudSync.ts`:
- Around line 46-63: Update verifyCloudSignature and the syncToCloud flow so
unsigned responses are rejected when OMNIROUTE_CLOUD_SYNC_SECRET is unset,
unless an explicit legacy-unverified opt-in flag is enabled. Preserve
fail-closed rejection for signed responses without a secret, and add tests under
tests/ covering signed, unsigned, and missing-secret responses.

In `@src/lib/db/stateReset.ts`:
- Around line 24-30: Update resetAllDbModuleState to collect resetter failures
and return an aggregate failure instead of silently continuing after logging. In
restoreDbBackup, check that result immediately after resetting module state and
abort before unlinking or replacing the database when reset fails. Add a
regression test registering a throwing resetter and verifying restore stops
before file replacement.

---

Major comments:
In @.github/workflows/cross-platform.yml:
- Line 104: Update the dtolnay/rust-toolchain step in the cross-platform
workflow to add a with.toolchain value specifying the required exact Rust
release, replacing the action’s floating stable default while preserving the
existing pinned action revision.

In @.gitignore:
- Around line 31-36: Update the .gitignore AgilePlus entries by removing the
blanket .agileplus/ rule, while retaining the agileplus-*.db,
agileplus-*.db-shm, and agileplus-*.db-wal patterns so specification Markdown
files under .agileplus/ remain trackable.

In @.mergify.yml:
- Around line 20-46: Add a pull_request_rule that automatically invokes the
Mergify queue action when a pull request targets a release branch and has the
queue label, using conditions aligned with the release queue rule. Keep the
existing queue_rules configuration unchanged and ensure the action enqueues
matching PRs rather than only gating already-queued PRs.

In `@plans/encryption-failclosed-spec.md`:
- Around line 648-650: Update the AC-16 typecheck command so TypeScript failures
propagate as a nonzero status instead of being masked by the grep/wc pipeline.
Run tsc directly with its output handling, or enable pipefail and explicitly
assert the compiler status while preserving the zero-error count check.
- Around line 296-299: Standardize encryptConnectionFields() to return null on
encryption failure, matching AC-11 and the provider logic. Update the
implementation steps, acceptance criteria, and encryption fault-injection tests
to use this contract, and ensure providers refuse insert/update when the result
is null.
- Line 557: Update the verification steps around the encryption startup checks
so an empty or unset STORAGE_ENCRYPTION_KEY only emits a warning and does not
cause startup failure. Replace the empty-key fail-fast case with a non-empty
invalid key or other fault-injection value, and verify that case exits non-zero
with the expected log line while preserving the known-good key check.
- Around line 214-215: The encryption failure contract should consistently throw
`EncryptionRuntimeError` from runtime `encrypt()` failures instead of calling
`process.exit(1)`, allowing request handlers to perform cleanup and return HTTP
500 responses. Reserve process termination for `validateEncryptionAtStartup()`
when startup validation fails, while preserving structured failure logging and
optional Sentry reporting.
- Around line 504-521: Update the EncryptionRuntimeError branch in the
markCommandCodeAuthSessionReceived flow to invoke the existing
authentication-session failure transition before returning null. Persist status
'failed' and the relevant last_error through that transition, while preserving
the current logging and rethrow behavior for other errors.
- Around line 217-255: The startup module is missing dependencies and has an
ambiguous owner for validateEncryptionAtStartup. Choose a single owner between
encryptionStartup.ts and encryption.ts, then update that module’s imports and
exports so StartupEncryptionError, PREFIX, and log are defined or imported from
their existing sources, while keeping encrypt, decrypt, and isEncryptionEnabled
available; remove or re-export the duplicate implementation so the documented
public API type-checks.
- Around line 820-828: Update the “11.3 Delivery sequence” checklist so user
approval and resolution of Q1–Q7 occur before implementation and before any
merge or delivery steps. Make the sequence explicitly stop until all open
questions are answered, then retain the existing implementation, verification,
commit, PR, merge, registration, and cleanup steps in order.
- Around line 528-532: Make the legacy-row migration in the providers migration
flow atomic by wrapping all row updates, including
migrateLegacyEncryptedString() encryption calls, in a database transaction; roll
back every prior update when encryption fails, and perform the final backup and
cache invalidation only after commit. Add or update tests covering a
mid-migration failure and successful retry behavior.
- Around line 681-713: Replace the Section 8.4 inline Node test command with a
smoke invocation that exercises only the startup canary, reusing the existing
encryptionStartup entry point and removing the ineffective randomBytes
monkey-patch and placeholder encrypt() test. Keep the explicit AC-2 failure-path
assertions in the canonical Vitest test referenced by Section 8.3.
- Around line 543-557: Update src/instrumentation-node.ts to invoke
runEncryptionStartupCheck() before any database or service bootstrap path,
including getDbInstance() and ensureDbInitialized(). Keep the existing
src/instrumentation.ts register hook, but ensure the node instrumentation entry
point performs the check first and awaits it before continuing.

In `@plans/keyv-as-embedded-default-spec.md`:
- Line 261: Update the import of listAllocationsForApiKey to use the specific
src/lib/db domain module that owns the allocation tables, replacing the
`@/lib/localDb` barrel import while leaving the migration behavior unchanged.
- Around line 3-8: Update the plan metadata and delivery steps for the
keyv-as-embedded-default feature to use a dedicated worktree under
.claude/worktrees/ instead of repos/OmniRoute/.worktrees/. Include confirming
the base branch before creating the worktree, while preserving the existing base
branch value.
- Around line 151-156: The backend resolution branch incorrectly accepts “file”
while returning the SQLite URI, so implement a real file backend or remove
“file” consistently. Update the backend schema, documentation, tests, and the
resolution logic around resolvedKvUrl to ensure only supported backends remain
and AC-9 is satisfied.
- Around line 299-310: Update the `.env.example` guidance in section 4.3.4 so it
does not set QUOTA_STORE_KEYV_URL unconditionally: keep a single commented URL
example, remove the preserved stale URL assignment, and ensure
QUOTA_KEYV_BACKEND can select memory without being overridden by an explicit
URL.
- Around line 265-276: Update the migration entry point to declare an explicit
Promise<void> return type on main, and define a named TypeScript type for
allocation rows returned by listAllocationsForApiKey. Apply that named row type
to the keys collection or function return so the loop variable k is explicitly
typed while preserving the existing migration behavior.
- Around line 143-147: Update the keyv factory branch around driver selection to
use readKeyvDefaultConfigFromEnv() and the canonical Zod schemas from
src/shared/validation/schemas.ts instead of directly reading
dbSettings.keyvBackend or QUOTA_KEYV_BACKEND. Validate both database and
environment backend settings before selecting the backend, ensuring invalid
values cannot reach the durable else branch while preserving explicit URL
handling.
- Around line 326-334: Update seed to avoid overwriting the shared pool counter
when called for multiple API keys: aggregate values by (poolId, dim) and seed
poolDimKey once with the sum, while retaining each API key’s individual seed
behavior. Ensure pool totals are independent of API-key iteration order.
- Around line 650-655: Update the delivery sequence around the merge step to
remove `gh pr merge --admin` as the default procedure. Require an explicitly
authorized exception and operator approval before allowing any protected-check
bypass, while making the normal merge path comply with branch protections and
successful required checks.
- Around line 257-269: Update main to call readKeyvDefaultConfigFromEnv(), then
pass the resolved target URI explicitly to getKeyvQuotaStore() so migration
always writes to the configured Keyv store instead of an implicit default or
in-memory target.
- Around line 425-427: Update the planned tests in quota-store-migration.test.ts
to call resetDbInstance() and close every opened SQLite database handle in
test.after(...). Keep this cleanup required for both dry-run and apply migration
cases so no state or locked handles leak between tests.
- Around line 185-209: Update readKeyvDefaultConfigFromEnv and the associated
behavior matrix/AC-6 to use one consistent invalid-driver contract: either make
QUOTA_STORE_DRIVER_SCHEMA parsing fall back to SQLite for unknown values, or
revise AC-6 and the matrix to require validation failure. Ensure the factory and
helper follow the same behavior without bypassing schema validation.
- Around line 80-82: Expand the default-switch specification to define
compatibility criteria for the differing SqliteQuotaStore two-bucket
sliding-window and KeyvQuotaStore single-TTL behaviors, especially at window
boundaries. Add regression tests covering quota decisions at those boundaries
before treating the Keyv default promotion as complete; do not rely on the
deferred benchmark as evidence of semantic compatibility.
- Around line 342-349: The documented upgrade behavior contradicts the stated
lack of SQLite-data detection and could cause unpinned existing users to switch
stores. Resolve this consistently by either updating the factory’s
default-selection logic to detect and preserve existing SQLite data, or revise
the table, acceptance criteria, and release instructions to require an explicit
SQLite store pin before upgrading; ensure all documented paths agree.
- Around line 180-186: The KEYV_BACKEND_SCHEMA and QUOTA_STORE_DRIVER_SCHEMA
definitions must come from the shared validation module rather than the config
implementation. Move or re-export both schemas and their inferred types through
src/shared/validation/schemas.ts, then update the keyvDefaultConfig reader to
import and use those canonical symbols instead of importing Zod directly or
defining local schemas.
- Around line 429-431: Add coverage verification to the “Run full
acceptance-criteria sweep” step: invoke the repository’s coverage command from
the verification workflow and verify statements, lines, functions, and branches
each meet the 60% minimum without regressing the established baseline, alongside
the existing functional tests and typecheck.
- Around line 274-281: Update the apply branch in the migration loop to write
each nonzero sqliteCount into Keyv after initializing the bucket with
keyv.consume, using the documented direct kv.set API from §4.4. Remove the
placeholder comment and increment migrated only after the source value has been
successfully persisted.
- Around line 157-169: Update the Keyv initialization path around
getKeyvQuotaStore so a backend failure no longer falls through to
SqliteQuotaStore. After logging the Keyv error, either propagate an actionable
failure or select the platform-safe in-memory fallback, while preserving the
Keyv success path and avoiding any silent driver switch.
- Around line 20-24: Correct the backend comparison in the Keyv rationale:
remove the claim that keyv://sqlite: is JS-only or avoids native bindings, since
`@keyv/sqlite`@4.0.8 depends on better-sqlite3. Reframe the platform
recommendation around a genuinely JS-only backend, or make the default
non-SQLite unless the local-db runtime is addressed.
- Around line 136-142: Update the kvUrl fallback in the documented quota
configuration to use the supported sqlite:// Keyv URI and resolve the database
path from DATA_DIR before quota.db. Replace the relative .agileplus/quota
location while preserving explicit dbSettings.kvUrl and QUOTA_STORE_KEYV_URL
precedence.

In `@src/lib/db/adapters/nodeSqliteShared.ts`:
- Around line 159-167: Update nodeSqliteShared.checkpoint to validate mode
against PASSIVE, FULL, RESTART, and TRUNCATE before interpolating it into the
PRAGMA statement. Apply the same whitelist through the shared checkpointMode
validation path, rejecting invalid values before db.exec while preserving the
existing default and error logging.
- Around line 72-78: Update runSavepoint recovery in nodeSqliteShared.ts (lines
72-78) and sqljsAdapter.ts (lines 99-104) so any ROLLBACK TO or RELEASE failure
marks the adapter unusable or closes it before rethrowing the original error;
ensure subsequent adapter methods cannot execute against the potentially
inconsistent database.
- Around line 151-155: Update db.backup() so a failed PRAGMA
wal_checkpoint(TRUNCATE) in its catch block propagates an error and prevents
fs.copyFileSync(filePath, destination) from running. Ensure failed checkpoints
reject the backup operation rather than reporting success, and add coverage for
a busy checkpoint failure.
- Around line 96-100: Keep adapter state accurate when database closing fails:
in nodeSqliteShared.ts lines 96-100 and sqljsAdapter.ts lines 169-172, update
the close logic so _isOpen is set to false only after db.close() succeeds; when
it throws, retain the open flag and preserve the existing error handling or
propagate the failure.

In `@src/lib/db/adapters/sqljsAdapter.ts`:
- Around line 165-167: The gracefulClose persistence path must not close the
adapter or mark _isOpen false after a failed persist. Update gracefulClose and
the signal flush flow to retry persist with a bounded limit, and only close
after persistence succeeds; if retries are exhausted, reject or return the
failure while preserving the open dirty adapter state. Add tests covering write
failures during close() and signal-triggered flush.

In `@src/lib/db/encryption.ts`:
- Around line 206-213: Remove the ciphertext prefix from the decryption failure
log in the decryption error path, leaving only a static failure message and, if
needed, safe metadata such as ciphertext length or encryption version. Keep the
existing error handling and the separate catch logging in place.
- Around line 122-125: Update encrypt() in the encryption module so both the
missing STORAGE_ENCRYPTION_KEY branch and the encryption-exception branch never
return plaintext; instead throw or return a typed failure that persistence
callers reject before writing. Update affected persistence handling accordingly,
and add regression tests covering both failure paths to verify plaintext is not
persisted.

In `@src/lib/machineToken.ts`:
- Around line 12-20: Update the machine-ID fallback around machineIdSync and the
token authentication functions, including isCliTokenAuthValid, so CLI-token
authentication fails closed when node-machine-id is unavailable. Require a real
machine ID or an explicit per-installation secret before deriving or accepting
tokens; do not treat the empty-string fallback as valid authentication, while
preserving normal behavior when secure key material is available.

In `@src/lib/oauth/providers/antigravity.ts`:
- Around line 104-111: Update the user-info handling in postExchange around the
fetch catch and userInfoRes.json call so JSON parsing and validation occur
inside the same best-effort error path. Reuse the shared Zod schema to validate
the parsed payload, log failures consistently, and return an empty user-info
object for fetch, parse, or validation errors without rejecting the OAuth
exchange.

In `@src/lib/quota/keyvQuotaStore.ts`:
- Line 31: Replace the getPool import from the prohibited "`@/lib/localDb`" barrel
with the domain-specific import from "`@/lib/db/quotaPools`", leaving all getPool
usage unchanged.

In `@src/lib/quota/keyvQuotaStoreExtras.ts`:
- Around line 28-30: The shared planKey causes recordPlanUsage and
upsertProviderPlan to overwrite each other and read incompatible values. Update
planKey and the corresponding key construction in recordPlanUsage and
upsertProviderPlan to use distinct prefixes such as planUsage: and
providerPlan:, then add regression coverage for both upsert-before-usage and
usage-before-upsert orders.

In `@src/lib/quota/storeFactory.ts`:
- Around line 138-146: Update the Redis failure handling around
getRedisQuotaStore(redisUrl) so driver === "redis" does not fall through to
getSqliteQuotaStore(); fail startup or require an explicit SQLite-fallback
opt-in instead. Preserve the existing redacted error logging, and add a
regression test covering the configured Redis driver failure path.
- Around line 138-146: Update the Redis error handling around the QuotaStore
fallback and its visible redisUrl logging to use a complete URL sanitizer: parse
each supported Redis, TLS, and sentinel URL, clear embedded username/password
credentials, and mask or remove sensitive query parameters such as password.
Apply the same sanitization to err before logging when its message may contain
the connection string, while preserving the fail-fast error log and sqlite
fallback.

In `@src/lib/resilience/anomalyHook.ts`:
- Around line 64-75: Update getProviderManagerRegistry so a missing
providerRegistry does not cache the fallback empty Map in
_providerManagerRegistry; retain the empty Map only for the current call while
allowing later invocations to observe the registry once `@/engine/providers`
initializes it. Preserve the existing cached-registry behavior and error
logging.

In `@src/lib/versionManager/processManager.ts`:
- Around line 155-160: The process-info read failure handler in getProcessInfo
currently reports a successfully detected process as dead. Return alive: true
when isProcessRunning(pid) succeeded but /proc or ps usage details cannot be
read, while preserving the PID and existing error logging; add a unit test
covering a live PID with failed process-info reads.

In `@src/server/ws/liveServer.ts`:
- Around line 482-487: Update startLiveServer()’s auth warmup catch around
loadAuthModule() to clear the cached authModulePromise and rethrow the original
error before startup returns, allowing later loadAuthModule() calls to retry.
Add coverage for a failed warmup followed by a successful authorization
resolution.

In `@tests/e2e/quota-store.e2e.ts`:
- Around line 19-23: Update the quota-store test cleanup around its
afterEach/afterAll hooks to close all SQLite database handles and call
resetDbInstance() before removing TEST_DATA_DIR. Prefer deleting the shared
TEST_DATA_DIR once in afterAll, while preserving per-test cleanup for test state
and ensuring every test that opens the database resets the singleton before
cleanup.

In `@tests/unit/quota/keyvQuotaStoreExtras.test.ts`:
- Around line 3-6: Update the tests in KeyvQuotaStoreExtras to import and use
getKeyvQuotaStoreExtras and __resetKeyvQuotaStoreExtrasForTests instead of the
base KeyvQuotaStore helpers. Replace each direct store method call, including
recordPlanUsage and upsertProviderPlan, with calls on the extension instance
returned by getKeyvQuotaStoreExtras, and reset that instance with the matching
extras test helper.

In `@tests/unit/quota/quotaStore.contract.test.ts`:
- Around line 24-40: Add tests/unit/quota/quotaStore.contract.test.ts to the
explicit file set checked by tsconfig.typecheck-core.json, or otherwise include
it in the typecheck:core command used by quality.yml. Ensure the compile-time
assertions around _SqliteConforms, _RedisConforms, and _KeyvConforms are
enforced by the CI typecheck guard.

---

Minor comments:
In `@plans/encryption-failclosed-spec.md`:
- Around line 417-441: Update encryptConnectionFields so encryption occurs on a
copy of conn rather than mutating the input object; apply all encrypted values
to the copy, return it only after every field succeeds, and return null on
EncryptionRuntimeError while leaving the original connection unchanged.
- Around line 314-318: The encryption fail-closed specification lists
conflicting test locations for AC-2 and incorrectly requires encryption.spec.ts
to remain unchanged. Choose one canonical test file, then update AC-2, Step 9,
and the checklist references consistently while preserving the requirement that
the selected test covers the crypto failure, cause chain, remediation hint, and
audit logging.
- Around line 388-405: Update the encryption failure logging around the catch
block to emit the AC-5 fields: a stable masked keyId derived from the configured
key without exposing key material, plus a timestamp; remove keyBytes. Define or
reuse one valid keyId helper consistently, including the related logging at the
other affected location, and confirm timestamps are supplied by the logger
before omitting an explicit timestamp.
- Around line 330-331: Update the AC-18 verification command to run both
required integration suites, tests/integration/db/encrypt.test.mjs and
tests/integration/db/providers.test.ts. Keep AC-18’s requirement that both tests
pass, and remove or revise any conflicting “if present” wording in Section 8.2.
- Line 612: Update the R-4 Markdown table entry describing
encryptConnectionFields so the T | null union does not create an extra table
column; escape the pipe character or express the type without a table delimiter,
while preserving the existing meaning.
- Around line 63-65: Rewrite the key-length failure scenarios to remove claims
that short, non-base64, or rotated environment keys cause derived-key length
failures, since scryptSync always produces a 32-byte buffer. If startup
rejection of malformed or weak keys is required, describe it as explicit
validation and separate policy testing, and update the corresponding R-1
discussion as well.

In `@plans/keyv-as-embedded-default-spec.md`:
- Around line 240-245: Update the one-shot migration specification to define and
implement a concrete idempotency mechanism instead of claiming
lastUpdatedAt-based skipping without support. Add the necessary timestamp read,
persistence, and comparison across SqliteQuotaStore and KeyvQuotaStore, and
cover both repeated and interrupted migrations with tests.
- Around line 370-373: Update AC-15’s verification command to accept the single
documented QUOTA_KEYV_BACKEND entry, using an at-least-one-match check or
assertions for the exact expected environment lines instead of requiring more
than one grep match.
- Around line 491-492: Update the AC-12 typecheck command so TypeScript errors
produce a nonzero exit status instead of being masked by wc -l; enable pipefail
while preserving the existing filtering, or explicitly compare the resulting
error count and fail when it is nonzero.
- Around line 522-529: Update the smoke-test command invoking getQuotaStore and
resetQuotaStoreSingleton to run through tsx instead of plain node, preserving
the existing DISABLE_SQLITE_AUTO_BACKUP environment setting and validation
output so TypeScript and `@/`... aliases load correctly.

In `@plans/quota-keystore-type-drift-spec.md`:
- Around line 60-65: Update the KeyvQuotaStoreExtras descriptions in
plans/quota-keystore-type-drift-spec.md lines 60-65 and
.agileplus/quota-keystore-type-drift/spec.md lines 60-65 to say “5 dead methods”
instead of “5 dead methods + dispose”; keep dispose assigned to KeyvQuotaStore
in both specifications.

In `@src/lib/db/cleanup.ts`:
- Around line 188-193: Update the cleanup log entries in the relevant cleanup
functions to use the actual SQL table names: mcp_tool_audit, a2a_task_events,
and memories. Replace both the table fields and success/error message labels,
preserving the existing deletion details and error handling.

In `@src/lib/machineToken.ts`:
- Around line 12-19: Update the fallback message in the machineIdSync error
handler to state that the machine ID or HMAC key falls back to an empty value,
without claiming that activeSalt becomes empty. Preserve the existing
fallbackLogged guard and structured error logging.

In `@src/lib/quota/keyvQuotaStore.ts`:
- Around line 145-165: Update fair-share calculation in poolUsageWithDimensions
to divide alloc.weight by the persisted totalWeight instead of the hardcoded
100. Preserve the zero-total behavior that produces a zero fair share, and
ensure the calculation remains consistent with allocations persisted by create,
update, and upsertAllocations.

In `@src/lib/versionManager/binaryManager.ts`:
- Around line 203-207: Update the error handling in getCurrentBinaryPath and
getInstalledVersions so filesystem ENOENT errors from fs.realpath and fs.readdir
are suppressed or logged below error level, while unexpected failures continue
using error-level logging; preserve the existing null and empty-list return
behavior for missing paths.

---

Nitpick comments:
In `@open-sse/services/tierResolver.ts`:
- Around line 117-121: Rename the structured log field in the catch block of the
tier resolver’s tierConfig loading flow from module to configModulePath, while
preserving its existing configuration path value and error logging behavior.

In `@src/lib/cloudSync.ts`:
- Around line 4-6: Annotate each newly declared log variable with the explicit
logger type, preferably ReturnType<typeof createLogger>, while preserving the
existing createLogger calls: src/lib/cloudSync.ts lines 4-6,
open-sse/config/credentialLoader.ts lines 19-21,
open-sse/services/autoRefreshDaemon.ts lines 14-16,
open-sse/services/tierResolver.ts lines 6-8, src/lib/machineToken.ts lines 2-4,
src/lib/oauth/providers/antigravity.ts lines 8-10,
src/lib/resilience/anomalyHook.ts lines 16-20,
src/lib/versionManager/binaryManager.ts lines 10-14, and src/lib/db/settings.ts
lines 14-18. No other logger behavior needs to change.

In `@src/lib/db/settings.ts`:
- Around line 201-205: Update the catch block in the runtime settings update
flow to pass the caught error object directly as the structured log’s err field,
preserving its type and stack. If normalized text is still needed, add it
separately as errorMessage rather than replacing err; keep the existing warning
message and pino context.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d9a0bd4-f19a-479b-b469-11cec9c21199

📥 Commits

Reviewing files that changed from the base of the PR and between a7b72af and f6284ab.

⛔ Files ignored due to path filters (2)

• .agileplus/agileplus.db is excluded by !**/*.db
• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (55)

• .agileplus/agileplus.db-shm
• .agileplus/agileplus.db-wal
• .agileplus/quota-keystore-type-drift/spec.md
• .github/workflows/build-fork.yml
• .github/workflows/ci.yml
• .github/workflows/cross-platform.yml
• .github/workflows/docker-publish.yml
• .github/workflows/trunk-check.yml
• .gitignore
• .mergify.yml
• electron/package.json
• open-sse/config/credentialLoader.ts
• open-sse/services/autoRefreshDaemon.ts
• open-sse/services/tierResolver.ts
• package.json
• plans/encryption-failclosed-spec.md
• plans/keyv-as-embedded-default-spec.md
• plans/quota-keystore-type-drift-spec.md
• src/lib/cloudSync.ts
• src/lib/db/adapters/betterSqliteAdapter.ts
• src/lib/db/adapters/driverFactory.ts
• src/lib/db/adapters/nodeSqliteAdapter.ts
• src/lib/db/adapters/nodeSqliteShared.ts
• src/lib/db/adapters/sqljsAdapter.ts
• src/lib/db/apiKeys.ts
• src/lib/db/backup.ts
• src/lib/db/cleanup.ts
• src/lib/db/core.ts
• src/lib/db/encryption.ts
• src/lib/db/migrationRunner.ts
• src/lib/db/models.ts
• src/lib/db/optimizationSettings.ts
• src/lib/db/providers.ts
• src/lib/db/quotaPools.ts
• src/lib/db/quotaSnapshots.ts
• src/lib/db/schemaColumns.ts
• src/lib/db/sessionAccountAffinity.ts
• src/lib/db/settings.ts
• src/lib/db/settings/cacheMetrics.ts
• src/lib/db/stateReset.ts
• src/lib/machineToken.ts
• src/lib/oauth/providers/antigravity.ts
• src/lib/quota/keyvQuotaStore.ts
• src/lib/quota/keyvQuotaStoreExtras.ts
• src/lib/quota/storeFactory.ts
• src/lib/quota/types.ts
• src/lib/resilience/anomalyHook.ts
• src/lib/versionManager/binaryManager.ts
• src/lib/versionManager/processManager.ts
• src/lib/vscode/serviceTierVariants.ts
• src/server-init.ts
• src/server/ws/liveServer.ts
• tests/e2e/quota-store.e2e.ts
• tests/unit/quota/keyvQuotaStoreExtras.test.ts
• tests/unit/quota/quotaStore.contract.test.ts

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Never restore plaintext after encryption returns null.

The ?? conn.apiKey fallbacks reintroduce plaintext when encrypt() returns null. This violates the fail-closed requirement and also covers the getStaticKey()-returns-null path while the environment variable is set.

Reject the connection instead. Require a valid encrypted value for every populated credential field before returning or persisting the object.

Proposed fail-closed shape

-    if (conn.apiKey) conn.apiKey = encrypt(conn.apiKey) ?? conn.apiKey;
+    if (conn.apiKey) {
+      const encryptedApiKey = encrypt(conn.apiKey);
+      if (!encryptedApiKey || !encryptedApiKey.startsWith(PREFIX)) return null;
+      conn.apiKey = encryptedApiKey;
+    }

Apply the same validation to every credential field.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plans/encryption-failclosed-spec.md` around lines 421 - 429, Update
encryptConnectionFields so every populated credential field—apiKey, accessToken,
refreshToken, and idToken—must receive a non-null encrypted value from
encrypt(); remove the plaintext fallbacks. If any encryption result is null,
reject the connection and do not return or persist the partially encrypted
object, including when getStaticKey() returns null while encryption is enabled.

[sonarqubecloud]

Quality Gate failed

Failed conditions
12.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[kilo-code-bot]

CRITICAL: Behavior regression — getProcessInfo now returns { alive: false } on any error, including transient failures like /proc/<pid>/status permission errors or NFS stale handles. Previously the catch returned { alive: true }, so a healthy process that was temporarily unreadable was not falsely reported as dead. Callers that restart processes based on alive: false may now restart healthy processes unnecessarily.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: log.error({ err: message }, ...) passes the error message string as the err field instead of the actual Error object. This loses stack-trace context needed for debugging and is inconsistent with other error logging in the same file (e.g., line 213 uses { err }).

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: Same issue as nearby line 1184: log.error({ err: (err as Error)?.message ?? String(err) }, ...) logs only the message string, not the Error object. Stack-trace context is lost.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: Logs the raw err object from require("@/lib/dataPaths"). Error objects may carry stack traces containing internal filesystem paths. This is a minor information-disclosure vector in a credentials-loading path. Consider logging only err.message or a sanitized summary.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

WARNING: recordPlanUsage keys entries as plan:${connectionId}:${provider} and ignores the poolId parameter in the key. Two calls with the same connectionId/provider but different poolId values will overwrite each other in Keyv, silently losing pool-scoped usage data.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

SUGGESTION: getKeyv() exposes the internal Keyv instance. Any caller can bypass store invariants (e.g., the dual-write pool-bucket tracking in consume()), mutating the backend without the store's bookkeeping. Consider making this private or exposing only the operations needed by KeyvQuotaStoreExtras.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: 6 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	1
WARNING	4
SUGGESTION	1

Issue Details (click to expand)

CRITICAL

File	Line	Issue
src/lib/versionManager/processManager.ts	160	Behavior regression — getProcessInfo now returns { alive: false } on any error, including transient failures like /proc/<pid>/status permission errors or NFS stale handles. Previously the catch returned { alive: true }, so a healthy process that was temporarily unreadable was not falsely reported as dead. Callers that restart processes based on alive: false may now restart healthy processes unnecessarily.

WARNING

File	Line	Issue
src/lib/db/core.ts	1184	log.error({ err: message }, ...) passes the error message string as the err field instead of the actual Error object. This loses stack-trace context needed for debugging and is inconsistent with other error logging in the same file (e.g., line 213 uses { err }).
src/lib/db/core.ts	1501	Same issue as nearby line 1184: log.error({ err: (err as Error)?.message ?? String(err) }, ...) logs only the message string, not the Error object. Stack-trace context is lost.
open-sse/config/credentialLoader.ts	53	Logs the raw err object from require("@/lib/dataPaths"). Error objects may carry stack traces containing internal filesystem paths. This is a minor information-disclosure vector in a credentials-loading path. Consider logging only err.message or a sanitized summary.
src/lib/quota/keyvQuotaStoreExtras.ts	50	recordPlanUsage keys entries as plan:${connectionId}:${provider} and ignores the poolId parameter in the key. Two calls with the same connectionId/provider but different poolId values will overwrite each other in Keyv, silently losing pool-scoped usage data.

SUGGESTION

File	Line	Issue
src/lib/quota/keyvQuotaStore.ts	76	getKeyv() exposes the internal Keyv instance. Any caller can bypass store invariants (e.g., the dual-write pool-bucket tracking in consume()), mutating the backend without the store's bookkeeping. Consider making this private or exposing only the operations needed by KeyvQuotaStoreExtras.

Files Reviewed (62 files)

• .mergify.yml
• electron/package.json
• .github/workflows/build-fork.yml
• .github/workflows/ci.yml
• .github/workflows/docker-publish.yml
• .github/workflows/trunk-check.yml
• .gitignore
• open-sse/config/credentialLoader.ts — 1 issue
• open-sse/services/tierResolver.ts
• src/lib/quota/storeFactory.ts
• src/lib/resilience/anomalyHook.ts
• src/lib/machineToken.ts
• src/lib/versionManager/processManager.ts — 1 issue
• src/server-init.ts
• src/server/ws/liveServer.ts
• tests/unit/quota/keyvQuotaStoreExtras.test.ts
• src/lib/db/encryption.ts
• open-sse/services/autoRefreshDaemon.ts
• src/lib/cloudSync.ts
• src/lib/oauth/providers/antigravity.ts
• src/lib/vscode/serviceTierVariants.ts
• src/lib/db/adapters/betterSqliteAdapter.ts
• src/lib/db/adapters/nodeSqliteAdapter.ts
• src/lib/db/adapters/nodeSqliteShared.ts
• src/lib/db/adapters/sqljsAdapter.ts
• src/lib/versionManager/binaryManager.ts
• .github/workflows/cross-platform.yml
• plans/encryption-failclosed-spec.md
• plans/keyv-as-embedded-default-spec.md
• .agileplus/agileplus.db
• .agileplus/agileplus.db-shm
• .agileplus/agileplus.db-wal
• .agileplus/quota-keystore-type-drift/spec.md
• plans/quota-keystore-type-drift-spec.md
• src/lib/quota/keyvQuotaStore.ts — 1 issue
• src/lib/quota/keyvQuotaStoreExtras.ts — 1 issue
• src/lib/quota/types.ts
• tests/e2e/quota-store.e2e.ts
• tests/unit/quota/quotaStore.contract.test.ts
• src/lib/db/adapters/driverFactory.ts
• src/lib/db/apiKeys.ts
• src/lib/db/backup.ts
• src/lib/db/cleanup.ts
• src/lib/db/core.ts — 2 issues
• src/lib/db/migrationRunner.ts
• src/lib/db/models.ts
• src/lib/db/optimizationSettings.ts
• src/lib/db/providers.ts
• src/lib/db/quotaPools.ts
• src/lib/db/quotaSnapshots.ts
• src/lib/db/schemaColumns.ts
• src/lib/db/sessionAccountAffinity.ts
• src/lib/db/settings.ts
• src/lib/db/settings/cacheMetrics.ts
• src/lib/db/stateReset.ts
• package-lock.json
• package.json

Fix these issues in Kilo Cloud

---

_{Reviewed by step-3.7-flash · Input: 161.3K · Output: 19.9K · Cached: 4.9M}

[KooshaPari]

Closing as superseded. Branch commits already in main.