settings: implement print/get/set/reset + wire the centrs.env config tier - #159
Merged
Conversation
Reviewed spec baseline for issue #135 before any code lands, so later implementation commits can be diffed against a stable, agreed spec: merges the earlier managed/unmanaged key split into one fully-validated tier, corrects config-vs-comment-kv precedence direction, adds the missing config-layer provenance prerequisite, and adds 32 numbered examples mirroring commands/devices/examples.md.
Implements the prerequisite issue #135 requires before `settings` can have anything truthful to report: `default < config < comment-kv < env < cli` was documented in the constitution and typed in SettingSourceKind, but no code ever loaded centrs.env or produced a "config" source. - New src/resolver/config-file.ts: XDG-path resolver (injectable env, mirroring defaultCdbPath's pattern), a raw file reader for settings' own always-read-the-file needs, and loadEnvFileDefaults() which every other command calls (honoring CENTRS_SKIP_ENV_FILE). - resolveStringSetting/resolveBooleanSetting/resolveOptionalIntegerSetting gain a config argument, checked between comment-kv and default. - Threaded through retrieve/execute/api/transfer/terminal and their fan-out siblings, resolver/target.ts (host/port/ssh-key), resolver/mac.ts (MAC-detection host candidate, kept in sync with resolveTarget's own ladder), resolveCdb/expandCdbSelection (CENTRS_CDB_FILE), and mcp/config.ts (CENTRS_CDB_FILE, CENTRS_MCP_ALLOW_ADHOC). - CENTRS_USERNAME/CENTRS_PASSWORD/CENTRS_CDB_PASSWORD deliberately never read the config tier, even if hand-added to the file — a stronger defense than read-time redaction alone. - Fan-out via-pinning guards (retrieve/execute/api) deliberately exclude the config tier: a centrs.env default is the weakest precedence layer and must not override per-device comment-kv `via=` across a mixed fan-out. Not yet built: the settings CLI command itself that reads/writes this file (next commit).
Builds src/settings.ts (managed-key registry for the 13 real CENTRS_* keys, refused-key list, atomic centrs.env writer, __default__ CDB probe) and src/cli/settings.ts, wired into src/cli.ts + src/index.ts. - print/get resolve env > config > default, never cli/comment-kv (no call context exists for a bare `settings` invocation). - set validates per-key, canonicalizes booleans to 1/0 on disk (CENTRS_MCP_ALLOW_ADHOC's strict "1" reader), warns (not blocks) on consequential values (insecure=true, transfer-via=ftp), never partially writes on a validation failure. - reset deletes the line rather than blanking it (a blank value would be "set to empty string" for a bash-sourced file); reset with no <attr> clears every managed line, leaving unrecognized/foreign lines and comments untouched. - Four new error/warning codes (settings/reserved-key, settings/unknown-key, settings/consequential-value, settings/skip-env-file-active) plus internal/settings-failed, each cataloged and documented per the error-pages contract; the 6 pre-existing settings/* stub pages hand-enriched to match. Bare `centrs settings` behaves like `print` regardless of TTY — interactive first-time setup is an explicitly deferred later slice.
test/integration/settings.test.ts exercises every numbered example in commands/settings/examples.md 1:1 (print/get/set/reset, --all, --skip-env-file, the __default__ CDB boundary), in-process via runCli + console capture, mirroring devices.test.ts's pattern — no CHR needed since settings does no network IO. test/unit/settings-registry.test.ts and settings-file.test.ts add function-level coverage beyond the 32 examples: exhaustive per-key validation (all 13 managed keys' valid/invalid values), attr-name normalization, refused-key read/write behavior, and atomic-write mechanics (.bak creation, line preservation, reset-deletes-not-blanks, no-trailing-newline round-tripping). Fixed a real CLI parsing bug surfaced by testing example 20: a negative-integer value like `settings set max-results -1` was misparsed as an unrecognized flag rather than a positional value. package.json: settings.test.ts joins the fixture-only fast tier (test:integration:fast) alongside devices.test.ts; adds a dedicated test:integration:settings script.
docs/MATRIX.md's settings pointer now matches devices' fixture-backed CHR-passed language. commands/settings/README.md's Status line, the config-layer/testability sections, and the error-codes list are rewritten past-tense to describe what's actually built rather than what needs to land — the interactive TTY slice remains explicitly deferred (still not built). examples.md drops its now-resolved "prerequisite not yet built" callout. docs/errors/README.md notes src/settings.ts as a second settings/* code source.
|
Important Review skippedAuto incremental reviews are disabled on this repository. 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:
📝 WalkthroughWalkthroughThis PR adds a new ChangesSettings Command Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Config-file precedence tier wiring
Estimated code review effort: 4 (Complex) | ~80 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as runSettingsCli
participant Settings as settingsSet
participant Registry as settingsManagedKeys
participant File as centrs.env
User->>CLI: centrs settings set <attr> <value>
CLI->>Settings: settingsSet({attr, value, env})
Settings->>Registry: lookupAttr / parse(value)
Registry-->>Settings: validated value or error
Settings->>File: read existing lines
Settings->>File: write CENTRS_*=value (temp+fsync+rename)
Settings-->>CLI: SettingsSuccessEnvelope
CLI-->>User: rendered text/json/yaml output
sequenceDiagram
participant Command as resolveXRequest
participant ConfigLoader as loadEnvFileDefaults
participant Resolver as resolveStringSetting
participant Target as resolveTarget/resolveAuth
Command->>ConfigLoader: loadEnvFileDefaults(env)
ConfigLoader-->>Command: config map (centrs.env)
Command->>Resolver: resolve setting(env, commentKv, config)
Resolver-->>Command: value + source (cli/env/comment-kv/config/default)
Command->>Target: resolveTarget(..., config)
Target-->>Command: resolved host/port/auth
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Implements the previously designed centrs settings command surface and wires the missing centrs.env “config” precedence tier into the shared resolver so that settings print/get reports match real resolution behavior across commands.
Changes:
- Add
centrs settings print|get|set|resetwith a managed/refused key registry, validation, and atomiccentrs.envediting. - Implement
src/resolver/config-file.tsand thread a newconfigtier through core resolution paths (retrieve/execute/api/transfer/terminal, selection/CDB, and MCP config). - Add integration + unit tests for settings behavior and update docs/error catalog to match.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/settings-registry.test.ts | Unit coverage for managed/refused registry + per-key validation behavior. |
| test/unit/settings-file.test.ts | Unit coverage for centrs.env write/reset mechanics and preservation rules. |
| test/unit/settings-config-file.test.ts | Unit coverage for config-file path resolution and parsing behavior. |
| test/unit/resolver.test.ts | Updates precedence tests to include the new config tier. |
| test/integration/settings.test.ts | Fixture-backed integration tests for the full settings CLI surface. |
| src/settings.ts | New settings command implementation: registry, read/write, renderers, envelopes. |
| src/cli/settings.ts | New CLI command wiring for centrs settings and flag/subcommand parsing. |
| src/resolver/config-file.ts | New centrs.env loader for the resolver config tier. |
| src/resolver/settings.ts | Add config tier to string/boolean/optional-integer resolver helpers. |
| src/resolver/index.ts | Export config-file tier helpers from resolver index. |
| src/retrieve.ts | Load and thread config into retrieve resolution paths. |
| src/retrieve-fanout.ts | Load config once per invocation and thread through fanout resolution. |
| src/execute.ts | Load/thread config and optionally accept preloaded config from fanout. |
| src/execute-fanout.ts | Load config once and pass through member resolution. |
| src/api.ts | Load/thread config and accept preloaded config for fanout. |
| src/api-fanout.ts | Load config once and pass through member resolution. |
| src/transfer.ts | Load/thread config into transfer resolution paths. |
| src/transfer-fanout.ts | Load config once and pass into selection expansion. |
| src/terminal.ts | Load/thread config into terminal resolution and resolve policy selection. |
| src/resolver/target.ts | Thread config into host/port/ssh-key resolution (credentials intentionally excluded). |
| src/resolver/mac.ts | Include config in host candidate resolution and MAC targeting inputs. |
| src/resolver/cdb.ts | Treat CENTRS_CDB_FILE in config as “explicit” for missing-file semantics; thread config into device settings resolution. |
| src/resolver/selection.ts | Thread config into selection expansion and explicit-CDB detection. |
| src/mcp/config.ts | Allow MCP config resolution to consume config tier (kept sync). |
| src/devices.ts | Allow devices settings resolution to read CENTRS_CDB_FILE from config tier. |
| src/cli/mcp.ts | Load config tier for MCP CLI before resolving MCP config. |
| src/cli.ts | Wire settings command into top-level CLI routing and help listing. |
| src/index.ts | Export settings APIs/types from public entrypoint. |
| src/core/error-catalog.ts | Register new settings/internal error codes. |
| README.md | Update settings precedence description to name centrs.env user config. |
| commands/settings/README.md | Promote settings command spec to CHR-passed and document final surface/behavior. |
| commands/settings/examples.md | Add executable examples for settings command behavior. |
| docs/MATRIX.md | Mark settings row CHR-passed with evidence pointer. |
| docs/errors/README.md | Expand settings/* family description to include settings command rules. |
| docs/errors/internal/settings-failed.md | New error page for internal settings failure. |
| docs/errors/settings/unknown-key.md | New error page for unknown settings key. |
| docs/errors/settings/reserved-key.md | New error page for refused/reserved keys. |
| docs/errors/settings/consequential-value.md | New warning page for consequential-but-valid values. |
| docs/errors/settings/skip-env-file-active.md | New warning page for print under skip-env-file. |
| docs/errors/settings/invalid-format.md | Enrich existing format error page. |
| docs/errors/settings/invalid-boolean.md | Enrich existing boolean error page. |
| docs/errors/settings/invalid-integer.md | Enrich existing integer error page. |
| docs/errors/settings/invalid-timeout.md | Enrich existing timeout error page. |
| docs/errors/settings/invalid-via.md | Enrich existing via error page. |
| commands/devices/README.md | Align precedence prose with the centrs.env config tier wording. |
| package.json | Add integration test scripts for settings and include it in fast integration set. |
Comment on lines
+453
to
+465
| const handle = await open(tempPath, "w"); | ||
| try { | ||
| await handle.write(content); | ||
| await handle.sync(); | ||
| } finally { | ||
| await handle.close(); | ||
| } | ||
| try { | ||
| await rename(tempPath, path); | ||
| } catch (error) { | ||
| await unlink(tempPath).catch(() => undefined); | ||
| throw error; | ||
| } |
Comment on lines
+574
to
+577
| const knownKeys = new Set<string>([ | ||
| ...settingsManagedKeys.map((def) => def.envKey), | ||
| ...settingsRefusedKeys.map((def) => def.envKey), | ||
| ]); |
Comment on lines
+116
to
+125
| export async function loadEnvFileDefaults( | ||
| env: Record<string, string | undefined> = Bun.env, | ||
| ): Promise<Record<string, string>> { | ||
| if (isSkippingEnvFile(env)) { | ||
| return {}; | ||
| } | ||
| const path = defaultSettingsPath(env); | ||
| const { lines } = await readSettingsFileRaw(path); | ||
| return parseEnvFileDefaults(lines); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/resolver/target.ts (1)
82-98: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass
configthroughresolveTargetandresolveAuthinsrc/retrieve.ts
buildResolvedRetrievestill calls both helpers without the new config argument, socentrs retrievebypassescentrs.envvalues for host/port/ssh-key while the other command paths use them.🤖 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/resolver/target.ts` around lines 82 - 98, Pass the new config object through the retrieve flow so `buildResolvedRetrieve` does not bypass `centrs.env` values. Update the `buildResolvedRetrieve` call sites to supply `config` into both `resolveTarget` and `resolveAuth`, matching the existing `resolveTarget` signature and the `resolveAuth` helper in `src/retrieve.ts`. Ensure host, port, and ssh-key resolution use the same config-backed precedence as the other command paths.src/retrieve.ts (1)
650-665: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThread
configintoresolveTargetandresolveAuth.src/retrieve.ts:650-665still drops the config tier here, soCENTRS_HOST/CENTRS_PORTfromcentrs.envwon’t apply during target resolution, andCENTRS_SSH_KEYwon’t apply for SSH auth.🤖 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/retrieve.ts` around lines 650 - 665, The target/auth resolution path in retrieve.ts is missing the config tier, so CENTRS_HOST, CENTRS_PORT, and CENTRS_SSH_KEY from centrs.env are not considered. Update the calls to resolveTarget and resolveAuth to thread the config object through alongside env, via.value, and cdbResolution, and make sure the resolver functions use that config tier when choosing target host/port and SSH auth material.
🧹 Nitpick comments (5)
commands/settings/README.md (1)
222-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove shared precedence/error docs out of the command README.
This section duplicates the repo-level constitution and error-catalog contract. Keeping it here will drift from the canonical source and makes
commands/settings/README.mdharder to keep command-focused.As per coding guidelines,
commands/*/README.md: "Each commands// directory must contain a README.md file that documents intent, flags, and behavior (the 'designed' tier), and should not restate the constitution or link elsewhere for envelope, errors, settings precedence, target selection, and protocol selection documentation".🤖 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 `@commands/settings/README.md` around lines 222 - 333, Remove the duplicated precedence and error-catalog contract content from commands/settings/README.md and keep this README focused on settings-specific intent, flags, and behavior. Move or delete the sections that restate the shared constitution, config precedence, source provenance, and error code listings, and leave only command-local guidance tied to symbols like settings print/get/set/reset and defaultSettingsPath/config-file resolution.Source: Coding guidelines
src/cli/settings.ts (1)
40-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--all/--cdb-file/--cdb-password/--skip-env-filearen't validated against subcommand.These flags are documented as "
parseSettingsCliArgsaccepts them unconditionally for any subcommand (e.g.centrs settings set foo bar --allsilently ignores--allinstead of erroring). Minor UX polish; not currently causing incorrect behavior sincesettingsGet/Set/Resetsimply don't read these fields.Also applies to: 86-140
🤖 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/cli/settings.ts` around lines 40 - 69, The `settings` CLI options for `--all`, `--cdb-file`, `--cdb-password`, and `--skip-env-file` are being accepted by `parseSettingsCliArgs` for every subcommand even though they are intended for `print` only. Update the subcommand validation in `parseSettingsCliArgs` (and any related settings command wiring in `settingsGet`, `settingsSet`, and `settingsReset`) so these flags are rejected with an error when used with non-`print` subcommands, while still remaining valid for `print`.src/settings.ts (2)
648-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
print <attr>omits comment-kv tips thatgetprovides.
settingsGetcallsviaCommentKvTip(lookup.def)(Line 783) to surface the "a device's own comment-kv override may take precedence" tip for keys likevia/timeout/port. The single-attr branch ofsettingsPrinthardcodestips: [], so the same information is silently missing when a user runscentrs settings print viainstead ofcentrs settings get via.♻️ Suggested fix
const data: SettingsPrintData = {}; + const tips: Tip[] = []; if (lookup.kind === "managed") { data[lookup.def.attr] = resolvePrintEntry(lookup.def, args.env, config); + tips.push(...viaCommentKvTip(lookup.def)); } else { data[lookup.def.attr] = resolveRefusedEntry(lookup.def, args.env, config); } return { ok: true, data, warnings, - tips: [], + tips, meta: settingsMeta("print", settingsFile), };🤖 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/settings.ts` around lines 648 - 666, The single-attr branch in settingsPrint is dropping the same comment-kv guidance that settingsGet already exposes. Update the args.attr path in settingsPrint to reuse viaCommentKvTip(lookup.def) when building the response, so keys like via, timeout, and port include the comment-kv precedence tip instead of always returning tips: [].
439-466: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAtomic-write helper looks solid; note the read-modify-write race across concurrent invocations.
writeSettingsFileLinesitself is atomic (temp file + fsync + rename), butsettingsSet/settingsResetread the full file, mutate in memory, then write back — two concurrentcentrs settings setinvocations can still lose one writer's update (last-writer-wins on the whole file, not per-key). Acceptable for a single-user local CLI; flagging for awareness rather than as a blocker.🤖 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/settings.ts` around lines 439 - 466, `writeSettingsFileLines` is atomic, but `settingsSet`/`settingsReset` still have a read-modify-write race that can drop concurrent updates. Fix this by serializing settings mutations or adding a file-level lock around the full read/update/write flow so only one writer can modify the settings file at a time. Use the existing `settingsSet`, `settingsReset`, and `writeSettingsFileLines` paths as the place to coordinate the protection and prevent last-writer-wins overwrites.src/retrieve.ts (1)
754-786: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
resolveRetrieveRequestcan't accept a pre-loaded config, causing redundant reloads in fanout.Unlike
resolveExecuteRequest/resolveApiRequest, which acceptoptions.configso fan-out can supply an already-loaded config,resolveRetrieveRequestalways callsloadEnvFileDefaults(env)itself. This forcesretrieve-fanout.ts's ad-hoc-member path to re-read/parsecentrs.envonce per ad-hoc target instead of reusing the fanout's already-computedconfig.♻️ Proposed fix
export async function resolveRetrieveRequest( request: RetrieveRequest, env: Record<string, string | undefined>, + options: { config?: Record<string, string | undefined> } = {}, ): Promise<ResolvedRetrieveRequest> { const attributeSelections = validateRetrieveRequestShape(request); - const config = await loadEnvFileDefaults(env); + const config = options.config ?? (await loadEnvFileDefaults(env));And in
retrieve-fanout.ts's ad-hoc path:return resolveRetrieveRequest( { ...request, targetInput: member.input }, env, + { config }, );🤖 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/retrieve.ts` around lines 754 - 786, resolveRetrieveRequest currently always calls loadEnvFileDefaults(env), which prevents fanout from reusing an already-loaded config and causes repeated centrs.env reloads. Update resolveRetrieveRequest to accept an optional preloaded config in the same style as resolveExecuteRequest and resolveApiRequest, then use that config when provided and only fall back to loadEnvFileDefaults(env) otherwise. Also update the retrieve-fanout.ts ad-hoc member path to pass its existing config into resolveRetrieveRequest so the fanout path avoids redundant parsing.
🤖 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 `@docs/errors/internal/settings-failed.md`:
- Around line 7-9: The `internal/settings-failed` markdown still contains
placeholder text instead of real recovery guidance. Replace the stub in the
`docs/errors/internal/settings-failed.md` entry with concrete trigger and
remediation details that explain when this error is emitted and what the user
should do next, keeping the wording consistent with the error contract
referenced from `docs/CONSTITUTION.md`.
In `@docs/errors/settings/invalid-timeout.md`:
- Around line 12-14: The timeout wording in this doc is too broad because
parseDuration only accepts unsigned inputs, so this branch can reject 0 but not
a negative duration. Update the text in invalid-timeout.md to say that
settings.ts rejects a syntactically valid duration that parses to 0, and remove
the mention of negative numbers so the description matches parseDuration and the
timeout validation rule precisely.
In `@docs/errors/settings/unknown-key.md`:
- Line 3: The opening summary for the unknown-key settings error only mentions
settings get and set, so update the short description to also include reset.
Keep the detailed explanation unchanged, and make sure the summary sentence in
the unknown-key error doc matches the full set of supported commands by
referencing the existing settings get/set/reset wording.
In `@src/settings.ts`:
- Around line 807-826: The settingsSet flow is re-parsing the existing on-disk
value with def.parse(previousRaw, def.envKey), which throws before a valid
replacement can be written when the stored value is malformed. Update
settingsSet to tolerate an invalid previous entry by avoiding strict parse of
previousRaw and instead only use the raw existing line/value as the previous
comparison target, similar to the tolerant
resolvePrintEntry/buildPrintEntryTolerant path. Keep the main parsed new value
handling unchanged, and ensure the write path still replaces the old entry even
if the prior value is bad.
---
Outside diff comments:
In `@src/resolver/target.ts`:
- Around line 82-98: Pass the new config object through the retrieve flow so
`buildResolvedRetrieve` does not bypass `centrs.env` values. Update the
`buildResolvedRetrieve` call sites to supply `config` into both `resolveTarget`
and `resolveAuth`, matching the existing `resolveTarget` signature and the
`resolveAuth` helper in `src/retrieve.ts`. Ensure host, port, and ssh-key
resolution use the same config-backed precedence as the other command paths.
In `@src/retrieve.ts`:
- Around line 650-665: The target/auth resolution path in retrieve.ts is missing
the config tier, so CENTRS_HOST, CENTRS_PORT, and CENTRS_SSH_KEY from centrs.env
are not considered. Update the calls to resolveTarget and resolveAuth to thread
the config object through alongside env, via.value, and cdbResolution, and make
sure the resolver functions use that config tier when choosing target host/port
and SSH auth material.
---
Nitpick comments:
In `@commands/settings/README.md`:
- Around line 222-333: Remove the duplicated precedence and error-catalog
contract content from commands/settings/README.md and keep this README focused
on settings-specific intent, flags, and behavior. Move or delete the sections
that restate the shared constitution, config precedence, source provenance, and
error code listings, and leave only command-local guidance tied to symbols like
settings print/get/set/reset and defaultSettingsPath/config-file resolution.
In `@src/cli/settings.ts`:
- Around line 40-69: The `settings` CLI options for `--all`, `--cdb-file`,
`--cdb-password`, and `--skip-env-file` are being accepted by
`parseSettingsCliArgs` for every subcommand even though they are intended for
`print` only. Update the subcommand validation in `parseSettingsCliArgs` (and
any related settings command wiring in `settingsGet`, `settingsSet`, and
`settingsReset`) so these flags are rejected with an error when used with
non-`print` subcommands, while still remaining valid for `print`.
In `@src/retrieve.ts`:
- Around line 754-786: resolveRetrieveRequest currently always calls
loadEnvFileDefaults(env), which prevents fanout from reusing an already-loaded
config and causes repeated centrs.env reloads. Update resolveRetrieveRequest to
accept an optional preloaded config in the same style as resolveExecuteRequest
and resolveApiRequest, then use that config when provided and only fall back to
loadEnvFileDefaults(env) otherwise. Also update the retrieve-fanout.ts ad-hoc
member path to pass its existing config into resolveRetrieveRequest so the
fanout path avoids redundant parsing.
In `@src/settings.ts`:
- Around line 648-666: The single-attr branch in settingsPrint is dropping the
same comment-kv guidance that settingsGet already exposes. Update the args.attr
path in settingsPrint to reuse viaCommentKvTip(lookup.def) when building the
response, so keys like via, timeout, and port include the comment-kv precedence
tip instead of always returning tips: [].
- Around line 439-466: `writeSettingsFileLines` is atomic, but
`settingsSet`/`settingsReset` still have a read-modify-write race that can drop
concurrent updates. Fix this by serializing settings mutations or adding a
file-level lock around the full read/update/write flow so only one writer can
modify the settings file at a time. Use the existing `settingsSet`,
`settingsReset`, and `writeSettingsFileLines` paths as the place to coordinate
the protection and prevent last-writer-wins overwrites.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13079b0b-5c38-4a31-8446-c2ee01d807f4
📒 Files selected for processing (46)
README.mdcommands/devices/README.mdcommands/settings/README.mdcommands/settings/examples.mddocs/MATRIX.mddocs/errors/README.mddocs/errors/internal/settings-failed.mddocs/errors/settings/consequential-value.mddocs/errors/settings/invalid-boolean.mddocs/errors/settings/invalid-format.mddocs/errors/settings/invalid-integer.mddocs/errors/settings/invalid-timeout.mddocs/errors/settings/invalid-via.mddocs/errors/settings/reserved-key.mddocs/errors/settings/skip-env-file-active.mddocs/errors/settings/unknown-key.mdpackage.jsonsrc/api-fanout.tssrc/api.tssrc/cli.tssrc/cli/mcp.tssrc/cli/settings.tssrc/core/error-catalog.tssrc/devices.tssrc/execute-fanout.tssrc/execute.tssrc/index.tssrc/mcp/config.tssrc/resolver/cdb.tssrc/resolver/config-file.tssrc/resolver/index.tssrc/resolver/mac.tssrc/resolver/selection.tssrc/resolver/settings.tssrc/resolver/target.tssrc/retrieve-fanout.tssrc/retrieve.tssrc/settings.tssrc/terminal.tssrc/transfer-fanout.tssrc/transfer.tstest/integration/settings.test.tstest/unit/resolver.test.tstest/unit/settings-config-file.test.tstest/unit/settings-file.test.tstest/unit/settings-registry.test.ts
Comment on lines
+7
to
+9
| See [`docs/CONSTITUTION.md`](../../CONSTITUTION.md) for the centrs error | ||
| contract. This stub will be expanded with the typical trigger and remediation | ||
| for `internal/settings-failed`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the stub with actual recovery guidance.
This still says it "will be expanded" instead of explaining when internal/settings-failed is emitted and what the user should do next. A shipped error page should include a concrete trigger and remediation path.
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Ensure spelling is correct
Context: ...ION.md`](../../CONSTITUTION.md) for the centrs error contract. This stub will be expan...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/errors/internal/settings-failed.md` around lines 7 - 9, The
`internal/settings-failed` markdown still contains placeholder text instead of
real recovery guidance. Replace the stub in the
`docs/errors/internal/settings-failed.md` entry with concrete trigger and
remediation details that explain when this error is emitted and what the user
should do next, keeping the wording consistent with the error contract
referenced from `docs/CONSTITUTION.md`.
Comment on lines
+12
to
+14
| `settings.ts` layers one more rule on top for `timeout` specifically: even a | ||
| syntactically valid duration that parses to `0` or a negative number is | ||
| rejected — a zero or negative timeout isn't a meaningful value to set. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the timeout wording.
parseDuration only accepts unsigned inputs, so this branch can reject 0 but not a negative duration. As written, the doc implies negatives are reachable here, which is misleading.
Fix
-`settings.ts` layers one more rule on top for `timeout` specifically: even a
-syntactically valid duration that parses to `0` or a negative number is
-rejected — a zero or negative timeout isn't a meaningful value to set.
+`settings.ts` layers one more rule on top for `timeout` specifically: even a
+syntactically valid duration that parses to `0` is rejected — a zero timeout
+isn't a meaningful value to set.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `settings.ts` layers one more rule on top for `timeout` specifically: even a | |
| syntactically valid duration that parses to `0` or a negative number is | |
| rejected — a zero or negative timeout isn't a meaningful value to set. | |
| `settings.ts` layers one more rule on top for `timeout` specifically: even a | |
| syntactically valid duration that parses to `0` is rejected — a zero timeout | |
| isn't a meaningful value to set. |
🤖 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 `@docs/errors/settings/invalid-timeout.md` around lines 12 - 14, The timeout
wording in this doc is too broad because parseDuration only accepts unsigned
inputs, so this branch can reject 0 but not a negative duration. Update the text
in invalid-timeout.md to say that settings.ts rejects a syntactically valid
duration that parses to 0, and remove the mention of negative numbers so the
description matches parseDuration and the timeout validation rule precisely.
| @@ -0,0 +1,25 @@ | |||
| # `settings/unknown-key` | |||
|
|
|||
| `settings get`/`set` received a token that is not a recognized CENTRS_*-shaped setting. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include reset in the one-line summary.
The detailed section already covers reset, but the opening sentence only names get/set. That makes the short description incomplete for readers scanning the error list.
Fix
-`settings get`/`set` received a token that is not a recognized CENTRS_*-shaped setting.
+`settings get`/`set`/`reset` received a token that is not a recognized CENTRS_*-shaped setting.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `settings get`/`set` received a token that is not a recognized CENTRS_*-shaped setting. | |
| `settings get`/`set`/`reset` received a token that is not a recognized CENTRS_*-shaped setting. |
🤖 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 `@docs/errors/settings/unknown-key.md` at line 3, The opening summary for the
unknown-key settings error only mentions settings get and set, so update the
short description to also include reset. Keep the detailed explanation
unchanged, and make sure the summary sentence in the unknown-key error doc
matches the full set of supported commands by referencing the existing settings
get/set/reset wording.
Comment on lines
+807
to
+826
| export async function settingsSet( | ||
| args: SettingsSetArgs, | ||
| ): Promise<SettingsSuccessEnvelope<SettingsSetData>> { | ||
| const settingsFile = defaultSettingsPath(args.env); | ||
| const lookup = lookupAttr(args.attr); | ||
| if (lookup.kind === "unknown") { | ||
| throw unknownKeyError(args.attr); | ||
| } | ||
| if (lookup.kind === "refused") { | ||
| throw reservedKeyError(lookup.def); | ||
| } | ||
| const def = lookup.def; | ||
| const parsed = def.parse(args.value, def.envKey); | ||
|
|
||
| const lines = await readRawLines(settingsFile); | ||
| const index = findLineIndex(lines, def.envKey); | ||
| const previousRaw = index >= 0 ? rawValueAtLine(lines, index) : undefined; | ||
| const previous = | ||
| previousRaw !== undefined ? def.parse(previousRaw, def.envKey).value : null; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
settings set can't fix an already-malformed value.
previous is computed by strictly re-parsing the existing raw on-disk value via def.parse(previousRaw, def.envKey). If the file was hand-edited into an invalid state (e.g. CENTRS_PORT=abc), this throws before the new (valid) value is ever written — so centrs settings set port 8080 fails with settings/invalid-integer citing the old garbage value, even though the new value is fine. This defeats the exact recovery path the tolerant resolvePrintEntry/buildPrintEntryTolerant design (Lines 521-569) was built for.
🐛 Proposed fix: tolerate a malformed previous value
- const previous =
- previousRaw !== undefined ? def.parse(previousRaw, def.envKey).value : null;
+ const previous = (() => {
+ if (previousRaw === undefined) {
+ return null;
+ }
+ try {
+ return def.parse(previousRaw, def.envKey).value;
+ } catch {
+ // Malformed value in a hand-edited file: report it as-is so `set`
+ // can still overwrite it (mirrors buildPrintEntryTolerant).
+ return previousRaw;
+ }
+ })();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function settingsSet( | |
| args: SettingsSetArgs, | |
| ): Promise<SettingsSuccessEnvelope<SettingsSetData>> { | |
| const settingsFile = defaultSettingsPath(args.env); | |
| const lookup = lookupAttr(args.attr); | |
| if (lookup.kind === "unknown") { | |
| throw unknownKeyError(args.attr); | |
| } | |
| if (lookup.kind === "refused") { | |
| throw reservedKeyError(lookup.def); | |
| } | |
| const def = lookup.def; | |
| const parsed = def.parse(args.value, def.envKey); | |
| const lines = await readRawLines(settingsFile); | |
| const index = findLineIndex(lines, def.envKey); | |
| const previousRaw = index >= 0 ? rawValueAtLine(lines, index) : undefined; | |
| const previous = | |
| previousRaw !== undefined ? def.parse(previousRaw, def.envKey).value : null; | |
| export async function settingsSet( | |
| args: SettingsSetArgs, | |
| ): Promise<SettingsSuccessEnvelope<SettingsSetData>> { | |
| const settingsFile = defaultSettingsPath(args.env); | |
| const lookup = lookupAttr(args.attr); | |
| if (lookup.kind === "unknown") { | |
| throw unknownKeyError(args.attr); | |
| } | |
| if (lookup.kind === "refused") { | |
| throw reservedKeyError(lookup.def); | |
| } | |
| const def = lookup.def; | |
| const parsed = def.parse(args.value, def.envKey); | |
| const lines = await readRawLines(settingsFile); | |
| const index = findLineIndex(lines, def.envKey); | |
| const previousRaw = index >= 0 ? rawValueAtLine(lines, index) : undefined; | |
| const previous = (() => { | |
| if (previousRaw === undefined) { | |
| return null; | |
| } | |
| try { | |
| return def.parse(previousRaw, def.envKey).value; | |
| } catch { | |
| // Malformed value in a hand-edited file: report it as-is so `set` | |
| // can still overwrite it (mirrors buildPrintEntryTolerant). | |
| return previousRaw; | |
| } | |
| })(); |
🤖 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/settings.ts` around lines 807 - 826, The settingsSet flow is re-parsing
the existing on-disk value with def.parse(previousRaw, def.envKey), which throws
before a valid replacement can be written when the stored value is malformed.
Update settingsSet to tolerate an invalid previous entry by avoiding strict
parse of previousRaw and instead only use the raw existing line/value as the
previous comparison target, similar to the tolerant
resolvePrintEntry/buildPrintEntryTolerant path. Keep the main parsed new value
handling unchanged, and ensure the write path still replaces the old entry even
if the prior value is bad.
Copilot: - writeSettingsFileLines: fsync the directory after rename, matching winbox-cdb-write.ts's durability pattern. - print --all: refused/credential keys hand-added to centrs.env now surface in the unrecognized list (redacted if secret) instead of being silently treated as "known" and hidden. - loadEnvFileDefaults: strip credential/self-referential keys from the config map itself (REFUSED_CONFIG_ENV_KEYS, resolver/settings.ts), so no call site can accidentally read one back out of `config`, even if a resolver's own guard is ever missed. parseEnvFileDefaults stays unfiltered so `settings` can still inspect/redact hand-added lines. CodeRabbit: - settingsSet no longer throws when the *previous* on-disk value is already malformed (e.g. a hand-edited CENTRS_PORT=abc) -- it reports the raw value tolerantly and still writes the new, valid one. Previously `set` couldn't repair a broken file. - Fixed invalid-timeout.md's wording: parseDuration only accepts unsigned durations, so negative values were never reachable. - unknown-key.md's summary now mentions `reset`, not just get/set. Left internal/settings-failed.md as the generic stub: it deliberately mirrors internal/devices-failed.md's identical catch-all shape, and enriching only one would break that parity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Pushed two follow-up commits addressing review feedback: Copilot
CodeRabbit
Left CodeQL ( All new/updated tests pass ( |
Root-caused both new js/insecure-temporary-file findings (src/devices.ts:215,
src/transfer.ts:1205) by downloading the PR's SARIF and reading codeFlows: the
sole taint source across every path is the literal "/tmp/does-not-exist.cdb"
in test/integration/settings.test.ts (example 32), which CodeQL's OSTempDir
source model treats as an os-temp-dir source purely because it matches
/tmp/% -- no real tmpdir() call is involved. The sink model (InsecureFileOpen)
fires whenever the path argument has no secure mode (no group/other bits),
regardless of wx/O_EXCL collision-safety.
Two real fixes, not just noise-chasing:
- test/integration/settings.test.ts: build the nonexistent-cdb path under the
test's own sandbox dir (freshDir()) instead of a hardcoded "/tmp/..."
literal, removing the false taint source and improving test portability.
- src/devices.ts createEmptyCdbNoClobber: pass 0o600 to the exclusive-create
open() so a new CDB (device credentials) is owner-only instead of
inheriting the process umask.
- src/transfer.ts writeLocalSink: pass { mode: 0o600 } to writeFileSync so a
newly created download destination (may hold RouterOS backups/configs) is
owner-only. Mode only applies on creation, so existing destinations are
unaffected.
Also dismissed alerts #88-90 (js/clear-text-logging, src/cli/settings.ts) as
false positives after tracing their codeFlows against
CleartextLoggingCustomizations.qll: #88's source is passwordSet, a boolean
matched purely by a property-name heuristic (not value/type aware). #89/#90's
apparent taint is context-insensitive over-tainting of the shared
expectValue()/CentrsError argv-parsing plumbing reused by every CLI
subcommand -- the actual sink content is a hardcoded literal error string
("Missing value for --password."), never a real secret. Written
justifications are attached to the dismissals via the code-scanning API.
Documented both false-positive shapes, how to fetch a PR's alert-diff state
and SARIF codeFlows, and the "no inline suppression, dismiss with reason or
query-filter" reality in
.github/instructions/github-security-quality.instructions.md so future PRs
can self-diagnose instead of re-litigating this each time.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
CodeQL alert-diff: root-caused and resolvedDownloaded the PR's SARIF (
|
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.
Summary
Closes #135.
configprecedence tier (default < config < comment-kv < env < cli) into the shared resolver —src/resolver/config-file.tsloadscentrs.env, andresolveStringSetting/resolveBooleanSetting/resolveOptionalIntegerSettinggain aconfigargument threaded throughretrieve/execute/api/transfer/terminaland their fan-outs,resolveTarget/resolveAuth,resolveCdb/expandCdbSelection, andmcp/config.ts. This was the real prerequisite the issue's own follow-up comment called out — without it,settings printwould have had nothing truthful to report.centrs settings print/get/set/reset(src/settings.ts+src/cli/settings.ts): a 13-key managed registry with full validation, a 5-key refused list (credential-shaped/self-referential keys), an atomiccentrs.envwriter that preserves hand-edited comments/foreign lines byte-for-byte, boolean canonicalization to1/0on disk, and a read-only__default__CDB probe onprint.settings/reserved-key,settings/unknown-key,settings/consequential-value,settings/skip-env-file-active) plusinternal/settings-failed, each cataloged and documented per the error-pages contract; the 6 pre-existingsettings/*stub pages hand-enriched.docs/MATRIX.mdandcommands/settings/README.mdflipped toCHR-passed.Deliberately out of scope (see "New issue" below): the interactive TTY first-time-setup flow. Bare
centrs settingsbehaves likeprintregardless of TTY for now.Test plan
bun run lint && bun run test && bun run build— 1042 unit tests, 0 failbun run test:integrationagainst real CHR 7.23.1 — 121 tests, 0 fail across all 25 integration files (confirms the resolver threading didn't regress any transport-backed command)test/integration/settings.test.ts— all 32 examples incommands/settings/examples.md, 1:1test/unit/settings-registry.test.ts/settings-file.test.ts— exhaustive per-key validation + atomic-write mechanicscentrs settings set format jsonchanges a realretrieve()resolution's output, not justsettings print's own report🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
settingscommand for viewing, updating, and resetting app settings.Bug Fixes
Documentation