feat(cli): advisory ESLint rule cli/no-daemon-internals#30157
Conversation
![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: da7ee746f8
ℹ️ 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 allowedPrefixes = ALLOWED_PREFIXES[transport]; | ||
| if (!allowedPrefixes) { | ||
| // Unknown transport — no allowlist to enforce. | ||
| return; | ||
| } |
There was a problem hiding this comment.
If a command declares transport with a typo (for example "ipcc"), the rule silently returns without reporting anything, so import restrictions are skipped entirely for that file. This creates an easy accidental bypass where a migrated command appears compliant but can still import daemon internals unchecked; the rule should emit a diagnostic for unrecognized transport values.
Useful? React with 👍 / 👎.
| "../logger", | ||
| "../output", | ||
| "../lib/", |
There was a problem hiding this comment.
The allowlist hardcodes ../logger, ../output, and ../lib/, which only match files directly under src/cli/commands/. Commands in nested folders (such as src/cli/commands/platform/* or oauth/*) use ../../... relative imports, so once they add registerCommand({ transport }) they will be falsely reported as forbidden imports, creating noisy lint and blocking migration.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const ALLOWED_PREFIXES: Record<string, string[]> = { | ||
| ipc: [ | ||
| "node:", | ||
| "bun:", | ||
| "commander", | ||
| "../../ipc/cli-client", | ||
| "../logger", | ||
| "../output", | ||
| "../lib/", | ||
| ], | ||
| local: [ | ||
| "node:", | ||
| "bun:", | ||
| "commander", | ||
| "../../config/loader", | ||
| "../../config/schema", | ||
| "../../util/platform", | ||
| "../lib/", | ||
| ], | ||
| bootstrap: [ | ||
| "node:", | ||
| "bun:", | ||
| "commander", | ||
| "../../config/loader", | ||
| "../../config/schema", | ||
| "../../util/platform", | ||
| "../lib/", | ||
| ], | ||
| }; |
There was a problem hiding this comment.
🟡 ALLOWED_PREFIXES assume single directory depth but ESLint config glob matches nested subdirectories
The ALLOWED_PREFIXES in the rule use relative paths hardcoded for files at src/cli/commands/*.ts depth (e.g., ../../ipc/cli-client, ../logger, ../lib/). However, the ESLint config at assistant/eslint.config.mjs:49 applies the rule to src/cli/commands/**/*.ts, which also matches files in subdirectories like oauth/ and platform/. Files at those deeper paths use different relative import prefixes — e.g., src/cli/commands/oauth/connect.ts:6 imports ../../../ipc/cli-client.js, not ../../ipc/cli-client.js. If any subdirectory file adds a registerCommand() call with a transport tag, all of its imports would be flagged as forbiddenImport even when they're perfectly valid, because the prefix depths don't match.
Currently no subdirectory files call registerCommand, so only the missingTransport path fires for them. But the mismatch between the glob scope and the prefix assumptions is a latent correctness issue.
Prompt for agents
The ALLOWED_PREFIXES record at cli-no-daemon-internals.ts:21-49 uses hardcoded relative path prefixes (e.g. '../../ipc/cli-client', '../logger') that assume command files live directly under src/cli/commands/. But the ESLint config glob at eslint.config.mjs:49 is 'src/cli/commands/**/*.ts', which also matches files in subdirectories (oauth/, platform/). Those files use deeper relative paths like '../../../ipc/cli-client.js'.
To fix this, the rule should either:
1. Normalize the import source relative to a fixed root (e.g. resolve the import path relative to the file being linted and compare against absolute-style module identifiers), or
2. Express the allowlist in terms of the resolved module path (e.g. 'src/ipc/cli-client', 'src/cli/logger') and use context.filename to resolve each import, or
3. Narrow the ESLint config glob to only match top-level command files: 'src/cli/commands/*.ts' (removing the ** wildcard).
Option 3 is simplest if subdirectory files are intentionally excluded from the forbiddenImport check. Options 1 or 2 are more robust if the rule should cover all depths.
Was this helpful? React with 👍 or 👎 to provide feedback.
| files: ["src/cli/commands/**/*.ts"], | ||
| ignores: ["src/cli/commands/**/__tests__/**"], | ||
| plugins: { cli: { rules: { "no-daemon-internals": cliNoDaemonInternals } } }, | ||
| rules: { "cli/no-daemon-internals": "warn" }, |
There was a problem hiding this comment.
🚩 Rule will fire missingTransport on ~38 existing command files
The rule fires missingTransport on any file in src/cli/commands/**/*.ts that has imports but no registerCommand() call. There are approximately 38 such files (e.g., conversations.ts, credentials.ts, all oauth/*.ts helpers, platform/*.ts). Since the rule is set to "warn" at assistant/eslint.config.mjs:52, this won't break CI, but it will produce significant lint noise. This is presumably intentional — the rule is being introduced incrementally to guide migration — but worth confirming that the volume of warnings is expected and won't cause developers to ignore lint output.
Was this helpful? React with 👍 or 👎 to provide feedback.
…find it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
credence-the-bot
Bot
merged commit a686a94
into
credence/cli-foundation-tags-lint
…COMMAND_INVENTORY (#30159) * feat(cli): registerCommand helper + transport tags for 21 commands + §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 (PR #30157) * 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> * feat(cli): COMMAND_INVENTORY.md + bidirectional CI check + AGENTS.md 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> * fix(cli): address Codex + Devin findings on Section B rollup 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. * fix(eslint-rule): cycle-1 review findings — chained registerCommand + 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. * fix(cli): status — narrow zero-exit fallback to daemon-unreachable cases (cycle-3) --------- Co-authored-by: credence-the-bot[bot] <277301654+credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: credence-the-bot[bot] <test@test.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: credence-the-bot <195577519+credence-the-bot@users.noreply.github.com> Co-authored-by: credence-the-bot <226213748+credence-the-bot@users.noreply.github.com>
Summary
Part of plan: cli-foundation-tags-lint.md (PR 2 of 3)