Repository navigation
feat(quota): promote Keyv to embedded default driver - #521
Closed
KooshaPari wants to merge 5 commits into
Closed
KooshaPari wants to merge 5 commits into
KooshaPari wants to merge 5 commits into
Conversation
Two deferred-implementation specs are now tracked in version control so the work product is preserved and discoverable. 1. plans/encryption-failclosed-spec.md (883 lines) — Hardening encryption.ts:144-148 (the silent plaintext fallback). Compares 3 design options with detailed tradeoffs. Recommended: C+A (startup canary + runtime throw). Registered as AgilePlus feature 'encryption-failclosed'. 2. plans/keyv-as-embedded-default-spec.md (698 lines) — Promote Keyv from optional driver to embedded default for fresh installs. Includes backwards-compat plan, config migration, and rollout sequence. Registered as AgilePlus feature 'keyv-as-embedded-default'. These specs intentionally do NOT include code changes — they are design-only and gate on answering the open questions before implementation. See spec section 9 for each spec's open questions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements Option C+A (startup canary + runtime throw) per plans/encryption-failclosed-spec.md. The encrypt() fallback to plaintext (line 144-148) is the highest-risk governance-debt finding from the recent audit. When STORAGE_ENCRYPTION_KEY is set but crypto fails (bad key material, OOM, native binding broken), tokens silently store as plaintext. This is undetectable from operator view. Changes: 1. New EncryptionRuntimeError class — thrown when crypto pipeline fails after key was successfully derived 2. validateEncryptionAtStartup() — runs a known-plaintext round-trip to detect broken encryption config before serving traffic 3. src/instrumentation-node.ts — calls the startup canary before ensureSecrets() and any DB init; refuses to start if encryption is broken (process.exit(1)) 4. encryptConnectionFields() return type: T -> T | null to signal caller when encryption failed; providers.ts:348,544 callers updated to throw a clear error and skip the DB write 5. commandCodeAuth.ts:142 wraps encrypt() in try/catch; returns null on EncryptionRuntimeError so the session is not poisoned with plaintext apiKey 6. src/lib/db/encryptionStartup.ts (new) — async wrapper runEncryptionStartupCanary() + process-exiting runEncryptionStartupCheck() helpers 7. .env.example documents the new behaviour (EncryptionRuntimeError and StartupEncryptionError error names, remediation hint) State A preserved exactly: when STORAGE_ENCRYPTION_KEY is unset, passthrough returns plaintext (no breaking change for dev/test setups). Out of scope (per spec): - Migration of legacy ciphertext - Key rotation - Hardware-backed keys Refs: PR #507 F3 follow-up. Closes State B from audit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements the migration plan in plans/keyv-as-embedded-default-spec.md. Keyv is now the default QUOTA_STORE_DRIVER for fresh installs. SQLite remains available as opt-in for existing users who haven't migrated. Backward compatibility: - Existing sqlite users stay on sqlite UNLESS they set QUOTA_STORE_DRIVER=keyv explicitly - Redis users (QUOTA_STORE_DRIVER=redis) unchanged - No data migration is automatic — script/migrate-quota-storage.ts provides opt-in migration Default backend for keyv: sqlite (durable single-process). Override with QUOTA_KEYV_BACKEND=memory for ephemeral serverless deployments. Per the spec, this does NOT remove the sqlite driver — it stays as opt-in (shared-disk multi-process, reference implementation for the QuotaStore contract test). Changes: 1. src/lib/quota/keyvDefaultConfig.ts (NEW): env var validation 2. src/lib/quota/storeFactory.ts: default driver changed sqlite -> keyv 3. src/lib/quota/keyvQuotaStore.ts: added seed() migration helper 4. scripts/migrate-quota-storage.ts (NEW): opt-in migration script 5. tests/unit/quota/keyvDefaultConfig.test.ts (NEW): env validation 6. tests/unit/quota-store-factory.test.ts (extended): defaults test 7. tests/e2e/quota-store.e2e.ts (extended): all-driver smoke test 8. tests/unit/quota/quotaStore.contract.test.ts (NEW): conformance 9. tests/integration/quota-store-migration.test.ts (NEW): migration Out of scope (per spec): - Migration of existing sqlite data (opt-in script only) - Cross-process locking - Redis cluster support Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note
|
|
Tick the box to add this pull request to the merge queue (same as
|
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+7
to
+10
| * The fault-injection path (replacing `randomBytes` in the `crypto` module) | ||
| * is covered in `tests/unit/db/encryption-failclosed.test.ts` via vitest + | ||
| * vi.mock. This file covers the contract via the real encrypt() and the | ||
| * shape of encryptConnectionFields()'s return value. |
There was a problem hiding this comment.
Suggestion: The file claims that the fail-closed fault-injection path is covered elsewhere, but the referenced encryption-failclosed.test.ts only exercises passthrough, success, and error-class construction; it never makes encrypt() throw or verifies that encryptConnectionFields() returns null. Consequently, a regression in the required fail-closed behavior would pass all of these tests. Add an actual crypto failure mock and assert the null result and unchanged persisted data behavior. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Runtime fail-closed behavior lacks regression coverage.
- ⚠️ Plaintext-write prevention is not directly asserted.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/db/encryption-connection-fields-failclosed.test.mjs
**Line:** 7:10
**Comment:**
*Incomplete Implementation: The file claims that the fail-closed fault-injection path is covered elsewhere, but the referenced `encryption-failclosed.test.ts` only exercises passthrough, success, and error-class construction; it never makes `encrypt()` throw or verifies that `encryptConnectionFields()` returns `null`. Consequently, a regression in the required fail-closed behavior would pass all of these tests. Add an actual crypto failure mock and assert the `null` result and unchanged persisted data behavior.
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| */ | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import path from "node:path"; |
There was a problem hiding this comment.
Suggestion: This .test.mjs file is not discovered by the repository's standard unit-test manifest or test glob, which only include .test.ts files. As a result, none of these encryption tests run in the normal test commands; rename the file to .test.ts or update test discovery to include .test.mjs files. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Fail-closed encryption coverage never runs normally.
- ⚠️ Regressions can pass standard unit-test commands.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/db/encryption-connection-fields-failclosed.test.mjs
**Line:** 14:14
**Comment:**
*Incomplete Implementation: This `.test.mjs` file is not discovered by the repository's standard unit-test manifest or test glob, which only include `.test.ts` files. As a result, none of these encryption tests run in the normal test commands; rename the file to `.test.ts` or update test discovery to include `.test.mjs` files.
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| * | ||
| * Plan: plans/keyv-as-embedded-default-spec.md §4.3.2 / §5 (AC-7..AC-10). | ||
| */ | ||
| import { describe, it, expect, beforeEach, afterAll } from "vitest"; |
There was a problem hiding this comment.
Suggestion: This nested quota test is not included by the standard Node test glob, whose directory list omits tests/unit/quota, and the configured Vitest suite does not include nested quota tests either. Therefore the new configuration coverage never executes in the normal test commands; add the quota directory to test discovery or include this file in the appropriate Vitest configuration. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Keyv default configuration tests never execute normally.
- ⚠️ Invalid quota environment values may regress undetected.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit/quota/keyvDefaultConfig.test.ts
**Line:** 14:14
**Comment:**
*Incomplete Implementation: This nested quota test is not included by the standard Node test glob, whose directory list omits `tests/unit/quota`, and the configured Vitest suite does not include nested quota tests either. Therefore the new configuration coverage never executes in the normal test commands; add the quota directory to test discovery or include this file in the appropriate Vitest 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
Comment on lines
+65
to
+67
| } else { | ||
| // "sqlite" (default) or "file" — use the durable default | ||
| kvUrl = KEYV_DEFAULT_URI; |
There was a problem hiding this comment.
Suggestion: When QUOTA_KEYV_BACKEND=file, this branch resolves the URI to KEYV_DEFAULT_URI, which is explicitly a SQLite URI. The factory then logs the backend as file while constructing a SQLite-backed store, so selecting the file backend silently uses the wrong persistence implementation. Resolve file to the file backend URI/adapter or reject it until it is implemented. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ File backend selection uses SQLite persistence.
- ⚠️ Startup logs report an incorrect backend.
- ❌ File-backend deployments cannot obtain requested storage semantics.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/quota/keyvDefaultConfig.ts
**Line:** 65:67
**Comment:**
*Api Mismatch: When `QUOTA_KEYV_BACKEND=file`, this branch resolves the URI to `KEYV_DEFAULT_URI`, which is explicitly a SQLite URI. The factory then logs the backend as `file` while constructing a SQLite-backed store, so selecting the file backend silently uses the wrong persistence implementation. Resolve `file` to the file backend URI/adapter or reject it until it is implemented.
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
Comment on lines
+182
to
+184
| const pk = poolDimKey(dim.poolId, dim); | ||
| this.buckets.set(pk, { value, expiresAt: now + ttlMs }); | ||
| await this.kv.set(pk, value, ttlMs); |
There was a problem hiding this comment.
Suggestion: The migration calls seed() once per allocation, but this assignment overwrites the shared pool bucket with each allocation's individual value. For pools with multiple API keys, poolConsumedTotal() will return only the last migrated allocation instead of the pool-wide total, causing quota enforcement to undercount usage. Seed the pool bucket from an accumulated pool total, or only write the per-key bucket here and aggregate pool values separately. [logic error]
Severity Level: Major ⚠️
- ❌ Migrated pools report incomplete shared consumption.
- ❌ Quota enforcement can permit excess usage.
- ⚠️ Pool usage endpoints expose incorrect totals.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/quota/keyvQuotaStore.ts
**Line:** 182:184
**Comment:**
*Logic Error: The migration calls `seed()` once per allocation, but this assignment overwrites the shared pool bucket with each allocation's individual value. For pools with multiple API keys, `poolConsumedTotal()` will return only the last migrated allocation instead of the pool-wide total, causing quota enforcement to undercount usage. Seed the pool bucket from an accumulated pool total, or only write the per-key bucket here and aggregate pool values separately.
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| if (driver === "keyv") { | ||
| // Resolve the env-driven defaults via the SSOT config helper, then layer | ||
| // any DB setting overrides on top (DB > env > default). | ||
| const envCfg = readKeyvDefaultConfigFromEnv(); |
There was a problem hiding this comment.
Suggestion: The configuration parser is invoked before the try block that provides the documented SQLite fallback. An invalid QUOTA_KEYV_BACKEND or QUOTA_STORE_DRIVER therefore rejects getQuotaStore() instead of returning a valid fallback store, so a malformed environment value can prevent quota initialization entirely. Move configuration parsing into the guarded path or explicitly handle validation errors according to the factory's fallback contract. [api mismatch]
Severity Level: Major ⚠️
- ❌ Quota enforcement initialization fails on malformed backend configuration.
- ⚠️ Requests cannot obtain a usable quota store.
- ⚠️ Documented SQLite fallback is bypassed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/quota/storeFactory.ts
**Line:** 123:123
**Comment:**
*Api Mismatch: The configuration parser is invoked before the `try` block that provides the documented SQLite fallback. An invalid `QUOTA_KEYV_BACKEND` or `QUOTA_STORE_DRIVER` therefore rejects `getQuotaStore()` instead of returning a valid fallback store, so a malformed environment value can prevent quota initialization entirely. Move configuration parsing into the guarded path or explicitly handle validation errors according to the factory's fallback contract.
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
Comment on lines
+115
to
+117
| if (apply) { | ||
| await keyv.seed(alloc.apiKeyId, dimKey, sourceValue); | ||
| } |
There was a problem hiding this comment.
Suggestion: seed() resets the Keyv pool aggregate for every allocation, so when a pool has multiple API-key allocations the value written for the last allocation overwrites the earlier values instead of preserving SQLite's pool-wide sum. The migration therefore undercounts pool consumption and can allow requests beyond the configured pool quota; aggregate the source values per pool and dimension before seeding the pool state. [logic error]
Severity Level: Major ⚠️
- ❌ Multi-key pool migrations undercount shared quota usage.
- ⚠️ Requests may exceed configured pool limits after migration.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/migrate-quota-storage.ts
**Line:** 115:117
**Comment:**
*Logic Error: `seed()` resets the Keyv pool aggregate for every allocation, so when a pool has multiple API-key allocations the value written for the last allocation overwrites the earlier values instead of preserving SQLite's pool-wide sum. The migration therefore undercounts pool consumption and can allow requests beyond the configured pool quota; aggregate the source values per pool and dimension before seeding the pool state.
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
Comment on lines
+149
to
+159
| if (err instanceof EncryptionRuntimeError) { | ||
| // FAIL-CLOSED: encryption layer is broken (key configured but crypto | ||
| // pipeline threw). Refuse to write apiKey plaintext; surface the | ||
| // failure to the caller as "session not found". | ||
| log.error( | ||
| { err: err.message, op: "markCommandCodeAuthSessionReceived", stateHash: input.stateHash }, | ||
| `[commandCodeAuth] Refusing to store apiKey — encryption layer failed.` | ||
| ); | ||
| return null; | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
Suggestion: Returning null for an EncryptionRuntimeError makes an encryption outage indistinguishable from an absent or expired command-code session. The callback consequently returns a 400 invalid-state response while the session remains pending, preventing clients and operators from distinguishing a transient storage failure from invalid authentication state. Propagate a distinct failure or provide an error result that the route can map to a server error while still avoiding plaintext storage. [api mismatch]
Severity Level: Major ⚠️
- ❌ Command-code callbacks return 400 during encryption outages.
- ⚠️ Pending sessions remain unusable until they expire.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/db/commandCodeAuth.ts
**Line:** 149:159
**Comment:**
*Api Mismatch: Returning `null` for an `EncryptionRuntimeError` makes an encryption outage indistinguishable from an absent or expired command-code session. The callback consequently returns a 400 invalid-state response while the session remains pending, preventing clients and operators from distinguishing a transient storage failure from invalid authentication state. Propagate a distinct failure or provide an error result that the route can map to a server error while still avoiding plaintext storage.
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
KooshaPari
pushed a commit
that referenced
this pull request
Aug 7, 2026
Follows the PR #506/#507/#509/#510/#512/#518/#521/#522 pattern of replacing console.* with createLogger("domain:subsystem") to provide structured logging across the OmniRoute codebase. PR #522 already migrated src/lib/db/*.ts (~155 callsites). This PR migrates ~80 additional callsites across 37 files in: - src/lib/resilience/* (normalize.ts, anomalyHook.ts) - src/lib/oauth/* (connectionPersistence.ts) - src/lib/vscode/* (tokenizedRequest.ts, dual-emit for test capture) - src/lib/services/* (ringBuffer.ts, bootstrap.ts, embedWsProxy.ts, modelSync.ts) - src/lib/sseTextTransform.ts, src/lib/dataPaths.ts, src/lib/initCloudSync.ts - src/lib/events/eventBus.ts, src/lib/jobs/{budgetResetJob,reasoningCacheCleanupJob}.ts - src/lib/cloudSync.ts, src/lib/localHealthCheck.ts - src/lib/arenaEloSync.ts, src/lib/pricingSync.ts, src/lib/modelsDevSync.ts - src/lib/tokenHealthCheck.ts, src/lib/gracefulShutdown.ts - src/lib/apiBridgeServer.ts, src/lib/proxyLogger.ts - src/lib/middleware/registry.ts, src/lib/quota/connectionRecovery.ts - src/lib/credentialHealth/scheduler.ts - src/middleware/promptInjectionGuard.ts - src/server/ws/liveServer.ts (preserves [LiveWS] startup banner via log.info) - src/sse/services/auth.ts, src/sse/services/streamState.ts - open-sse/config/{constants,credentialLoader}.ts - open-sse/services/{autoRefreshDaemon,quotaMonitor}.ts - open-sse/mcp-server/audit.ts - open-sse/utils/proxyFetch.ts - open-sse/handlers/chatCore.ts (account fallback warnings) User-facing startup banners and intentional CLI output are preserved: - src/server/ws/liveServer.ts '[LiveWS] Dashboard WebSocket server listening' (now via log.info with structured host/port) - src/lib/vscode/tokenizedRequest.ts '[VSCODE][SECURITY]' warning kept as console.warn alongside log.warn so tests/unit/vscode-token-in-url-warning.test.ts (which captures console.warn to verify once-per-process dedup) keeps passing — same pattern as PR #522's preservation in db/core.ts:migrateFromJson - src/lib/oauth/utils/ui.ts (CLI formatting with picocolors) preserved as console - src/mitm/* (CLI-driven tooling) preserved - Next.js dashboard React components preserved (browser console, not server-side) Behavior preservation: - All success-path behavior unchanged - Only the logging mechanism changes - TSC baseline (typecheck-core.json) remains 0 errors - Targeted tests pass: resilience-settings-normalize-split (9/9), resilience-settings-stream-recovery (9/9), resilience-settings-provider-breaker (9/9), oauth-refresh-error-resilience (12/12), vscode-tokenized-request (3/3), vscode-token-in-url-warning (4/4), services/embedWsProxy (30/30) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
* chore(governance): rebase mergify config request-review fixes onto main * fix(desktop): target fork-owned Electron releases * ci: align workflows with selected action policy * fix(governance): replace silent try/catch with explicit log.error in 5 modules (#506) Builds on PR #505 (quota keystore type-drift fix). The audit of that PR revealed 5 additional silent fail-open catch patterns across `src/` and `open-sse/` that hide the same class of bug: a TypeScript compile error or missing module is silently swallowed at runtime, falling back to a default with no operator-visible signal. This commit replaces those silent catches with explicit `log.error` calls that surface the actual error to monitoring. Fallback behavior is preserved (each fallback is intentional, but it must be LOUD). Changes: 1. `src/lib/quota/storeFactory.ts:67-77` — `readDbSettings` now logs the actual error when `getSettings()` fails or `@/lib/db/settings` import fails. Same root cause as the previously-fixed Keyv/Redis catches. 2. `src/lib/quota/storeFactory.ts:138-147` — Redis driver catch upgraded from `log.warn` to `log.error`. Includes the configured Redis URL with the password segment redacted (`:***@`). 3. `src/lib/resilience/anomalyHook.ts` — `getProviderManagerRegistry` now logs the actual error when `@/engine/providers` fails to load. Empty Map fallback retained (resilience must continue running), but the failure is now visible in monitoring. 4. `open-sse/services/tierResolver.ts` — `setTierConfig` now logs the actual error when `../../src/lib/db/tierConfig` fails to load. `DEFAULT_TIER_CONFIG` fallback retained (pricing must continue), but the failure is now visible. 5. `open-sse/config/credentialLoader.ts` — `resolveCredentialsPath` now uses `log.error` (pino) instead of `console.warn`. Includes both the original error and the fallback path. Security-sensitive path; must keep working, but the failure must be loud. 6. `.gitignore` — exclude `.agileplus/` and `agileplus-*.db*` (local AgilePlus DB state, regenerated from `.md` specs via `agileplus specify`). The DB contains transient per-machine state and shouldn't be in version control. Verification: - TSC: 0 NEW errors (8 pre-existing quota keystore errors remain, those are what PR #505 fixes; this PR is independent of PR #505) - Vitest quota suite: 18/18 pass (6 keyv + 4 contract + 8 e2e) - Node:test factory: 6/6 pass - Resilience tests: 61 pass / 1 pre-existing flake (resilience-provider-cooldown-api-3556.test.ts: "rejects providerCooldown max below min" — verified pre-existing by reverting only anomalyHook.ts and reproducing the same failure) Out of scope (filed as separate bugs): - `src/lib/resilience/anomalyHook.ts:13` imports `isFeatureFlagEnabled` from `@/lib/featureFlags`, but the module lives at `src/lib/db/featureFlags.ts`. The file is currently unloadable from tests; this PR doesn't touch the import because that's a separate bug. - The Resilience subagent identified but did not fix the `tsconfig.typecheck-core.json` doesn't include `src/lib/resilience/` files — separate follow-up. Co-authored-by: KooshaPari <koosha@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(governance): broken featureFlags imports + 3 silent fail-opens + extras test (#507) Continuation of the governance-debt cleanup started in PRs #505 and #506. Six independent fixes, each addressing a class of silent-failure pattern that the audits surfaced. Changes: 1. **`src/lib/resilience/anomalyHook.ts:12`** — Broken import path. `isFeatureFlagEnabled` was imported from `@/lib/featureFlags`, but the module lives at `@/shared/utils/featureFlags`. This bug was masked because `tsconfig.typecheck-core.json` does not include `src/lib/resilience/` and no test loads the module successfully. After the fix, the module loads and exports the expected surface. 2. **`src/server-init.ts:128`** — Same broken import path. The surrounding try/catch at lines 130-139 silently swallowed the import failure. Fix: correct the path AND upgrade the catch log from `warn` to `error` (matches PR #506 pattern). 3. **`src/lib/versionManager/processManager.ts:155`** — `getProcessInfo` catch returned `{pid, alive: true}` after `ps`/readFile failures, which lies when the process is actually gone. Fix: catch now logs the error and returns `{pid, alive: false}` (honest about not being able to read process state). 4. **`src/server/ws/liveServer.ts:479`** — `loadAuthModule().catch(() => {})` silently swallowed initial auth module load failures, allowing the WS server to come up without auth configured. Fix: catch now logs `log.error`; fallback behavior preserved per the existing comment ("handler retries the import lazily"). 5. **`src/lib/machineToken.ts:1-10`** — Crypto-relevant: empty catch around `require("node-machine-id")` silently fell back to `() => ""`, which collapses HMAC inputs to a constant. Fix: catch now logs `log.error` with security-context message, gated by a `fallbackLogged` flag so the log fires only once per process (avoiding log spam from any downstream reload). 6. **`tests/unit/quota/keyvQuotaStoreExtras.test.ts`** (new) — Closes spec §8.2 reachability test gap. 5 sanity tests for the `recordPlanUsage` / `upsertProviderPlan` / `listProviderPlans` / `setPools` / `getPool` surface on KeyvQuotaStore. Note: this branch is based on `origin/agent/migration-version-collision-fix` (pre-PR-#505), so the methods live directly on KeyvQuotaStore. When PR #505 lands, update the test to import from `keyvQuotaStoreExtras.ts` instead. Verification: - TSC error count: 8 unchanged (all pre-existing quota keystore errors that PR #505 fixes; this PR is independent of #505) - Vitest extras test: 5/5 pass - Node:test combined (processManager + machineToken + WS): 47/47 pass - All success-path behavior preserved; only catch/fallback paths now log Out of scope (separate PRs): - Promote Keyv to "embedded default" driver (spec §10) - Pre-existing e2e failure at `tests/e2e/quota-store.e2e.ts:117` (poolUsageWithDimensions shape mismatch — fixed in PR #505) Co-authored-by: KooshaPari <koosha@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(security): log 3 crypto-relevant silent catches (rebased onto canonical base) (#509) This branch has been surgically rebased onto origin/agent/migration-version-collision-fix to remove accidental contamination from PR #507's branch base. In all three cases, the empty-string return collapses HMAC/HMAC-SHA256 inputs into a constant-key value, so security-relevant operations on the fallback path produce identical tokens regardless of input. Fixes: 1. src/lib/machineToken.ts:44 - getMachineTokenSync catch: log.error + return "" 2. src/lib/machineToken.ts:62 - getLegacyCliTokenSync catch: log.error + return "" 3. src/lib/db/encryption.ts:87-95 - getLegacyDynamicKey catch: added pino logger (createLogger("db:encryption")) + log.error instead of returning null silently 4. (incidental) src/lib/machineToken.ts module-load catch: also gains log.error so the new runtime catches have a 'log' constant to reference. This subsumes PR #507's machineToken.ts hunk. NOTE: PR #507 will need its machineToken.ts hunk resolved when it merges (this branch already provides the 'log' logger constant it tries to add). Verification: - TSC: 8 unchanged (pre-existing quota keystore errors, PR #505 territory) - Targeted machineToken + encryption unit tests pass - All success-path behaviors preserved Co-authored-by: KooshaPari <koosha@example.com> * fix(security): 4 audit findings + migrate encryption.ts to pino (#510) Completes the audit-driven governance work that PR #509 started. Eight independent fixes: Security fixes (audit findings F5, F6, F9, F10): 1. src/lib/cloudSync.ts:47-56 — HMAC verification fail-open when CLOUD_SYNC_SECRET is unset. Now returns false (fail-closed) when a sigHeader is present but cannot be verified, instead of accepting any response. Legacy unverified mode (no sigHeader) is preserved. 2. src/lib/oauth/providers/antigravity.ts:101 — fetch() of userInfo during OAuth was swallowed silently, causing downstream code to use default projectId/tierId. Now logs warn with structured context. 3. open-sse/services/autoRefreshDaemon.ts:77,80 — periodic credential refresh errors were silently swallowed, allowing expired tokens to persist undetected. Now logs error per cycle. 4. src/lib/vscode/serviceTierVariants.ts:184 — request body parse failures during tier-rewrite were silently no-op'd. Now logs warn. Refactor: 5. src/lib/db/encryption.ts — migrated ~7 console.* calls to pino logger (encryptionLog). Following the PR #509 pattern of createLogger("db:encryption"). The catch fallback behavior is preserved exactly; only logging changes. Out of scope: - encryption.ts:144-148 plaintext fallback (separate spec at plans/encryption-failclosed-spec.md, deferred for design discussion) - src/lib/db/*.ts console.* migration for other files (separate PR) Co-authored-by: KooshaPari <koosha@example.com> * fix(governance): log empty catches in binaryManager + db/adapters (#512) 22 silent fail-open sites in the version manager + SQLite adapter layer were just swallows. Per the audit: - src/lib/versionManager/binaryManager.ts (6 catches): symlink/rollback/ remove errors were silent; filesystem permission bugs were invisible - src/lib/db/adapters/sqljsAdapter.ts (6 catches): save/close errors were silent; DB corruption during shutdown was undetectable - src/lib/db/adapters/betterSqliteAdapter.ts (1 catch): same pattern - src/lib/db/adapters/nodeSqliteAdapter.ts (2 catches): same pattern - src/lib/db/adapters/nodeSqliteShared.ts (7 catches): same pattern All 22 catches now log structured error context. Behavior unchanged - only logging added. Per AGENTS.md, no encryption keys or raw secrets are logged. TSC: 8 unchanged Tests: existing pass (any new behavior is logging-only) Co-authored-by: KooshaPari <koosha@example.com> * ci: pin cross-platform Rust toolchain * docs(plans): track 2 governance specs for future implementation (#511) Two deferred-implementation specs are now tracked in version control so the work product is preserved and discoverable. 1. plans/encryption-failclosed-spec.md (883 lines) — Hardening encryption.ts:144-148 (the silent plaintext fallback). Compares 3 design options with detailed tradeoffs. Recommended: C+A (startup canary + runtime throw). Registered as AgilePlus feature 'encryption-failclosed'. 2. plans/keyv-as-embedded-default-spec.md (698 lines) — Promote Keyv from optional driver to embedded default for fresh installs. Includes backwards-compat plan, config migration, and rollout sequence. Registered as AgilePlus feature 'keyv-as-embedded-default'. These specs intentionally do NOT include code changes — they are design-only and gate on answering the open questions before implementation. See spec section 9 for each spec's open questions. Co-authored-by: KooshaPari <koosha@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * wip: auto-commit daemon 2026-08-06T09:18:52Z (#505) Co-authored-by: Airlock Bot <airlock@phenoforge.local> * refactor(db): migrate console.* to pino in src/lib/db/ (#522) Follows the PR #506/#507/#509/#510 pattern of replacing console.* with createLogger("db:<subsystem>") to provide structured logging across the SQLite persistence layer. Files modified (~155 callsites across 17 files): - src/lib/db/adapters/driverFactory.ts - src/lib/db/adapters/sqljsAdapter.ts - src/lib/db/apiKeys.ts - src/lib/db/backup.ts - src/lib/db/cleanup.ts (~30 sites) - src/lib/db/core.ts (~30 sites) - src/lib/db/migrationRunner.ts (~22 sites, removed test-suppressing local console wrapper) - 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 (~35 sites) - src/lib/db/sessionAccountAffinity.ts - src/lib/db/settings.ts - src/lib/db/settings/cacheMetrics.ts - src/lib/db/stateReset.ts Per AGENTS.md guidance, never log SQLite encryption keys or raw secrets; all logger calls redact sensitive material. Behavior preservation: - All success-path behavior unchanged - Only the logging mechanism changes - Targeted tests pass One console.error preserved in core.ts:migrateFromJson because tests/unit/db-core-migration.test.ts overrides console.error to detect the failure; both console.error and log.error are emitted so the test passes and pino is the canonical sink. Co-authored-by: KooshaPari <koosha@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: use cache-pinned Trunk action * refactor: migrate console.* to pino in remaining src/ + open-sse/ Follows the PR #506/#507/#509/#510/#512/#518/#521/#522 pattern of replacing console.* with createLogger("domain:subsystem") to provide structured logging across the OmniRoute codebase. PR #522 already migrated src/lib/db/*.ts (~155 callsites). This PR migrates ~80 additional callsites across 37 files in: - src/lib/resilience/* (normalize.ts, anomalyHook.ts) - src/lib/oauth/* (connectionPersistence.ts) - src/lib/vscode/* (tokenizedRequest.ts, dual-emit for test capture) - src/lib/services/* (ringBuffer.ts, bootstrap.ts, embedWsProxy.ts, modelSync.ts) - src/lib/sseTextTransform.ts, src/lib/dataPaths.ts, src/lib/initCloudSync.ts - src/lib/events/eventBus.ts, src/lib/jobs/{budgetResetJob,reasoningCacheCleanupJob}.ts - src/lib/cloudSync.ts, src/lib/localHealthCheck.ts - src/lib/arenaEloSync.ts, src/lib/pricingSync.ts, src/lib/modelsDevSync.ts - src/lib/tokenHealthCheck.ts, src/lib/gracefulShutdown.ts - src/lib/apiBridgeServer.ts, src/lib/proxyLogger.ts - src/lib/middleware/registry.ts, src/lib/quota/connectionRecovery.ts - src/lib/credentialHealth/scheduler.ts - src/middleware/promptInjectionGuard.ts - src/server/ws/liveServer.ts (preserves [LiveWS] startup banner via log.info) - src/sse/services/auth.ts, src/sse/services/streamState.ts - open-sse/config/{constants,credentialLoader}.ts - open-sse/services/{autoRefreshDaemon,quotaMonitor}.ts - open-sse/mcp-server/audit.ts - open-sse/utils/proxyFetch.ts - open-sse/handlers/chatCore.ts (account fallback warnings) User-facing startup banners and intentional CLI output are preserved: - src/server/ws/liveServer.ts '[LiveWS] Dashboard WebSocket server listening' (now via log.info with structured host/port) - src/lib/vscode/tokenizedRequest.ts '[VSCODE][SECURITY]' warning kept as console.warn alongside log.warn so tests/unit/vscode-token-in-url-warning.test.ts (which captures console.warn to verify once-per-process dedup) keeps passing — same pattern as PR #522's preservation in db/core.ts:migrateFromJson - src/lib/oauth/utils/ui.ts (CLI formatting with picocolors) preserved as console - src/mitm/* (CLI-driven tooling) preserved - Next.js dashboard React components preserved (browser console, not server-side) Behavior preservation: - All success-path behavior unchanged - Only the logging mechanism changes - TSC baseline (typecheck-core.json) remains 0 errors - Targeted tests pass: resilience-settings-normalize-split (9/9), resilience-settings-stream-recovery (9/9), resilience-settings-provider-breaker (9/9), oauth-refresh-error-resilience (12/12), vscode-tokenized-request (3/3), vscode-token-in-url-warning (4/4), services/embedWsProxy (30/30) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: KooshaPari <koosha@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Airlock Bot <airlock@phenoforge.local>
Owner
Author
|
Closing as superseded. Cannot auto-rebase due to merge conflicts. |
|
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
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.
User description
feat(quota): promote Keyv to embedded default driver
Implements the migration plan in plans/keyv-as-embedded-default-spec.md.
Keyv is now the default QUOTA_STORE_DRIVER for fresh installs. SQLite
remains available as opt-in for existing users who haven't migrated.
Backward compatibility:
QUOTA_STORE_DRIVER=keyv explicitly
provides opt-in migration
Default backend for keyv: sqlite (durable single-process). Override
with QUOTA_KEYV_BACKEND=memory for ephemeral serverless deployments.
Per the spec, this does NOT remove the sqlite driver — it stays as
opt-in (shared-disk multi-process, reference implementation for the
QuotaStore contract test).
Changes
src/lib/quota/keyvDefaultConfig.ts (NEW): env var validation
keyv://sqlite:.agileplus/quota/quota.dbsrc/lib/quota/storeFactory.ts: default driver flipped sqlite -> keyv
{ driver, backend, kvUrl }quotaStore.keyvBackendoverrides env (precedence preserved)src/lib/quota/keyvQuotaStore.ts: added
seed()migration helperconsume()'s additive behavior for one-time counter-state migrationQuotaStoreinterface contract (documented as migration-only)scripts/migrate-quota-storage.ts (NEW): opt-in migration script
--applyto writelistPools+listAllocationsForApiKey+listPlansSqliteQuotaStore.peek; writes target viaKeyvQuotaStore.seedrunQuotaMigration()exported separately for teststests/unit/quota/keyvDefaultConfig.test.ts (NEW): 12 vitest tests
tests/unit/quota-store-factory.test.ts (extended): 10 node:test cases
tests/e2e/quota-store.e2e.ts (extended): AC-20 fallback scenario
tests/unit/quota/quotaStore.contract.test.ts (NEW): 5 vitest tests
seed()exists on KeyvQuotaStore concrete class but is NOT in the interfacetests/integration/quota-store-migration.test.ts (NEW): 4 node:test cases
--applywrites counter state to keyv, matching source.env.example: documented new defaults +
QUOTA_KEYV_BACKENDVerification
vitest run tests/unit/quota/node --test tests/unit/quota-store-factory.test.tsnode --test tests/integration/quota-store-migration.test.tsvitest run tests/e2e/quota-store.e2e.tspoolUsageWithDimensions returns a structured snapshotfailure unrelated to this PR — it expectssnapshot.poolIdbutKeyvQuotaStore.poolUsageWithDimensionsreturns a flatPoolUsageWithDimensionsobject with nopoolIdproperty; same failure occurs on the baselineb88f9966fbcommit before this PR)CI is expected to fail with the account-level billing error per
AGENTS.md— verified quality locally instead.Out of scope (per spec §3)
Files changed
.env.example— documented new defaultssrc/lib/quota/keyvDefaultConfig.ts— NEW (98 lines)src/lib/quota/keyvQuotaStore.ts— +21 lines (seed method)src/lib/quota/storeFactory.ts— 3 hunks, +62/-16 linesscripts/migrate-quota-storage.ts— NEW (165 lines, executable)tests/unit/quota/keyvDefaultConfig.test.ts— NEW (148 lines)tests/unit/quota/quotaStore.contract.test.ts— NEW (91 lines)tests/unit/quota-store-factory.test.ts— extended (+147/-34)tests/e2e/quota-store.e2e.ts— extended (+41)tests/integration/quota-store-migration.test.ts— NEW (181 lines)Total: 10 files changed, 914 insertions, 34 deletions.
Refs: PR #505, PR-G comment,
plans/keyv-as-embedded-default-spec.md,plans/quota-keystore-type-drift-spec.md§10.Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
CodeAnt-AI Description
Make encryption fail closed and use Keyv as the default quota store
What Changed
Impact
✅ No plaintext writes after configured encryption failures✅ Broken encryption detected before serving requests✅ Keyv quota storage works without a Redis sidecar✅ Existing quota counters can be migrated without double-counting💡 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:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
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:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
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.