feat(cli): migrate sequence + routes to thin IPC wrappers#30241
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: fd85eacf2a
ℹ️ 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".
| const basePath = join(routesDir, routePath); | ||
|
|
||
| for (const ext of HANDLER_EXTENSIONS) { | ||
| const candidate = `${basePath}${ext}`; | ||
| if (existsSync(candidate)) { |
There was a problem hiding this comment.
resolveHandlerFile() joins routePath directly onto routesDir without a containment check, so a caller can pass values like ../../runtime/routes/index and make user_routes_inspect read/import files outside /workspace/routes. In this migration the handler now runs inside the daemon and is exposed through shared routes, so this becomes a server-side path traversal/import primitive rather than a local CLI-only behavior; mirror the dispatcher’s resolve(...).startsWith(resolve(routesDir)) guard before probing candidates.
Useful? React with 👍 / 👎.
| operationId: "sequence_list", | ||
| method: "POST", | ||
| endpoint: "sequences/list", | ||
| handler: handleSequenceList, |
There was a problem hiding this comment.
These new shared sequences/* routes are added without policyKey overrides and there are no corresponding registerPolicy(...) entries for derived keys like sequences/list in runtime/auth/route-policy.ts, so HttpRouter treats them as unprotected (enforcePolicy no-ops on missing policy). This also violates the route-policy coverage guard (runtime/auth/__tests__/guard-tests.test.ts) that requires each dispatched endpoint to have a policy mapping.
Useful? React with 👍 / 👎.
| operationId: "user_routes_list", | ||
| method: "GET", | ||
| endpoint: "user-routes/list", | ||
| handler: handleUserRoutesList, |
There was a problem hiding this comment.
The new user-routes/list and user-routes/inspect route definitions also lack policy mapping, so any authenticated principal can hit these endpoints regardless of intended scope/profile, and the route-policy coverage guard will report missing policies for these derived keys. Add explicit policyKey values and matching entries in runtime/auth/route-policy.ts (or register the derived keys directly) before exposing them through the shared route table.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const r = await cliIpcCall<{ sequences: SequenceSummary[] }>( | ||
| "sequence_list", | ||
| params, | ||
| ); |
There was a problem hiding this comment.
🔴 CLI passes IPC params at top level but route handlers read from body, breaking all parameterized commands
All parameterized IPC calls in sequence.ts and routes.ts pass flat params (e.g., cliIpcCall("sequence_pause", { id })) but the shared route handlers destructure { body = {} } from RouteHandlerArgs and parse body with Zod. The IPC server (assistant-server.ts:346-350) spreads req.params as top-level RouteHandlerArgs fields via injectLocalActorHeader (assistant-server.ts:584-587), never wrapping them in a body property. So body is always undefined → defaults to {} → Zod parse fails for required fields (id, path, key/value, enrollmentId) and silently ignores optional fields (status filter in sequence_list).
Affected commands and their failure modes
sequence list --status active→ status filter silently ignored (returns all sequences)sequence get <id>→ Zod error:idrequiredsequence pause <id>→ Zod error:idrequiredsequence resume <id>→ Zod error:idrequiredsequence cancel-enrollment <id>→ Zod error:enrollmentIdrequiredsequence guardrails set <key> <value>→ Zod error:key/valuerequiredroutes inspect <path>→ Zod error:pathrequired
Prompt for agents
The CLI passes IPC params as flat objects (e.g. { id: "xxx" }) but the shared route handlers read from the body field of RouteHandlerArgs. The IPC adapter in assistant-server.ts (injectLocalActorHeader) casts params directly to RouteHandlerArgs without wrapping them in body.
Either:
1. Wrap the params in body on the CLI side for all calls: cliIpcCall("sequence_pause", { body: { id } }) — check what convention existing working THIN commands use (e.g. task commands, avatar commands).
2. OR modify injectLocalActorHeader in assistant-server.ts to automatically place non-RouteHandlerArgs keys into body.
This affects all parameterized calls in assistant/src/cli/commands/sequence.ts (sequence_list, sequence_get, sequence_pause, sequence_resume, sequence_cancel_enrollment, sequence_guardrails_set) and assistant/src/cli/commands/routes.ts (user_routes_inspect).
Was this helpful? React with 👍 or 👎 to provide feedback.
| * discovery and inspection endpoints for CLI consumption. The filesystem | ||
| * scanning logic that was previously in the CLI command is now here. |
There was a problem hiding this comment.
🔴 Comment references removed code, violating mandatory AGENTS.md rule
The module comment at user-routes-cli.ts:6 says "The filesystem scanning logic that was previously in the CLI command is now here." This uses the phrase "was previously in" to narrate history, which violates the mandatory rule in assistant/AGENTS.md: "do not reference code that has been removed … Avoid phrases like 'no longer does X', 'previously used Y', or 'was removed in PR Z'." Comments should describe the current state only.
| * discovery and inspection endpoints for CLI consumption. The filesystem | |
| * scanning logic that was previously in the CLI command is now here. | |
| * discovery and inspection endpoints for CLI consumption. Contains the | |
| * filesystem scanning and module-loading logic for route discovery. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const ROUTES: RouteDefinition[] = [ | ||
| { | ||
| operationId: "user_routes_list", | ||
| method: "GET", | ||
| endpoint: "user-routes/list", | ||
| handler: handleUserRoutesList, | ||
| summary: "List user-defined route handlers", | ||
| description: | ||
| "Scan workspace routes directory for handler files and return discovered routes with methods and public URLs.", | ||
| tags: ["user-routes"], | ||
| }, |
There was a problem hiding this comment.
🚩 GET handler user_routes_list executes dynamic imports which have potential side effects
The handleUserRoutesList handler is registered as a GET route (method: "GET") but internally calls discoverRoutes() → inspectModule() → await import(filePath), which executes arbitrary user-authored code. The assistant/src/runtime/AGENTS.md states "GET handlers must be safe and side-effect-free." Dynamic imports of user code can theoretically produce side effects (file writes, network calls). However, the import is purely for inspection (detecting exported HTTP methods and descriptions), and this exact logic existed in the old CLI code. The risk is low since user route handlers are already trusted code in the workspace. A POST method might be more semantically appropriate given the import side effects, but this is a judgment call rather than a clear bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
) 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
Summary
Part of plan: cli-ipc-migration.md (PR 7 of 11)