refactor(cli): route config command through daemon IPC#30262
Conversation
The `config` CLI was the last general-purpose command still tagged
`transport: "local"`, reading and writing `config.json` directly from the
CLI process. That bypassed the daemon's in-memory config cache, file
watcher, embedding cache invalidation, and provider re-init - so every
`assistant config set ...` left the running daemon stale until a restart
or unrelated PATCH happened to flush the cache.
This change wires every subcommand through the existing daemon IPC
surface so the daemon stays coherent on every write and is the single
source of truth on every read.
What changed
- `cli/commands/config.ts`: re-tagged `transport: "ipc"`, dropped direct
`loadRawConfig` / `saveRawConfig` / `AssistantConfigSchema` /
`getSchemaAtPath` imports.
- `set`: builds a single-key deep-merge patch from the dotted path and
sends it to `config_patch`. Daemon does the merge + cache/provider
reinit (already implemented in `handlePatchConfig`).
- `get` / `list`: fetch the full raw config via `config_get` and walk
the dotted path in the CLI process. Pure read.
- `schema`: calls the new `config_schema_get` IPC route (see below).
- `validate-allowlist`: kept as-is - uses a dynamic `require()` to
load `security/secret-allowlist.js`, which has no daemon-globals
and only reads `secret-allowlist.json` from disk. Marked with a
TODO to add a `config_allowlist_validate` daemon route later so
the subcommand can become a pure thin wrapper too.
- `cli/lib/nested-value.ts` (new): thin re-export of
`getNestedValue`/`setNestedValue` from `config/loader.js`. These two
helpers are pure and dependency-free, but `cli/no-daemon-internals`
forbids `ipc` commands from importing `config/loader.js` directly
(it pulls in daemon-only globals via other exports). Helper modules
with no `registerCommand` call are exempt from the rule, so this is
the canonical bridge.
- `runtime/routes/conversation-query-routes.ts`: new
`handleGetConfigSchema` handler + `config_schema_get` route
(`GET config/schema?path=...`). Pure read; calls
`z.toJSONSchema(AssistantConfigSchema)` or `getSchemaAtPath` for the
scoped case, returns `{ schema }`. No daemon state touched.
- `runtime/auth/route-policy.ts`: new actor-endpoint entry
`config/schema:GET → settings.read`.
After this change the only commands still tagged non-`ipc` are:
- `transport: "local"`: `keys` (hybrid - uses daemon-credential-client
side-channel), `completions` (shell scripts), `credential-execution`
(CES RPC, different transport)
- `transport: "bootstrap"`: `default-action` (interactive entry, starts
the daemon)
`autonomy` is being removed in #30261.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f45a0f68f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const patch: Record<string, unknown> = {}; | ||
| setNestedValue(patch, key, parsed); | ||
|
|
||
| const result = await cliIpcCall("config_patch", { body: patch }); |
There was a problem hiding this comment.
config set replacement semantics for object/null
Routing config set through config_patch changes behavior for JSON object and null values because the daemon patch path uses deepMergeOverwrite (in config/loader.ts) rather than direct assignment. For example, assistant config set llm {} now leaves existing llm.* keys intact (merge) instead of replacing llm, and assistant config set some.newKey null can become a no-op when the key is absent. This is a regression from the previous setNestedValue(raw, key, parsed) behavior and makes the command unable to reliably set exact values at a dotted path.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 validate-allowlist subcommand uses local file I/O under ipc-tagged command
The validate-allowlist subcommand at assistant/src/cli/commands/config.ts:273-299 uses a dynamic require("../../security/secret-allowlist.js") to read files directly from disk, while the parent config command is now tagged as transport: "ipc". The AGENTS.md rule says ipc-tagged commands should "never import daemon-internal modules." The dynamic require likely evades the cli/no-daemon-internals ESLint rule at lint time, so this won't cause a build failure. At runtime the transport tag is inert (it's just metadata), so the subcommand still works. However, this creates a semantic mismatch: the other subcommands need a running assistant (IPC), but validate-allowlist works offline. The TODO comment in the help text acknowledges this gap. Consider either (a) splitting validate-allowlist into a separate local-tagged command, or (b) adding the planned IPC route.
(Refers to lines 273-299)
Was this helpful? React with 👍 or 👎 to provide feedback.
| Configuration is managed by the assistant daemon. The CLI sends every | ||
| read/write through the daemon's IPC channel so the in-memory cache, |
There was a problem hiding this comment.
🔴 User-facing help text uses "daemon" instead of "assistant" (top-level config)
The AGENTS.md rule under User-Facing Terminology states: "In all user-facing text — CLI output, error messages, help strings… use 'assistant' instead of 'daemon'." The top-level config help text (shown via --help) says "assistant daemon" and "daemon's IPC channel" on lines 60–61, violating this mandatory rule.
| Configuration is managed by the assistant daemon. The CLI sends every | |
| read/write through the daemon's IPC channel so the in-memory cache, | |
| Configuration is managed by the assistant. The CLI sends every | |
| read/write through an IPC channel so the in-memory cache, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| The CLI sends the change to the daemon, which deep-merges it into | ||
| config.json, invalidates caches, and reinitializes providers so the new | ||
| value takes effect immediately. |
There was a problem hiding this comment.
🔴 User-facing help text uses "daemon" instead of "assistant" (set subcommand)
The config set subcommand's addHelpText block references "the daemon" on line 95. Per AGENTS.md, user-facing help strings must use "assistant" instead of "daemon".
| The CLI sends the change to the daemon, which deep-merges it into | |
| config.json, invalidates caches, and reinitializes providers so the new | |
| value takes effect immediately. | |
| The CLI sends the change to the assistant, which deep-merges it into | |
| config.json, invalidates caches, and reinitializes providers so the new | |
| value takes effect immediately. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| Fetches the full config from the daemon and prints the value at the | ||
| given key path. If the key is not set, prints "(not set)". Object | ||
| values are pretty-printed as indented JSON. |
There was a problem hiding this comment.
🔴 User-facing help text uses "daemon" instead of "assistant" (get subcommand)
The config get subcommand's addHelpText block references "the daemon" on line 144. Per AGENTS.md, user-facing help strings must use "assistant" instead of "daemon".
| Fetches the full config from the daemon and prints the value at the | |
| given key path. If the key is not set, prints "(not set)". Object | |
| values are pretty-printed as indented JSON. | |
| Fetches the full config from the assistant and prints the value at the | |
| given key path. If the key is not set, prints "(not set)". Object | |
| values are pretty-printed as indented JSON. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| Asks the daemon for the JSON Schema of the entire config object, or | ||
| the sub-schema at the given path. Useful for understanding available | ||
| fields, their types, defaults, and constraints. |
There was a problem hiding this comment.
🔴 User-facing help text uses "daemon" instead of "assistant" (schema subcommand)
The config schema subcommand's addHelpText block references "the daemon" on line 178. Per AGENTS.md, user-facing help strings must use "assistant" instead of "daemon".
| Asks the daemon for the JSON Schema of the entire config object, or | |
| the sub-schema at the given path. Useful for understanding available | |
| fields, their types, defaults, and constraints. | |
| Asks the assistant for the JSON Schema of the entire config object, or | |
| the sub-schema at the given path. Useful for understanding available | |
| fields, their types, defaults, and constraints. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| Fetches the full raw configuration from the daemon and prints it as | ||
| pretty-printed JSON. If no configuration has been set, prints | ||
| "No configuration set". |
There was a problem hiding this comment.
🔴 User-facing help text uses "daemon" instead of "assistant" (list subcommand)
The config list subcommand's addHelpText block references "the daemon" on line 209. Per AGENTS.md, user-facing help strings must use "assistant" instead of "daemon".
| Fetches the full raw configuration from the daemon and prints it as | |
| pretty-printed JSON. If no configuration has been set, prints | |
| "No configuration set". | |
| Fetches the full raw configuration from the assistant and prints it as | |
| pretty-printed JSON. If no configuration has been set, prints | |
| "No configuration set". |
Was this helpful? React with 👍 or 👎 to provide feedback.
| This subcommand reads the allowlist file directly from disk via a dynamic | ||
| require — no daemon route exists yet. (TODO: add config_allowlist_validate | ||
| IPC route so this command can be a pure thin wrapper too.) |
There was a problem hiding this comment.
🔴 User-facing help text exposes internal implementation details and uses "daemon"
The config validate-allowlist subcommand's addHelpText block on lines 266–268 says "This subcommand reads the allowlist file directly from disk via a dynamic require — no daemon route exists yet." This violates two AGENTS.md rules: (1) user-facing text must say "assistant" not "daemon", and (2) internal implementation details ("dynamic require", "IPC route", TODO notes) should not be exposed in --help output that users and AI agents read.
| This subcommand reads the allowlist file directly from disk via a dynamic | |
| require — no daemon route exists yet. (TODO: add config_allowlist_validate | |
| IPC route so this command can be a pure thin wrapper too.) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| * Helper modules (no `registerCommand` call) are exempt from the rule, | ||
| * so the daemon-internal import below is allowed here. | ||
| */ | ||
| export { getNestedValue, setNestedValue } from "../../config/loader.js"; |
There was a problem hiding this comment.
🚩 nested-value.ts re-export pulls in full config/loader.ts module at import time
The re-export at assistant/src/cli/lib/nested-value.ts:13 imports from ../../config/loader.ts, which has module-level side effects: it imports logger, platform utilities, feature flags, and initializes module-level variables (cached, loading, etc.). While getNestedValue and setNestedValue themselves are pure, importing them causes the entire config/loader.ts module to be evaluated. For ipc-tagged CLI commands, this means daemon-internal modules are loaded into the CLI process even though they're not used. This doesn't cause a runtime error (the side effects are benign initialization), but it increases CLI startup time and memory usage. A cleaner approach would be to extract these two pure functions into their own module.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce78802785
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Build a single-key deep-merge patch object from the dotted path. | ||
| const patch: Record<string, unknown> = {}; | ||
| setNestedValue(patch, key, parsed); | ||
|
|
||
| const result = await cliIpcCall("config_patch", { body: patch }); |
There was a problem hiding this comment.
config set now always forwards a one-key patch through config_patch, which uses deepMergeOverwrite semantics; when the target subtree/key does not yet exist, null leaves are stripped instead of persisted. This regresses prior CLI behavior: commands like assistant config set heartbeat.activeHoursStart null (or any nullable key under an unset parent) silently no-op and the daemon falls back to schema defaults instead of storing the explicit null override. That breaks disabling nullable settings on fresh configs unless users know a multi-step workaround.
Useful? React with 👍 / 👎.
…view Three issues caught in #30262 review, fixed together: 1. **Functional regression (Codex P1).** Routing `config set` through `config_patch` went through `deepMergeOverwrite`, which strips `null` leaves under newly-introduced subtrees and merges object subtrees rather than replacing them. So `config set heartbeat.activeHoursStart null` lost the null and `config set llm.profiles {}` no-op'd instead of clearing. Add a separate `config_set` IPC route that calls `setNestedValue` directly. Direct replacement, null preserved. 2. **User-facing text (Devin).** AGENTS.md mandates "assistant" not "daemon" in help/error text shown to users. Rewrote the addHelpText blocks accordingly. Internal docstrings/comments still say "daemon" (matches the rest of the codebase's internal terminology). 3. **validate-allowlist under ipc tag (Devin).** Was doing a dynamic `require("../../security/secret-allowlist.js")` from an IPC-tagged command. Move it behind a new `config_allowlist_validate` route so the CLI no longer reaches into daemon-internal modules. Implementation notes: - New routes `config_set` (POST /v1/config/set) and `config_allowlist_validate` (GET /v1/config/allowlist/validate), both in `conversation-query-routes.ts`. Both write paths share a new `commitConfigWrite` helper (watcher suppress + saveRawConfig + cache invalidation + provider re-init + fingerprint). - Route-policy entries added: `config/set:POST → settings.write`, `config/allowlist/validate:GET → settings.read`. - `cli/lib/nested-value.ts` is now an inlined duplicate of the loader helpers (not a bridge re-export). Documented in the file header. The CLI must not import `config/loader.js` because loader has module-load side effects (cache + watcher) that shouldn't run in the CLI process. - Tests rewritten to mock `ipc/cli-client.js` instead of `config/loader.js`. The platform-guard test now asserts on IPC call shape; the schema CLI test verifies that a daemon 400 maps to exit code 2 via `exitFromIpcResult`. - OpenAPI regenerated for the two new routes.
|
Pushed
Test rewrites:
OpenAPI regenerated for the two new routes. Lint + typecheck clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e526a11d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { path, value } = body as { path?: unknown; value?: unknown }; | ||
| if (typeof path !== "string" || path.length === 0) { | ||
| throw new BadRequestError("`path` must be a non-empty string"); |
There was a problem hiding this comment.
value
handleSetConfig only validates path, so a request like POST /v1/config/set with { "path": "foo" } is accepted and passes value === undefined into setNestedValue. When this is persisted, JSON.stringify drops undefined, which effectively removes the key instead of returning a 400. Because this route is exposed over HTTP/IPC (not just the CLI), malformed callers can silently delete config keys; require that the value field is present (while still allowing explicit null).
Useful? React with 👍 / 👎.
| function handleValidateAllowlist() { | ||
| const errors = validateAllowlistFile(); | ||
| if (errors == null) return { exists: false } as const; |
There was a problem hiding this comment.
handleValidateAllowlist calls validateAllowlistFile() directly, but that helper does a raw JSON.parse and can throw on malformed secret-allowlist.json. Before this change, the CLI wrapped this path in try/catch and returned a user-readable validation failure; now the same input bubbles up as a daemon 500/IPC error path. Wrapping this route in a local try/catch preserves predictable CLI behavior for malformed files.
Useful? React with 👍 / 👎.
Two findings from Codex review 4260902788 on 7e526a1: **P2 — `config_set` accepts missing `value` field** `handleSetConfig` validated `path` but not the presence of `value`. A request body like `{ "path": "foo" }` (no `value` key) passed `undefined` into `setNestedValue`, which then got dropped by `JSON.stringify` at save time — silently deleting the key. That is distinct from the documented "set to null to clear" semantics and made the route a footgun. Add an explicit `"value" in bodyRecord` check that throws a `BadRequestError("`value` is required (use `null` to clear a key)")`. Explicit `null` is still accepted (the existing null-preservation path in the writer handles it). **P3 — `config_allowlist_validate` doesn't catch JSON.parse errors** `validateAllowlistFile()` does a raw `JSON.parse` on `secret-allowlist.json` and can throw on malformed JSON. With the route calling it directly, a parse error propagated as a 500 — losing the pre-IPC behavior of printing a user-readable message and exiting 1. Wrap the handler in try/catch and return `{ exists: true, parseError: message, errors: [] }` on failure. The CLI side now renders `parseError` as `Failed to read secret-allowlist.json: <msg>` and exits 1, matching the pre-refactor surface. **Tests** New `assistant/src/__tests__/config-set-route.test.ts` covering both routes daemon-side (9 tests, all pass): - `config_set`: rejects missing `value`, accepts explicit null, rejects missing/empty `path`, rejects non-object body, writes scalar through to raw config. - `config_allowlist_validate`: returns `parseError` when underlying validator throws, returns `{ exists: false }` when file is absent, returns `{ exists: true, errors }` (empty or with pattern details) for the well-formed cases. No public API change beyond the new `parseError` field on the validate response, which the CLI is the only consumer of.
|
@codex review Round 2 — addressed both findings from review 4260902788: P2 ( P3 ( Added daemon-side tests in Head: |
1 participant
Why
configwas the last general-purpose CLI command still taggedtransport: "local", reading and writingconfig.jsonstraight from the CLI process. That bypassed the daemon's in-memory config cache,ConfigWatchersuppression, embedding cache invalidation, and provider re-init - soassistant config set llm.default.model gpt-5would touch the file on disk but leave the running daemon stale until a restart or an unrelated PATCH happened to flush the cache.Per offline conversation, the call is: every general-purpose CLI command should be a thin IPC wrapper. This PR closes the gap.
What changed
CLI (
cli/commands/config.ts):transport: "ipc". Dropped directloadRawConfig/saveRawConfig/AssistantConfigSchema/getSchemaAtPathimports.set <key> <value>now builds a single-key deep-merge patch from the dotted path and sends it throughcliIpcCall("config_patch", { body: patch }). The daemon's existinghandlePatchConfigalready deep-merges, suppresses the watcher, invalidates the embedding cache, and reinits providers.get <key>andlistfetch the full raw config viacliIpcCall("config_get")and walk the dotted path / flatten in the CLI process. Pure reads.schema [path]calls a newconfig_schema_getIPC route (see below).validate-allowlistis unchanged - uses a dynamicrequire()to loadsecurity/secret-allowlist.js, which has no daemon globals and only readssecret-allowlist.jsonfrom disk. I added a TODO for a futureconfig_allowlist_validatedaemon route so this subcommand can become a pure thin wrapper too.Helper bridge (
cli/lib/nested-value.ts, new):Thin re-export of
getNestedValue/setNestedValuefromconfig/loader.js. These two helpers are pure and dependency-free, but thecli/no-daemon-internalsESLint rule forbidsipc-tagged commands from importingconfig/loader.jsdirectly (because other exports from that module pull in daemon-only globals). Helper modules with noregisterCommandcall are exempt from the rule, so the canonical pattern is to bridge throughcli/lib/.Route (
runtime/routes/conversation-query-routes.ts):handleGetConfigSchema({ queryParams })handler. Pure function: callsz.toJSONSchema(AssistantConfigSchema, { unrepresentable: "any", io: "input" })for the full schema, orgetSchemaAtPath+toJSONSchemafor the scoped case. No daemon state touched.operationId: "config_schema_get",GET config/schema, optional?path=query param.Policy (
runtime/auth/route-policy.ts):config/schema:GET → settings.read. Mirrors the existingconfig:GETactor scope.CLI surface after this PR
After this PR + #30261 (autonomy delete) lands, the only non-IPC CLI commands are the ones that genuinely shouldn't be IPC:
transport: "local":keys(hybrid - uses the daemon-credential-client side-channel),completions(shell scripts),credential-execution(CES RPC, different transport)transport: "bootstrap":default-action(interactive entry, starts the daemon)Test plan
bun run typecheck✅bun run lint✅ (the helper-extraction pattern incli/lib/nested-value.tsclears the four would-be forbidden-import errors on the new IPC command)config_get,config_patch). The newconfig_schema_getis a pure function overAssistantConfigSchemawith no state - failure modes are the same as the inline path it replaces.Pre-merge manual smoke I'd suggest running:
Each of those should round-trip through the running daemon now.