feat(cli): migrate ui + inference-session to thin IPC wrappers#30244
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: ce093823cc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| endpoint: "inference/send", | ||
| method: "POST", | ||
| policyKey: "inference/send", |
There was a problem hiding this comment.
Adding inference/send to the shared route table exposes it over HTTP, but this commit does not add a matching entry in assistant/src/runtime/auth/route-policy.ts. Because unregistered endpoints are treated as unprotected by enforcePolicy, any authenticated token (regardless of scopes like chat.write/settings.write) can call this LLM-send endpoint, which is a scope bypass for a write/compute-costing operation.
Useful? React with 👍 / 👎.
| const effectiveTtlSeconds = | ||
| ttlSeconds !== undefined ? ttlSeconds : defaultTtlSeconds; | ||
| ttlSeconds !== undefined ? ttlSeconds : DEFAULT_TTL_SECONDS; |
There was a problem hiding this comment.
This hard-codes the no---ttl path to 1800 seconds and stops reading llm.profileSession.defaultTtlSeconds, so user config no longer affects assistant inference session open defaults. In workspaces that intentionally set a different default TTL (for example 2h), the command now silently opens 30-minute sessions instead of honoring configured behavior.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| * Default session TTL in seconds when --ttl is not specified. | ||
| * Matches the documented default of 30 minutes. | ||
| */ | ||
| const DEFAULT_TTL_SECONDS = 1800; |
There was a problem hiding this comment.
🟡 User-configured defaultTtlSeconds silently ignored after IPC migration
The old CLI code read the user's configured default session TTL from llm.profileSession.defaultTtlSeconds via getConfigReadOnly(), but the new code replaces it with a hardcoded DEFAULT_TTL_SECONDS = 1800. This means any user who customized their default TTL (e.g. set it to 3600 for 1-hour sessions) will now silently get 30-minute sessions when --ttl is omitted. The config schema at assistant/src/config/schemas/llm.ts:378-380 still documents defaultTtlSeconds as "read by the CLI" and the field remains in the schema, so users have no indication their setting is now dead. The daemon-side handler (setInferenceProfileSession) doesn't apply a default — it only clamps to maxTtlSeconds — so the CLI was the sole consumer of this setting. This violates the backwards compatibility rule in AGENTS.md: "Never ship a change that silently breaks existing behavior."
Prompt for agents
The CLI can no longer read config directly because it is now an ipc-tagged command (the cli/no-daemon-internals ESLint rule forbids importing from config/loader.js). The defaultTtlSeconds config setting is now silently ignored.
To fix this properly, the responsibility for applying the default TTL should move to the daemon-side handler in assistant/src/runtime/routes/inference-profile-session-handler.ts. Specifically:
1. In setInferenceProfileSession(), when ttlSeconds is undefined, instead of treating it as sticky/no-expiry, read llm.profileSession.defaultTtlSeconds from config and use that value.
2. In the CLI (inference-session.ts), when --ttl is not specified, do NOT send ttlSeconds at all (let it be undefined in the IPC body) so the daemon can apply the configured default.
3. Update the config schema comment at assistant/src/config/schemas/llm.ts:378-380 to reflect that the daemon handler (not the CLI) now reads defaultTtlSeconds.
This preserves backwards compatibility for users who have customized the setting while keeping the CLI as a thin IPC wrapper.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const llm = program | ||
| .command("llm") |
There was a problem hiding this comment.
🚩 llm alias command not wrapped in registerCommand()
The llm command at inference.ts:202-204 is created directly with program.command('llm') rather than via registerCommand({ transport: 'ipc', ... }). The CLI AGENTS.md states "Every command file declares its transport class via registerCommand()". However, llm is a lightweight alias that only attaches send (which is shared with the inference command that IS registered properly). The registerCommand function itself only creates the command and calls build — the transport field has no runtime effect. The llm alias also doesn't appear in COMMAND_INVENTORY.md as a separate entry. This is borderline but likely intentional since llm is just a namespace alias, not a standalone command.
(Refers to lines 202-204)
Was this helpful? React with 👍 or 👎 to provide feedback.
| process.stdout.write(JSON.stringify({ ok: true }) + "\n"); | ||
| } else { | ||
| process.stdout.write(`Deleted connection "${name}"\n`); | ||
| if (referencingProfiles.length > 0 && opts.force) { | ||
| process.stdout.write( | ||
| `Warning: ${referencingProfiles.length} profile(s) now reference a missing connection: ` + | ||
| `${referencingProfiles.join(", ")}\n`, | ||
| ); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🚩 Delete subcommand lost --force flag and profile-reference warning
The delete subcommand in inference-providers.ts removed the --force flag and the client-side check for profiles referencing the connection being deleted. The server-side handler at inference-provider-connection-routes.ts:122-163 now handles this logic entirely: it checks for referencing profiles and call sites and returns a ConflictError (409) if any exist. The server provides no --force bypass — deletion is simply rejected when references exist. This is a behavioral change (no more force-delete), but the server-side approach is arguably safer. Users who relied on --force to delete referenced connections will now get an error telling them to update the references first.
(Refers to lines 288-311)
Was this helpful? React with 👍 or 👎 to provide feedback.
…ation/pr-1 # Conflicts: # assistant/src/cli/COMMAND_INVENTORY.md
) The CLI IPC refactor PRs (#30238–#30247) added new shared routes without matching policy entries in route-policy.ts. The route-policy coverage guard test fails on main with 29 missing endpoints: - audit, auth/info - conversations/cli/{list,create,export,clear} - inference/send - internal/mcp/{list,add,remove} - oauth/apps.upsert, oauth/apps/lookup - oauth/providers.{register,update,delete} - platform/{status,connect,disconnect,callback-routes/register,callback-routes} - sequences/{list,get,pause,resume,cancel-enrollment,stats,guardrails} - user-routes/{list,inspect} http-router.ts treats unregistered endpoints as unprotected (enforcePolicy no-ops on missing keys), so any authenticated principal could hit these over HTTP without the intended scope/principal-type gates. The IPC dispatch path bypasses enforcePolicy entirely, so the existing CLI flows are unaffected by these additions. Scope/principal assignments mirror sibling routes: - oauth/apps mutations use ACTOR_ENDPOINTS settings.write (matches oauth/apps.create/.delete already present) - oauth/providers mutations use ACTOR_ENDPOINTS settings.write - oauth/apps/lookup uses ACTOR_ENDPOINTS settings.read - auth/info uses ACTOR_ENDPOINTS settings.read - internal/mcp/{list,add,remove} join INTERNAL_ENDPOINTS (gateway-only, internal.write) alongside the existing internal/mcp/reload entry - CLI-driven routes (audit, conversations/cli/*, inference/send, platform/*, sequences/*, user-routes/*) are local-only with settings.read or settings.write, mirroring tasks/*, cache/*, defer/*, domain/*, email/* — destructive 'conversations/cli/clear' elevates to settings.write to match conversations/clear-all - inference/send uses chat.write (LLM dispatch on behalf of caller), aligned with tts/synthesize-cli + stt/transcribe-file Closes route-policy coverage findings on PRs #30241 (sequences, user-routes), #30242 (conversations/cli/clear), #30244 (inference/send), #30245 (internal/mcp), #30246 (oauth/providers, oauth/apps), #30247 (platform/*). Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
1 participant
Part of plan: cli-ipc-migration.md (PR 1 of 11)