Skip to content

feat(cli): migrate platform commands to thin IPC wrappers#30247

Merged
noanflaherty merged 1 commit into
mainfrom
cli-ipc-migration/pr-11-platform
May 11, 2026
Merged

feat(cli): migrate platform commands to thin IPC wrappers#30247
noanflaherty merged 1 commit into
mainfrom
cli-ipc-migration/pr-11-platform

Conversation

@noanflaherty
Copy link
Copy Markdown
Contributor

@noanflaherty noanflaherty commented May 11, 2026

Part of plan: cli-ipc-migration.md (PR 11 of 11)


Open in Devin Review

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@noanflaherty noanflaherty merged commit f98a6cb into main May 11, 2026
8 of 12 checks passed
@noanflaherty noanflaherty deleted the cli-ipc-migration/pr-11-platform branch May 11, 2026 02:37
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment on lines +75 to +76
| `platform/connect` | _(inline in platform/index)_ | `ipc` | `platform_connect` | `THIN` | |
| `platform/disconnect` | _(inline in platform/index)_ | `ipc` | `platform_disconnect` | `THIN` | |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 COMMAND_INVENTORY.md status values are non-standard for connect/disconnect

The COMMAND_INVENTORY.md defines three status values: THIN, LOCAL, and LEGACY. The updated entries for platform/connect and platform/disconnect use _(inline in platform/index)_ which doesn't match any defined status. These commands ARE now thin IPC wrappers (they import only from ipc/cli-client.js and cli/output.js), so they should arguably be marked THIN. Additionally, they're not truly "inline" — they live in separate files (connect.ts, disconnect.ts) imported by index.ts. The table is truncated so the full status column isn't visible, but the description column value is misleading.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f4dff0e94

ℹ️ 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".

Comment on lines +187 to +189
await assistantEventHub.publish(
buildAssistantEvent({ type: "platform_disconnected" }),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't fail disconnect after event broadcast errors

assistantEventHub.publish(...) can throw an AggregateError when any subscriber callback fails, and this call happens after credentials are already deleted. In that case platform_disconnect returns a 500 even though the disconnect side effect has succeeded, so users see a failure and may retry only to get “nothing to disconnect.” This is a behavioral regression from the prior signal-file path, which did not couple disconnect success to event fanout reliability; treat publish failures as best-effort (log/ignore) once credential deletion has completed.

Useful? React with 👍 / 👎.

noanflaherty pushed a commit that referenced this pull request May 11, 2026
)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant