feat(config): add config set command#195
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a ChangesConfig set subcommand
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (index.ts)
participant Cmd as configSetCommand
participant Store as config/store.ts
User->>CLI: commit-echo config set key value
CLI->>Cmd: configSetCommand(key, value)
Cmd->>Store: loadRawConfig()
Store-->>Cmd: raw config object
Cmd->>Cmd: validate key is allowed
Cmd->>Cmd: parse and validate value (numeric/provider/baseUrl)
alt validation fails
Cmd-->>User: print error, exit(1)
else validation succeeds
Cmd->>Store: saveConfig(updated config)
Store-->>Cmd: config.json written
Cmd-->>User: print success message
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keep template whitespace intact when using config set and avoid\npersisting environment-only overrides back into the config file.
Reject unknown provider keys and invalid base URLs in config set so users cannot persist unusable configuration through the CLI. Also normalize saved base URLs and cover the new validation paths with targeted tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/store.ts (1)
74-89: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnvalidated raw config can be silently round-tripped by
config set.
readConfigFile()only validates JSON syntax; the parsed object is returned via an uncheckedas Partial<Config>cast.loadConfig()compensates by validatinghistorySize/maxDiffSizewithreadPositiveIntegerConfigValue, butloadRawConfig()(used byconfigSetCommandinsrc/commands/config.ts:177-188) bypasses that validation entirely. If the persisted file already has a corruptedhistorySize/maxDiffSize(e.g. hand-edited to a non-numeric or negative value), runningconfig set <otherKey> <value>will silently re-persist the corrupted value viaconfig.historySize ?? DEFAULT_HISTORY_SIZEinstead of surfacing the same friendly errorloadConfig()would raise.Consider reusing the same positive-integer validation (or a lenient variant that falls back to the default instead of throwing) when reconstructing the object to save, so
config setcan't perpetuate corrupted numeric fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/store.ts` around lines 74 - 89, `readConfigFile()` only checks JSON syntax, so `loadRawConfig()` can round-trip corrupted `historySize` and `maxDiffSize` values through `configSetCommand`. Update the config reconstruction path in `src/config/store.ts` to reuse the same positive-integer validation used by `loadConfig()` and apply it before returning the object for save, so invalid numeric fields are either rejected consistently or normalized to defaults instead of being silently preserved.
🤖 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 `@src/commands/config.ts`:
- Around line 52-83: The config update path in parseConfigSetValue/saveConfig
relies on an implicit key-to-value match that TypeScript does not enforce for
the computed property assignment. Make the correlation explicit by introducing a
generic helper like updateConfigField<K extends ConfigSetKey>(config, key,
value: Config[K]) or a Record-based update flow so each key in ConfigSetKey only
accepts its matching value type. Use parseConfigSetValue and the saveConfig call
site to ensure any future mismatch between key and parsed value fails to compile
instead of being silently accepted.
In `@src/index.ts`:
- Around line 70-86: The `configCliCommand` / `config set` wiring is fine, but
`configSetCommand(key, value)` currently allows secrets like `apiKey` to be
passed directly on the command line. Update the CLI behavior around
`configCliCommand.command('set')` to either document this security risk clearly
in the command help/description or add a safer sensitive-value path (for
example, prompting or reading from stdin) for secret keys. Keep the existing
`configSetCommand` signature behavior intact unless you add a dedicated secure
input flow.
---
Outside diff comments:
In `@src/config/store.ts`:
- Around line 74-89: `readConfigFile()` only checks JSON syntax, so
`loadRawConfig()` can round-trip corrupted `historySize` and `maxDiffSize`
values through `configSetCommand`. Update the config reconstruction path in
`src/config/store.ts` to reuse the same positive-integer validation used by
`loadConfig()` and apply it before returning the object for save, so invalid
numeric fields are either rejected consistently or normalized to defaults
instead of being silently preserved.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdc5d4a4-0c57-48cf-aaa9-7fa50a72cd2a
📒 Files selected for processing (4)
src/commands/config.tssrc/config/store.tssrc/index.tstests/config-command.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
The project is ESM-only: use
.jsextensions in local imports and keepverbatimModuleSyntax-compatible TypeScript imports (for example,import typefor type-only imports).
Files:
src/index.tssrc/commands/config.tssrc/config/store.ts
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-innode:testfor test files intests/andtests/e2e/; do not use Jest or Mocha.
Write test assertions withnode:assert/strictin test files undertests/andtests/e2e/.
Files:
tests/config-command.test.mjs
src/config/store.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/config/store.ts: All configuration keys must be overridable viaCOMMIT_ECHO_*environment variables, as defined byCONFIG_ENV_VARS.
Configuration directory resolution must be OS-aware (~/.config/commit-echoon Linux,~/Library/Application Support/commit-echoon macOS,%APPDATA%/commit-echoon Windows).
Files:
src/config/store.ts
🔇 Additional comments (4)
src/index.ts (1)
11-11: LGTM!tests/config-command.test.mjs (1)
3-3: LGTM!Also applies to: 68-71, 225-353
src/config/store.ts (1)
7-8: LGTM!Also applies to: 91-97
src/commands/config.ts (1)
3-10: LGTM!Also applies to: 29-51, 153-176, 189-192
| function parseConfigSetValue(key: ConfigSetKey, rawValue: string): string | number | undefined { | ||
| if (key === 'provider') { | ||
| if (rawValue !== CUSTOM_PROVIDER_KEY && !getProviderInfo(rawValue)) { | ||
| throw new Error(`Unknown provider: ${rawValue}.`); | ||
| } | ||
| return rawValue; | ||
| } | ||
|
|
||
| if (key === 'baseUrl') { | ||
| if (!rawValue) { | ||
| return undefined; | ||
| } | ||
|
|
||
| try { | ||
| const url = new URL(rawValue); | ||
| return normalizeBaseUrl(url.toString()); | ||
| } catch { | ||
| throw new Error('baseUrl must be a valid URL.'); | ||
| } | ||
| } | ||
|
|
||
| if (!NUMERIC_CONFIG_KEYS.has(key)) { | ||
| return rawValue; | ||
| } | ||
|
|
||
| const value = Number(rawValue); | ||
| if (!Number.isInteger(value) || value <= 0) { | ||
| throw new Error(`${key} must be a positive integer.`); | ||
| } | ||
|
|
||
| return value; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Key↔value type correlation isn't enforced by the type system.
parsedValue is typed string | number | undefined, and key: ConfigSetKey is a union covering all 8 config fields. In the saveConfig({...allFieldsExplicit, [key]: parsedValue}) call, TypeScript will not flag a mismatch (e.g. a string ending up in historySize: number) because when an object literal already declares explicit properties for every member of a computed key's union, TypeScript skips checking the computed property's value type against that member (confirmed via microsoft/TypeScript#38663 and #17975).
Today this is safe only because parseConfigSetValue's branches happen to correlate key and the returned type correctly — but that invariant is invisible to the compiler and won't be caught if the branching logic is refactored incorrectly later. Consider making the correlation explicit, e.g. a small Record-keyed update or a generic helper updateConfigField<K extends ConfigSetKey>(config: Config, key: K, value: Config[K]), so a future mismatch fails to compile instead of silently persisting bad data.
Also applies to: 177-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/config.ts` around lines 52 - 83, The config update path in
parseConfigSetValue/saveConfig relies on an implicit key-to-value match that
TypeScript does not enforce for the computed property assignment. Make the
correlation explicit by introducing a generic helper like updateConfigField<K
extends ConfigSetKey>(config, key, value: Config[K]) or a Record-based update
flow so each key in ConfigSetKey only accepts its matching value type. Use
parseConfigSetValue and the saveConfig call site to ensure any future mismatch
between key and parsed value fails to compile instead of being silently
accepted.
| const configCliCommand = program | ||
| .command('config') | ||
| .description('View current configuration') | ||
| .option('--json', 'Output the configuration as JSON') | ||
| .action(async (options) => { | ||
| await configCommand({ json: Boolean(options.json) }); | ||
| }); | ||
|
|
||
| configCliCommand | ||
| .command('set') | ||
| .description('Update one configuration value') | ||
| .argument('<key>', 'Configuration key to update') | ||
| .argument('<value>', 'New value') | ||
| .action(async (key: string, value: string) => { | ||
| await configSetCommand(key, value); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial
CLI wiring for config set looks correct.
Argument order and forwarding to configSetCommand(key, value) matches its signature.
One thing worth considering: since apiKey is a valid key for config set, running commit-echo config set apiKey <secret> will expose the secret in shell history and the process list (visible via ps). Consider documenting this risk or supporting a safer input path (e.g., prompting/stdin) for sensitive keys in a future iteration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 70 - 86, The `configCliCommand` / `config set`
wiring is fine, but `configSetCommand(key, value)` currently allows secrets like
`apiKey` to be passed directly on the command line. Update the CLI behavior
around `configCliCommand.command('set')` to either document this security risk
clearly in the command help/description or add a safer sensitive-value path (for
example, prompting or reading from stdin) for secret keys. Keep the existing
`configSetCommand` signature behavior intact unless you add a dedicated secure
input flow.
Make config set preserve key-to-value type safety during updates and reuse numeric validation when reconstructing raw config data. Also warn in CLI help that passing apiKey on the command line can expose secrets through shell history or process inspection.
1 participant
Summary
Add
commit-echo config set <key> <value>so users can update one persisted config value without re-runningcommit-echo init.Changes
config setconfig set <key> <value>as a nested CLI commandTesting
npm run buildnode --test tests/config-command.test.mjsNotes:
npm test -- tests/config-command.test.mjsran the full package test glob; the new config tests passed, but one unrelated Windows E2E failed on a Git CRLF warning.npm run format:checkcould not run becauseprettieris missing from this checkout'snode_modules.Related
Closes #158
Summary by CodeRabbit
New Features
config setcommand to update individual saved settings from the CLI.Bug Fixes