feat(cli): Section B foundation — registerCommand tags, ESLint rule, COMMAND_INVENTORY#30159
Conversation
…§3.7 daemon-down message (#30156) * feat(cli): registerCommand helper + transport tags for 21 commands + §3.7 daemon-down message * fix(lint): sort imports in cache.ts, clients.ts, register-command.test.ts * fix(tests): use console.log in status.ts; update exit-helper test for §3.7 message * fix(tests): use process.stdout.write in status.ts for testable output * fix: revert daemon terminology per AGENTS.md; use 'the assistant' in user-facing messages; 'Assistant: running/down' in status --------- Co-authored-by: credence-the-bot[bot] <test@test.com>
* feat(cli): advisory ESLint rule cli/no-daemon-internals * fix(lint): convert ESLint rule to plain JS so Node.js ESM loader can find it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: credence-the-bot[bot] <test@test.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…canonical example (PR #30158) * feat(cli): COMMAND_INVENTORY.md + bidirectional CI check + AGENTS.md canonical example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(lint): sort named imports alphabetically in check-cli-inventory.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(lint): correct named import sort order in check-cli-inventory.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: credence-the-bot[bot] <test@test.com> Co-authored-by: Claude Sonnet 4.6 <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: 8869493c13
ℹ️ 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".
| files: ["src/cli/commands/**/*.ts"], | ||
| ignores: ["src/cli/commands/**/__tests__/**"], |
There was a problem hiding this comment.
Applying cli/no-daemon-internals to every src/cli/commands/**/*.ts file causes persistent false positives on helper modules that are not command registrars (for example src/cli/commands/oauth/shared.ts imports modules but intentionally has no registerCommand(...) call). Those files can only trigger missingTransport, which adds noise and makes real transport-boundary warnings harder to spot during migration.
Useful? React with 👍 / 👎.
| local: [ | ||
| "node:", | ||
| "bun:", | ||
| "commander", | ||
| "../../config/loader", |
There was a problem hiding this comment.
The local allowlist omits common CLI output imports like ../logger (and related output helpers), so newly tagged local commands in this same change (autonomy.ts, config.ts, completions.ts, keys.ts, credential-execution.ts) are immediately reported as violations despite following the intended transport tagging. This makes the rule noisy and blocks using warnings as a reliable migration signal.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| local: [ | ||
| "node:", | ||
| "bun:", | ||
| "commander", | ||
| "../../config/loader", | ||
| "../../config/schema", | ||
| "../../util/platform", | ||
| "../lib/", | ||
| ], |
There was a problem hiding this comment.
🟡 ESLint local/bootstrap allowlists missing ../logger and ../output prefixes
The ipc transport allowlist includes "../logger" and "../output" (lines 7-8), but the local and bootstrap allowlists omit them. These are pure CLI utilities (assistant/src/cli/logger.ts, assistant/src/cli/output.ts) with no daemon-internal dependencies, yet every local-tagged command that imports them will produce a false-positive forbiddenImport warning. All five local-tagged commands in this PR import ../logger.js: autonomy.ts, config.ts, completions.ts, keys.ts, and credential-execution.ts (the latter also imports ../output.js). This significantly reduces the signal-to-noise ratio of the advisory lint rule.
| local: [ | |
| "node:", | |
| "bun:", | |
| "commander", | |
| "../../config/loader", | |
| "../../config/schema", | |
| "../../util/platform", | |
| "../lib/", | |
| ], | |
| local: [ | |
| "node:", | |
| "bun:", | |
| "commander", | |
| "../../config/loader", | |
| "../../config/schema", | |
| "../../util/platform", | |
| "../logger", | |
| "../output", | |
| "../lib/", | |
| ], |
Was this helpful? React with 👍 or 👎 to provide feedback.
| missingTransport: | ||
| "CLI command file must call registerCommand({ transport: ... }) to declare its transport class.", | ||
| forbiddenImport: | ||
| "'{{transport}}'-tagged CLI command imports forbidden module '{{source}}'. See DESIGN.md §3.1 for allowed imports.", |
There was a problem hiding this comment.
🟡 ESLint error message references nonexistent DESIGN.md §3.1
The forbiddenImport message says "See DESIGN.md §3.1 for allowed imports" but no DESIGN.md file exists anywhere in the repository (confirmed via find). Developers encountering this lint warning will look for a doc that doesn't exist. The reference should point to COMMAND_INVENTORY.md or src/cli/AGENTS.md, both of which are introduced in this PR and describe the allowed import rules.
| "'{{transport}}'-tagged CLI command imports forbidden module '{{source}}'. See DESIGN.md §3.1 for allowed imports.", | |
| "'{{transport}}'-tagged CLI command imports forbidden module '{{source}}'. See src/cli/AGENTS.md for allowed imports.", |
Was this helpful? React with 👍 or 👎 to provide feedback.
| - Single `registerCommand({ transport: "ipc", ... })` call | ||
| - One `cliIpcCall` per subcommand action | ||
| - Table output with `--json` flag alternative | ||
| - Clean error handling via `exitFromIpcResult` |
There was a problem hiding this comment.
🟡 AGENTS.md canonical example claims pending.ts uses exitFromIpcResult but it doesn't
The newly added canonical example section in assistant/src/cli/AGENTS.md:54 states pending.ts uses "Clean error handling via exitFromIpcResult", but pending.ts never imports or calls exitFromIpcResult — it uses manual log.error() + process.exitCode = 1 instead. No command file under src/cli/commands/ uses exitFromIpcResult. Since AGENTS.md is a mandatory instruction file, agents following this guidance to model new commands after pending.ts will look for a pattern that doesn't exist in the reference implementation.
| - Clean error handling via `exitFromIpcResult` | |
| - Clean error handling via `log.error` + `process.exitCode = 1` |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const workspace = getWorkspaceDirDisplay(); | ||
| process.stdout.write((socketExists ? "Assistant: running" : "Assistant: down") + "\n"); | ||
| process.stdout.write(`Workspace: ${workspace}\n`); | ||
| process.exit(0); |
There was a problem hiding this comment.
🚩 status.ts behavioral change: exit(1) → exit(0) on daemon-down
The old status.ts called process.exit(1) when the daemon was unreachable, making assistant status fail when the daemon was down. The new code at line 36 calls process.exit(0) with a "down"/"running" status message instead. This is an intentional design change (tested at assistant/src/cli/commands/__tests__/status.test.ts:152-158), but it changes the exit code contract for any scripts or CI pipelines that use assistant status as a daemon health check. The use of process.exit(0) instead of process.exitCode = 0; return; also skips any Commander cleanup, though this is unlikely to matter for a status command.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { McpClient } from "../../mcp/client.js"; | ||
| import { deleteMcpOAuthCredentials } from "../../mcp/mcp-oauth-provider.js"; | ||
| import { openInHostBrowser } from "../../util/browser.js"; |
There was a problem hiding this comment.
🚩 Several ipc-tagged commands import daemon-internal modules beyond the allowlist
mcp.ts (tagged ipc) imports ../../mcp/client.js, ../../mcp/mcp-oauth-provider.js, and ../../util/browser.js — these are daemon-internal modules. browser.ts imports ../../browser/operations.js and ../../browser/types.js. memory-v2.ts imports types from ../../runtime/routes/memory-v2-routes.js. These would all be flagged as forbiddenImport warnings. The COMMAND_INVENTORY.md acknowledges mcp touches config directly, but the others are labeled THIN despite importing daemon internals. Since the rule is warn, this is tolerable during migration but the inventory's THIN labels may be misleading for browser and mcp.
Was this helpful? React with 👍 or 👎 to provide feedback.
Cycle 1 fixes for PR #30159 review feedback: 1. Allowlist gap (Codex P2 / Devin 🟡): Add `../logger` and `../output` to `local` and `bootstrap` allowlists in `cli-no-daemon-internals.js`. Without this, every newly tagged `local` command (autonomy, config, completions, keys, credential-execution) false-positives on its standard logger/output imports, killing the rule's signal-to-noise ratio. 2. Doc reference (Devin 🟡): `forbiddenImport` error message pointed to nonexistent `DESIGN.md §3.1`. Now points to `src/cli/AGENTS.md` which is the actual shipped reference. 3. Target glob too broad (Codex P2): The rule fired `missingTransport` on every file under `src/cli/commands/**/*.ts` including helper modules like `oauth/shared.ts` that aren't command registrars. Narrowed by making the rule skip files that don't call `registerCommand` directly (now a tri-state `findTransport`: `string` for tagged, `MISSING_TRANSPORT` for registrars without transport prop, `null` for non-registrars). Helper modules pass silently. Backdoor closed because helpers can't be imported via blessed paths anyway (sibling `./helper.js` doesn't match any allowlist prefix). 4. Type-only imports (extends Codex P2 #1): `import type {...}` is erased at compile time and doesn't constitute a runtime boundary violation. Rule now skips type-only imports. Drops 3 false-positives on `memory-v2.ts` and `mcp.ts` type imports from runtime/routes/. 5. Canonical-example claim (Devin 🟡): `AGENTS.md` claimed `pending.ts` uses `exitFromIpcResult` but it didn't. Migrated `pending.ts` to actually use the helper added in Section A. Now matches the doc. 6. mcp.ts misclassification (Devin 🚩): `mcp.ts` was marked `THIN` in COMMAND_INVENTORY but imports `mcp/client.js`, `mcp/mcp-oauth-provider.js`, `util/browser.js` — actual daemon internals. Reclassified to `LEGACY` with a note that Section C/D will thin it. Until then the advisory rule will warn on this file (and that's the right signal). Lint: 18 warnings remain (down from 21) on partially-migrated commands (config, keys, credential-execution, status, task, mcp). All are legitimate signals — those files have non-allowlisted imports that need migration before Section J flips the rule from advisory → error. Tests: rule test suite extended with helper-module + type-only + local-with-logger regression cases; 5/5 manual smoke tests pass; tsc clean; lint:inventory passes.
Cycle 1 fixes pushed (
|
| # | Source | Finding | Fix |
|---|---|---|---|
| 1 | Codex P2 + Devin 🟡 | local/bootstrap allowlists missing ../logger/../output |
Added both to allowlists |
| 2 | Devin 🟡 | forbiddenImport referenced nonexistent DESIGN.md §3.1 |
→ src/cli/AGENTS.md |
| 3 | Codex P2 | Rule applied to non-registrar helpers (e.g. oauth/shared.ts) |
Skip files without registerCommand call |
| 4 | (extends #3) | Type-only imports false-positived | Skip importKind === "type" imports |
| 5 | Devin 🟡 | AGENTS.md claimed pending.ts uses exitFromIpcResult but it didn't |
Migrated pending.ts to actually use it |
| 6 | Devin 🚩 | mcp.ts tagged THIN but imports daemon internals |
Reclassified THIN → LEGACY in COMMAND_INVENTORY w/ note |
| 7 | Devin 🚩 | status.ts exit-code change is intentional but breaking for scripts |
Added release-note section to PR body w/ --json migration recipe |
State after cycle 1: 0 lint errors, 18 advisory warnings (was 21) — all on legitimately partially-migrated commands (config, keys, credential-execution, status, task, mcp), which is the rule's intended signal-vs-noise. CI 8/8 ✅ on bb15a8c0b.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb15a8c0bc
ℹ️ 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".
| case "CallExpression": | ||
| for (const arg of node.arguments) { | ||
| worklist.push(arg); | ||
| } |
There was a problem hiding this comment.
The AST walk only enqueues CallExpression.arguments and never the callee, so chained patterns like registerCommand(...).command(...) are treated as if no registerCommand call exists. In that case findTransport() returns null, and the rule skips all checks, allowing forbidden runtime imports and missing transport tags to pass silently in command-entry files that use chaining.
Useful? React with 👍 / 👎.
| // `import type {...}` and `import { type X }` are erased at compile | ||
| // time — they don't ship in the bundle and don't constitute a | ||
| // runtime boundary violation. Skip them. | ||
| if (importNode.importKind === "type") { | ||
| continue; |
There was a problem hiding this comment.
The rule claims to ignore both import type {...} and import { type X } ..., but it only checks importNode.importKind === "type". For inline type specifiers, importKind remains "value" at the declaration level, so these imports are still linted and incorrectly reported as forbidden. This creates false positives for type-only route/model imports and undermines the migration signal quality.
Useful? React with 👍 / 👎.
… inline-type imports
Codex P2 (cli-no-daemon-internals.js:88) — traverse callees:
The AST walk only enqueued CallExpression.arguments, never the callee.
Chained patterns like `registerCommand({...}).command(...).description(...)`
were treated as if no registerCommand call existed (findTransport returned
null), silently skipping all checks for those command-entry files.
Fix: enqueue node.callee on CallExpression and node.object on
MemberExpression so the walk reaches through the receiver chain.
Codex P2 (cli-no-daemon-internals.js:183) — inline type-only specifiers:
The rule only checked importNode.importKind === "type", which catches
`import type {...}` but not `import { type X }` — for the inline form
the declaration's importKind stays "value" and each ImportSpecifier
carries its own importKind. All-type-only inline imports are erased at
compile time and must not flag forbidden-import.
Fix: also skip when every specifier is an ImportSpecifier with
importKind === "type". Side-effect-only imports (no specifiers) and
mixed value+type imports still get linted.
Cycle 2 fix push — commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4c9d000ca
ℹ️ 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".
| if (!result.ok || !result.result) { | ||
| const socketPath = getAssistantSocketPath(); | ||
| const socketExists = existsSync(socketPath); | ||
| const workspace = getWorkspaceDirDisplay(); | ||
| process.stdout.write((socketExists ? "Assistant: running" : "Assistant: down") + "\n"); | ||
| process.stdout.write(`Workspace: ${workspace}\n`); | ||
| process.exit(0); |
There was a problem hiding this comment.
Only daemon-unreachable cases should be normalized to a successful status result, but this branch catches every !result.ok and every missing-body response and then exits 0. That masks real failures (e.g. daemon reachable but health returns an internal error or malformed payload) as a healthy command execution, so scripts can report success while the assistant is actually unhealthy. Restrict the zero-exit fallback to connection-level errors (e.g. ENOENT/ECONNREFUSED) and keep non-connection IPC failures as non-zero.
Useful? React with 👍 / 👎.
Cycle 3 — `a275ce18e`Finding addressed (Codex P2 · status.ts:36): Narrow zero-exit fallback to genuine daemon-unreachable cases only. What changed`status.ts` — Split the
`status.test.ts` — 9 tests (was 4):
Mapping
|
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
…on-tags-lint # Conflicts: # assistant/src/cli/commands/memory-v2.ts
Merged main → branch (
|
3 participants
Summary
This is the Section B foundation for the CLI refactor. Three sub-PRs land together:
registerCommandhelper (src/cli/lib/register-command.ts), transport tags (transport: "ipc" | "local") on all 21 already-landed commands, and the §3.7 daemon-down fallback instatus.tscli/no-daemon-internals: enforces import allowlists per transport class (warns, not errors — flips to error in Section J)src/cli/COMMAND_INVENTORY.md(source-of-truth table for all 58 CLI commands),scripts/check-cli-inventory.ts(bidirectional validator),lint:inventorynpm script, andsrc/cli/AGENTS.mdupdates (Transport tagging section + canonical example:pending.ts)Notable details
cache-fs.tsmoved fromcommands/→lib/(it's anode:fsre-export for test isolation, not an actual command)default-action.tsusesprogram.action()(root default handler) and cannot be wrapped withregisterCommand; taggedLOCALin the inventoryautonomyis taggedlocalper JARVIS-745 (file-write only, no daemon consumer)status.tsdaemon-down path usesprocess.stdout.write()directly (Bun'sconsole.logbypassesprocess.stdout.writeoverrides in tests)lint:inventoryneeds manual addition to.github/workflows/pr-assistant.yaml(GitHub App lacksworkflowspermission); diff is in PR feat(cli): COMMAND_INVENTORY.md + bidirectional CI check + AGENTS.md canonical example #30158 bodyRelease notes
Behavior change (script-facing):
assistant statusexit code on daemon-down.Previously
assistant statusexited with code1when the daemon was unreachable. As of this PR it exits with code0and prints a structured "daemon down" report (with--jsonreturning{ "ok": false, "reason": "daemon-not-running" }). This is intentional per DESIGN §3.7 — "daemon down" is a normal operational state, not an error.If you have scripts that grep
$?ofassistant statusto detect daemon-down, switch to:Test plan
bun run lintpasses (ESLint rule wired as warn — 18 advisory warnings on partially-migrated commands)bun run lint:inventoryexits 0bunx tsc --noEmitclean- name: Check CLI inventory / run: bun run lint:inventoryafterLint unused codestep in.github/workflows/pr-assistant.yamlReview cycle 1 fixes (commit
bb15a8c0b)Addressing Codex + Devin findings on the initial rollup:
../logger+../outputtolocalandbootstrapallowlists. Drops 5 false-positives on tagged local commands.forbiddenImporterror message → points tosrc/cli/AGENTS.md(was nonexistentDESIGN.md §3.1).registerCommandcalls. Helper modules undercommands/(e.g.oauth/shared.ts) no longer false-positive. Backdoor closed because sibling helpers aren't allowlisted as imports.import type {...}is erased at compile time and skipped. Drops 3 false-positives onmemory-v2.ts+mcp.tstype imports fromruntime/routes/.pending.tsmigrated to actually useexitFromIpcResultso AGENTS.md stops lying.THIN→LEGACYin COMMAND_INVENTORY with a note that Section C/D will thin it. The advisory rule correctly warns on this file (5 warnings, all real).After cycle 1: 0 errors, 18 warnings (down from 21), all on legitimately partially-migrated commands (
config,keys,credential-execution,status,task,mcp) — these are real Section C-I work, not noise.Part of plan: cli-foundation-tags-lint.md