fix(quota): rewrite KeyvQuotaStore to satisfy QuotaStore interface (P0 type-drift) - #505
Merged
KooshaPari merged 1 commit intoAug 7, 2026
Conversation
🤖 CodeAnt AI — Review Status
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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
|
codeant-ai
Bot
added
the
size:XXL
|
Tick the box to add this pull request to the merge queue (same as
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+21
to
+28
| const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-keyv-e2e-")); | ||
| process.env.DATA_DIR = TEST_DATA_DIR; | ||
| process.env.DISABLE_SQLITE_AUTO_BACKUP = "true"; | ||
|
|
||
| import { KeyvQuotaStore } from "@/lib/quota/keyvQuotaStore"; | ||
| import type { DimensionKey } from "@/lib/quota/dimensions"; | ||
| import { getDbInstance } from "@/lib/db/core"; | ||
| import { createPool } from "@/lib/db/quotaPools"; |
There was a problem hiding this comment.
Suggestion: The DATA_DIR override is assigned in the test module body, but the static imports of @/lib/db/core and @/lib/db/quotaPools are evaluated before that body executes. Since core.ts computes and exports DATA_DIR during module evaluation, the database can bind to the normal application directory instead of TEST_DATA_DIR; the test then writes shared state and removes only the temporary directory during cleanup. Set the environment in Vitest setup or dynamically import the DB modules after the override. [test isolation]
Severity Level: Major ⚠️
- ❌ E2E runs can write outside their temporary directory.
- ⚠️ Test state can leak into shared application storage.
- ⚠️ Cleanup may remove no files actually created by the test.
(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:** 21:28
**Comment:**
*Test Isolation: The `DATA_DIR` override is assigned in the test module body, but the static imports of `@/lib/db/core` and `@/lib/db/quotaPools` are evaluated before that body executes. Since `core.ts` computes and exports `DATA_DIR` during module evaluation, the database can bind to the normal application directory instead of `TEST_DATA_DIR`; the test then writes shared state and removes only the temporary directory during cleanup. Set the environment in Vitest setup or dynamically import the DB modules after the override.
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
| // Mirror to pool bucket (used for `poolUsage` aggregates). | ||
| // Mirror to pool bucket (used for `poolConsumedTotal` aggregates). | ||
| const pk = poolDimKey(dim.poolId, dim); | ||
| const pCurrent = this.buckets.get(pk); |
There was a problem hiding this comment.
Suggestion: The pool bucket expiration is reset to now + ttlMs on every consumption, so continuous traffic keeps the aggregate alive indefinitely instead of expiring according to the quota window. After sustained traffic, poolConsumedTotal() can include usage older than the configured window and block requests based on stale consumption. Store fixed window buckets or preserve the original bucket expiration rather than refreshing it on every increment. [logic error]
Severity Level: Major ⚠️
- ❌ Sustained traffic retains stale pool consumption.
- ❌ Quota enforcement can block valid later requests.
- ⚠️ Keyv window behavior diverges from fixed-window 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/keyvQuotaStore.ts
**Line:** 90:93
**Comment:**
*Logic Error: The pool bucket expiration is reset to `now + ttlMs` on every consumption, so continuous traffic keeps the aggregate alive indefinitely instead of expiring according to the quota window. After sustained traffic, `poolConsumedTotal()` can include usage older than the configured window and block requests based on stale consumption. Store fixed window buckets or preserve the original bucket expiration rather than refreshing it on every increment.
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![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // line below fails to typecheck. This catches drift before tests even run. | ||
| // (vitest does not run tsc on test files; this assertion fires when the | ||
| // project-wide `tsc -p tsconfig.typecheck-core.json` sweep picks the file up.) | ||
| type _SqliteConforms = SqliteQuotaStore extends QuotaStore ? true : false; |
There was a problem hiding this comment.
WARNING: Type assertions are unchecked by project typecheck config
These compile-time assertions (_SqliteConforms, _RedisConforms, _KeyvConforms) are designed to catch interface drift, but tsconfig.typecheck-core.json does not include this file. The project's tsc -p tsconfig.typecheck-core.json command (used for AC-1) will not validate them, making the "drift prevention" mechanism dead code.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| beforeEach(() => { | ||
| // Initialize DB (applies migrations) and create the test pool fixture. | ||
| // `getDbInstance()` is the singleton; calling it ensures migrations run. | ||
| getDbInstance(); |
There was a problem hiding this comment.
WARNING: getDbInstance() caches a singleton that is never reset between tests
beforeEach calls getDbInstance() which returns a cached singleton SQLite connection. afterEach deletes the temp DATA_DIR but does not call resetDbInstance(), so subsequent tests reuse the same DB instance and share pool data across the file. Current tests pass because each test uses unique pool IDs and API key IDs, but this is a hidden coupling that can cause flaky failures when future tests rely on clean DB state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 123.4K · Output: 27K · Cached: 4.1M |
|
This was referenced Aug 6, 2026
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
…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>
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
…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>
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
…onical 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>
KooshaPari
merged commit Aug 7, 2026
ec599ef
into
agent/migration-version-collision-fix
16 of 18 checks passed
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 --------- 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>
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
…ollowup) (#526) * 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 * fix(governance): log empty catches + weak peer-dep fallbacks (audit followup) Continues the audit-driven governance cleanup that PRs #506, #507, #509, #510, #512, #522, #525 started. This batch addresses the remaining empty catches and weak peer-dependency fallbacks in plugins, embeddings, oauth, monitoring, and combo paths. Empty catches fixed (~6 sites): - src/lib/plugins/loader.ts:200, 301 (plugin load/cleanup) - src/lib/plugins/manager.ts:151, 270 (plugin staging cleanup) - src/lib/embeddings/service.ts:50, 73 (embedding combo lookup) - src/lib/credentialHealth/scheduler.ts:122 (credential health sweep) - src/lib/usage/usageStats.ts:301 (usage stats) - src/lib/oauth/providers/kimi-coding.ts:44 (OAuth provider init) Weak peer-dep fallbacks fixed (~10 sites): - src/lib/a2a/skills/providerDiscovery.ts:388 (MCP module load) - open-sse/rpc/dispatchEdges.ts:189, 199, 209 (FFI/UDS transport — HIGH RISK, used log.error not warn) - src/lib/monitoring/providerHealthAutopilot.ts:262 (quota monitor) - open-sse/services/combo.ts:2222 (fetchCodexQuota) - open-sse/services/combo/quotaStrategies.ts:431 (getRuntimeProviderProfile) - open-sse/handlers/chatCore/codexFailover.ts:19 (provider connection) - open-sse/handlers/chatCore/comboContextCache.ts:57 (proxy config) All conversions preserve existing behavior (return null/false/etc.). Only logging is added — no semantic change. Test results unchanged from baseline. TSC: 8 unchanged (pre-existing quota keystore errors, PR #505 territory). --------- 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>
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>
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
Three new check scripts in scripts/check/ + audit doc: 1. scripts/check/crypto-failures.ts: detects crypto-relevant silent catches (createHmac, createHash, scryptSync, randomBytes, jwtVerify, etc. followed by silent catch blocks). Reports findings to stderr. 2. scripts/check/console-in-src.ts: detects console.* callsites in src/lib/ + open-sse/ (excluding tests, proxyLogger.ts, consoleInterceptor.ts, intentional CLI tools). 3. scripts/check/broken-imports.ts: scans for known broken import paths. Default registry includes @/lib/featureFlags (should be @/shared/utils/featureFlags — PR #507 fixed all callers). Add new entries as drift is discovered. 4. New npm scripts: - check:crypto-failures - check:console-in-src - check:broken-imports - check:governance (combined; runs check:fail-open + check:crypto-failures + check:broken-imports) 5. docs/governance-audit-summary-2026-08.md: documents the audit methodology, ~100 silent fail-open patterns fixed across 50+ files, ~245 console.* migrated to pino, the 13 PRs shipped (#505-#527), patterns to avoid, and lessons learned. Note: GitHub Actions CI is billing-disabled for this account (per /Users/kooshapari/CodeProjects/CLAUDE.md), so these checks run locally only. Run npm run check:governance before merging any PR. Co-authored-by: KooshaPari <koosha@example.com>
KooshaPari
added a commit
that referenced
this pull request
Aug 7, 2026
…536) Three breadth governance improvements: 1. vitest.config.ts: added src/lib/resilience/__tests__/**/*.test.ts to the include list. The selfHealingManager.test.ts file was already in vitest format but was never picked up by the test runner (no glob match). Now runs as part of `npm run test`. 2. src/lib/resilience/__tests__/selfHealingManager.test.ts: fixed a latent bug in the "updateSettings returns true when threshold changes" test. It used `zScoreThreshold: 4` which is not a real config field — the test was a draft from earlier work. Changed to `criticalThreshold: 0.99` (which IS tracked by updateSettings). Before this PR, this test was unreachable (vitest didn't pick up the path). When wired up, the bug would have caused 1 of 5 tests to fail. Now all 5 pass. 3. scripts/check/quota-contract.ts (NEW): runs the QuotaStore contract test (from PR #505, the type-drift fix that prompted this whole governance pass) via vitest and propagates the exit code. Added npm script `check:quota-contract` and added to the combined `check:governance` chain. TSC: 0 errors (was 0 baseline). Co-authored-by: KooshaPari <koosha@example.com>
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
fix(quota): rewrite KeyvQuotaStore to satisfy QuotaStore interface (Phase P0 type-drift)
Background: 8 TypeScript compile errors + 1 e2e test failure traced to
half-finished refactor in commit 1951e41 (2026-07-18). The polyglot
refactor migrated
types.tstoPoolUsageSnapshotbut leftkeyvQuotaStore.tsimporting three ghost types (PoolUsage,PoolUsageWithDimensions,PlanPoolUsage) that don't exist anywhere inthe codebase. The silent try/catch in
storeFactory.ts:103-112hid thefailure at runtime (logs a pino.warn and falls back to sqlite).
This commit aligns KeyvQuotaStore with the canonical QuotaStore contract
and moves the 5 dead-code methods (zero callers per audit) to a separate
extension class so the interface impl stays clean.
Changes:
PlanPoolUsagetosrc/lib/quota/types.ts(was a ghost import).src/lib/quota/keyvQuotaStore.ts:poolUsageWithDimensions, clear) mirror
SqliteQuotaStoresemantics.poolUsagereturns empty-shellPoolUsageSnapshot(no plan metadata).poolUsageWithDimensionsbuilds full snapshot viagetPool()from@/lib/localDb+ per-keypeek()(per-key/fairShare/deficit/borrowing/burnRate computation, matching SqliteQuotaStore:167-237).
new (uri: string, options?: object) => Keyv).src/lib/quota/keyvQuotaStoreExtras.ts:KeyvQuotaStoreExtrasclass holdsrecordPlanUsage,upsertProviderPlan,listProviderPlans,setPools,getPool.getKeyvQuotaStoreExtras()+__resetKeyvQuotaStoreExtrasForTests()singleton accessors.
tests/e2e/quota-store.e2e.ts:poolUsageWithDimensionsnow requires a DB-backed pool (matches thenew behavior). Test sets up an in-memory SQLite via
getDbInstance()and creates the test pool fixture via
createPoolbefore each test.poolUsageWithDimensions returns a structured snapshotfailing).tests/unit/quota/quotaStore.contract.test.ts:SqliteQuotaStore,RedisQuotaStore,and
KeyvQuotaStoreall extendQuotaStore.T extends QuotaStore ? true : falseensuresdrift is caught at
tsctime, not at runtime.Verification (all green):
Spec: plans/quota-keystore-type-drift-spec.md (also registered as AgilePlus
feature
quota-keystore-type-drift). Closes FORGE_WRAPUP.md Tier-5task 20 ("Migrate keyvQuotaStore.ts back to a caller-compatible signature
OR update storeFactory.ts:105 call site AND write data-migration script
for the new key scheme") — this commit ships option 1 of that task.
Out of scope (deferred to follow-up):
is structurally incompatible with caller
storeFactory.ts:105andwould require a data-migration script. That direction is parked until
a Keyv-backed plan/pool storage use case materializes.
errors at runtime) is governance debt; should be fail-fast at startup.
PR-G comment in storeFactory.ts:89 — separate config-migration PR.
CI note: GitHub Actions billing limit prevents remote CI from running.
Local verification used:
node node_modules/typescript/bin/tsc --pretty false -p tsconfig.typecheck-core.json
node node_modules/.bin/vitest run tests/unit/quota tests/e2e/quota-store.e2e.ts
node --import tsx --import ./open-sse/utils/setupPolyfill.ts
--import ./tests/_setup/isolateDataDir.ts --test
tests/unit/quota-store-factory.test.ts tests/integration/quota-*.test.ts
Full vitest run surfaced 27 unrelated pre-existing flakes (parseCliRegistry,
parseOpenapi, A2A, LRUCache fase07-09, etc.) — none reference quota/keyv
code. Per AGENTS.md, these are unrelated flakes (not regressions from this
commit). Filed under separate triage.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
CodeAnt-AI Description
Align Keyv quota storage with the shared quota contract and restore structured pool usage reporting
What Changed
Impact
✅ Valid structured pool usage snapshots✅ Consistent quota reporting across storage backends✅ Compile-time detection of quota store interface drift💡 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.