Skip to content

check(cli-command-ids): every module under src/commands must BE a command, not just yield an id - #17906

Merged
claude[bot] merged 2 commits into
mainfrom
claude/issue-17869-commands-dir-command-class-guard
Sep 12, 2026
Merged

check(cli-command-ids): every module under src/commands must BE a command, not just yield an id#17906
claude[bot] merged 2 commits into
mainfrom
claude/issue-17869-commands-dir-command-class-guard

Conversation

@claude

@claude claude Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #17869

packages/cli/package.json declares the oclif command table as a glob over the emitted tree:

"commands": { "strategy": "pattern", "target": "./dist/commands", "glob": "**/*.js" }

so every module under src/commands/ is taken to be a command. A helper placed beside the command it serves — the obvious place to put it — has no default-exported command class, and oclif then writes a findCommand ... not found warning to stderr on every single os invocation, whatever the user ran. That corrupts os validate --json for any consumer reading both streams, and a misplaced file that happens not to break a --json parse ships the warning silently to every user of every command.

Nothing caught it. check-cli-command-ids.mjs already walks packages/cli/src/commands/** and already derives an id per file, but it asks the opposite question (does a command-id string literal outside the CLI resolve to a derivable command path, #12016) — and a module with no command class still yields a derivable path, so it reads as a valid id.

So the second duty lands in the same walk: no new traversal, no new verification surface, and duty one is untouched. One file changed, scripts/check-cli-command-ids.mjs.

The ruled shape (triage 5648304561), item by item, with its reading

  • scripts/check-cli-command-ids.mjs(它已经packages/cli/src/commands/** 并已逐文件推导 id ⇒ ⛔ 无需新增遍历面,也 ⛔ 无需新增验证面):对该遍历里的每个非测试模块,要求 default export 是一个命令类。
  • 探测器必须认「继承另一个命令类」 —— 本席实测 build.ts 继承 Compilemigrate/index.ts 继承 MigratePlan一个只认 extends Command 的判据会把这两个现有文件误报成违规,那是假红,会让人把守卫关掉。⇒ 判据要沿继承链走,或用 oclif 自己的判定。
  • 门的消息里点名 packages/cli/src/utils/ 为 helper 的归属地
  • 需要豁免的 helper 走声明式豁免 —— 卡说「which is the point」,本席同意:豁免要显式,⛔ 不是静默通过。
  • ablation 腿:按卡的 repro 放一个只导出普通函数的模块进去,门必须红;移除后必须绿。
  • ⚠️ 卡提到的 dist/commands/** 陈旧产物(tsup 不删已移除的输出,本地重建清不掉)⇒ 验收时要说明门读的是还是 dist;若读 dist,这条陷阱要一并处理。

1. The predicate follows the inheritance chain — proved by file name. --list prints the resolved chain per module:

packages/cli/src/commands/build.ts          Build   -> Compile     -> Command (@oclif/core)
packages/cli/src/commands/migrate/index.ts  Migrate -> MigratePlan -> Command (@oclif/core)

Both are accepted, and neither extends Command directly — a predicate accepting only extends Command would false-red exactly these two. Pinned by name in --self-test, not by count.

2. The gate is GREEN on the tree as delivered, having examined 63 modules. The examined count is printed, because 0 violations over 0 modules and 0 violations over 63 are otherwise the same line and only one of them is a reading:

✓ check-cli-command-ids: 63 module(s) under packages/cli/src/commands examined, all of them
  default-export a class whose inheritance chain reaches oclif's `Command` (0 declared exemption(s)).

A CLI package that yields zero modules is a refusal, not a pass — discoverClis already required its oclif.bin declaration and an existing src/commands dir, so an empty walk means the traversal stopped.

3. The failure message names packages/cli/src/utils/ as the home for helpers, and states the consequence (stderr warning on every os invocation; JSON.parse failing for a --json consumer; one red in 3247 cases on PR #17859, in a test with nothing to do with the misplaced module) so nobody reads it as cosmetic. Full text under §4 below.

4. The exemption is declarative and self-retiring. COMMAND_MODULE_EXEMPTIONS is one list in the gate, keyed by path, each entry carrying its reason. It is empty today, which is the tree's measured state. A stale entry reds — both ways it can rot are checked: the file left the walked population (moved, renamed, deleted), or the module now does export a command class. Same shape as the two ledgers already in this file.

5. The gate reads SOURCE, never dist. Deliberately, and that is why it needs no build and inherits no stale artefact: tsc does not delete outputs for removed sources, so a deleted src/commands/x/helper.ts leaves dist/commands/x/helper.js behind that a local rebuild does not clear. Nothing here reads dist, so that trap is not inherited.

Two things worth a reviewer's eye

The module population is deliberately WIDER than the id population. The id rule drops a dotted base (foo.helpers.ts) and a base that is not lower-kebab (_shared.ts), because such a file cannot be a command id — but tsc emits it and the glob loads it, so those are exactly the shapes a misplaced helper takes. A directory that cannot name a topic (_priv/) used to end the traversal and is now descended with id derivation switched off. Scoring this duty on the id population would have left the guard blind at precisely its own subject.

Duty one is unchanged, and that is measured rather than argued — the old and new commandSurfaceUnder were imported side by side and compared on the live tree: 74 ids, 12 topics, set-equal, zero on either side only.

It parses rather than matching text, through scripts/ts-parse.mjs (check:parse-guard's sanctioned door). create.ts, generate.ts and init.ts are scaffolders whose template literals spell export default ... as emitted code — in all three, hundreds of lines before the module's own real export default class. A text scan reading the first match answers about a string the scaffolder prints. All three resolve correctly here (Create -> Command, Generate -> Command, Init -> Command).

The ablation leg, executed

The card's own repro, with the file placed at the id from the card's stderr sample. Restore is proved by state, not by an exit code.

Leg 0 — the tree as delivered: EXIT=0, 63 module(s) ... examined, all of them default-export a class.

Mutation: packages/cli/src/commands/migrate/file-column-move.ts, plain functions only. On-disk proof before any reading: exists=yes, marker occurrences=1.

Leg 1 — with the misplaced module: EXIT=1

✗ check-cli-command-ids: module(s) under src/commands/ that do not default-export a command class:

  packages/cli/src/commands/migrate/file-column-move.ts
    it has no default export

The oclif command table is a GLOB over the emitted tree — packages/cli/package.json
  "commands": { "strategy": "pattern", "target": "./dist/commands", "glob": "**/*.js" }
so EVERY module under src/commands/ is taken to be a command. A module that is not one
makes oclif print a "findCommand ... command ID not found" warning on STDERR for EVERY
`os` invocation, whatever the user actually ran.

⛔ That is not cosmetic. `os validate --json` writes its payload to stdout, so a consumer
reading both streams — this repo's own CLI test helper does, and it is the ordinary shape
for execFileSync error handling — gets valid JSON followed by that warning, and JSON.parse
fails on it. Measured on PR #17859: ONE red in 3247 cases, in a test with nothing to do
with the misplaced module. A file that happens not to break a --json parse ships the
warning silently to every user of every command.

The fix: move the helper to packages/cli/src/utils/, which is already where CLI helpers
live (schema-migrate.ts, migrate-occupancy-gate.ts, sqlite-occupancy.ts,
data-migration-plugins.ts), and import it from the command that needs it.

A module that genuinely must sit under src/commands/ without being a command needs a
declared entry in COMMAND_MODULE_EXEMPTIONS in this file, carrying its reason — ⛔ never a
silent pass.

The self-test with the module present is also red: 1 of 57 case(s) failed, EXIT=1.

Restore: file removed; exists=no; git status --porcelain empty; git diff HEAD empty.

Leg 2 — after removal: EXIT=0, back to 63 module(s) ... examined.

Verification

All readings below are on the merged head 71777c276 (origin/main fd8b2c049 merged in; main had touched packages/cli/src/commands/info.ts, so the population was re-taken after the merge — still 63).

The derived gate set: 31 families, 31 run, every one exit 0.

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack   ->  31 commands
node scripts/pm/dispatch-gates.mjs --ran <list> --repo objectstack-ai/objectstack ->  exit 0
  Run reconciliation — 31 derived, 31 run, 0 NOT-MEASURED, 0 UNRUN.
  EXIT CODES — all 31 accounted famil(ies) carry one ... a DERIVED zero — none of them is 3.

Every exit code was captured into a variable before any pipe. The family list was re-derived on the merged head and is identical.

Plus, by name:

command exit
pnpm lint (eslint . --no-inline-config, whole repo, 95s) 0
pnpm check:cli-command-ids (the changed gate: self-test and audit) 0
pnpm check:nul-bytes (8532 text files, 0 raw control bytes) 0
pnpm check:parse-guard · check:entry-guard · check:watch-hint-literal · check:self-test-wired 0
pnpm check:pm-dispatch-gates · check:scripts-symbol-anchors · check:declaration-mirrors 0

No command exited 3; no prerequisite was unmet. pnpm lint ran over the whole repository, so no narrowing was taken and none needs proving.

Duty one, proved unchanged. The old and new commandSurfaceUnder were imported side by side and run against packages/cli/src/commands:

ids  old=74 new=74  identical=true
topics old=12 new=12 identical=true
only-old: []  only-new: []
modules (new duty): 63   old had modules key: false

The self-test grew 39 -> 57 cases, in two new batteries (the command-class predicate, against a scratch tree ×10, the command-module duty on the live repo, by name ×8). SELF_TEST_BATTERY_FLOOR was raised 8 -> 10 so neither new battery can be deleted back to silence.

Not measured, and deliberately so: the user-visible half — the findCommand ... not found line on stderr through bin/run.js. It needs packages/cli built, and its dependency closure here is 58 packages behind a shared verify lock that a sibling dev currently holds. The order marks this optional-if-cheap; it is not cheap here, so it is skipped, not measured — ⛔ not reported as observed. The card already carries that measurement from PR #17859.

Acceptance notes

Measured on the way through, and not this card's:

  • The ** git pathspec trap DOES reproduce — this bullet previously said it did not, and the seat measured it out. ⭐ Corrected in place by the domain:cli seat (comment 5649347543 on cli: a non-Command module under src/commands makes EVERY os invocation warn on stderr, and nothing catches it #17869), ⛔ not edited away, because the retraction is the useful part. The delivered reading was real: git grep -l '' -- 'packages/cli/src/commands/**/*.ts' does return 63. The inference from it was not — those are a different 63. Five probes:
    • git ls-files -- 'packages/cli/src/commands/**/*.ts'62 files, of which 0 are top-level.
    • git grep -l '' … -- <that pathspec>63 files, and that set contains commands/build.ts 0 times.
    • git grep -l -E '.' … -- <that pathspec> (a pattern matching every line) → 63, build.ts again 0.
      ⇒ the pathspec selects only what lives in a subdirectory and excludes all twenty top-level command modules; a pattern matching every line does not rescue them, so the absence is not the pattern's doing. The seat's 43 was the subdirectory modules matching ^export default class .
      ⭐ The lesson is this card's own, one level up: a COUNT IS NOT A READING OF A SET. Two populations of size 63, disjoint on twenty members, and only a membership probe — is build.ts in there? — can tell them apart. Triage nearly mis-read its own probe by counting, the seat published 43 without saying which 43, and this bullet read 63 == 63 as agreement. Three instruments, one shape, one card, one day.
      ⛔ Nothing in this PR depends on the disputed spelling: the gate's population comes from a directory walk, and this PR's own census came from git ls-tree -r --name-only.
  • The bare-word trap DID reproduce. grep -v test over the same list returns 62, silently dropping packages/cli/src/commands/test.ts, which is a command. That file is now pinned into the gate's own self-test as an examined module, so the population can never quietly lose it again.
  • packages/cli builds with tsc -p tsconfig.build.json, not tsup. The card and the ruling both say "tsup does not delete removed outputs"; the stale-artefact hazard is identical under tsc and the conclusion is unchanged, but the tool name in the card's prose does not match this package. Noted, not filed: a prose detail on a closed premise, no behaviour rides on it. Carrier: this PR.
  • The guard reds on an UNTRACKED file too — the walk reads the filesystem, not git ls-files. That is correct for this duty (oclif loads what is emitted, and the build reads the filesystem), and it means a dev sees the red before they commit rather than in CI. Noted as a property, not a finding.

Nothing was filed as a new issue; nothing here is a reproducible defect, a contract violation, or a metadata-authoring trap.

Changeset

None, and that is measured. scripts/check-cli-command-ids.mjs sits in the root package, which is private: true — no published package's files[] reaches the path. Positive control on the same instrument: packages/cli declares files: ["dist","README.md","CHANGELOG.md"] and packages/cli/dist/... matches, so the zero is a reading and not an empty query. packages/cli/package.json was not touched.

Check Changeset's own exemption is the skip-changeset label, which is applied on this PR; no changeset is invented for a package this diff does not change, and no level is raised to quiet a gate.

Authored by Claude Code, session session_01TSf4DV7ziu4V5j73e46b7c (https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c).


Generated by Claude Code

… src/commands to BE a command

The oclif command table is a glob over the emitted tree
(`"commands": { "strategy": "pattern", "target": "./dist/commands", "glob": "**/*.js" }`),
so every module under `packages/cli/src/commands/` is taken to be a command. A helper
placed beside the command it serves has no default-exported command class, and oclif
then writes a `findCommand ... not found` warning to stderr on EVERY `os` invocation,
whatever the user ran — which corrupts `os validate --json` for any consumer that reads
both streams.

`check-cli-command-ids.mjs` already walks that directory and already derives an id per
file; it asked the opposite question (does a command-id literal outside the CLI resolve
to a derivable path), and a module with no command class still yields a derivable path.
So the second duty lands in the same walk: no new traversal, no new verification surface.

- The predicate follows the INHERITANCE CHAIN, across files. `build.ts` extends `Compile`
  and `migrate/index.ts` extends `MigratePlan`; a predicate accepting only
  `extends Command` would false-red both, which is how a guard gets switched off.
- It PARSES (via `scripts/ts-parse.mjs`) rather than matching text: `create.ts`,
  `generate.ts` and `init.ts` are scaffolders whose template literals spell
  `export default ...` hundreds of lines before the module's own real class.
- It reads SOURCE, never `dist`, so it needs no build and inherits no stale artefact.
- The module population is the EMITTED set, deliberately wider than the id population: a
  dotted or non-kebab base cannot be a command id but `tsc` still emits it and the glob
  still loads it, and those are the shapes a misplaced helper takes.
- Exemptions are declarative and SELF-RETIRING — an entry reds once its file leaves the
  population or starts exporting a command class.
- Zero examined modules is a refusal, not a pass.

Duty one is unchanged: the derived id and topic sets are set-equal to the previous
implementation (74 ids, 12 topics).

Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c
Co-authored-by: Claude <noreply@anthropic.com>
@claude

claude Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Contract review — PR #17906 (card #17869)

Head reviewed: 71777c276efff766354b67a9c9878e75964fcddc. One path, +579/−13. Readings taken 2026-09-12T23:10–23:25Z against origin/* refs and the merge base fd8b2c04951e5270a599d0dd4b70baeee9f7fd77.

⚠️ Independence: the implementer is a mode:subagent dev of this same PM session; ⛔ not an arm's-length second opinion.

① Derived judgments — measured here, ⛔ not ratified from the report

1. ⭐ Duty one is unchanged BY CONSTRUCTION, and that is readable in the diff rather than taken on the report's word. ids/topics can now only be written on the deriveIds === true path, and that path is line-for-line the old code (the same ^[a-z0-9][a-z0-9-]*$ topic test, the same ids.add, the same sidecar drops). The change is that a directory which cannot name a topic used to END the traversal and is now DESCENDED with derivation switched off — which cannot add an id, and lets duty two see below a badly-named folder. ⚠️ UNEMITTED_DIR_NAMES = new Set(['__tests__']) is new (zero occurrences at the merge base, two on this head), and it is not a behaviour change for duty one either: __tests__ already failed the old topic regex, so both versions skip it. The dev's independent side-by-side measurement agrees — old and new commandSurfaceUnder set-equal at 74 ids / 12 topics, empty on both only-old and only-new.

⚠️ And a control of this seat's that must NOT be read as a contradiction. Running pnpm check:cli-command-ids here printed 73 ids derived and a 39-case self-test. That is a reading about the shared checkout's working treeea2940d1c, a different branch — because this gate reads the working tree, ⛔ not about origin/main (d88a47d7) and ⛔ not about this PR. Recorded so that 73-vs-74 is not mistaken for a defect: 「⛔ 不用共享检出的工作树核验 main」 is a lane rule and this is what it is for.

2. ⭐ The predicate PARSES, and it had to. It follows the inheritance chain across files via scripts/ts-parse.mjs rather than matching text — necessary, not ornamental: create.ts / generate.ts / init.ts are scaffolders whose template literals spell export default hundreds of lines before their real class, so a text matcher is fooled by the file's own payload. ⭐ Same species as this lane's console?.error dead control: the instrument has to understand the thing it reads, not pattern-match it.

3. The two inheritance cases are pinned BY NAME, which is what makes the guard survivable. --self-test asserts packages/cli/src/commands/build.ts accepted via Build -> Compile -> Command and packages/cli/src/commands/migrate/index.ts via Migrate -> MigratePlan -> Command, plus a case asserting both chains are longer than two hops. ⇒ a future rewrite that narrows the predicate to extends Command reds the self-test instead of false-redding two real commands — which is the outcome triage warned would 「让人把守卫关掉」.

4. ⭐ Zero examined is a REFUSAL, and the examined count is PRINTED. The success line reads 「63 module(s) … examined」 with the reason in a comment beside it: 「0 violations over 0 modules and 0 violations over 63 are the same line otherwise, and only one of them is a reading」. The self-test additionally pins the population as non-trivial (≥ 40) and pins packages/cli/src/commands/test.ts as examined — it is a command, not a test, which is the exact trap that cost this seat a wrong population count. ⇒ the check-widening-tells failure mode (exit 0 over nothing) is closed by construction.

5. The failure message carries the CONSEQUENCE, not just the rule. It quotes the oclif glob from packages/cli/package.json, states the stderr warning fires for every os invocation whatever the user ran, then 「⛔ That is not cosmetic」 with the --json corruption and PR #17859's one red in 3247 — then names packages/cli/src/utils/ with its four existing residents, and says an exemption is declared 「⛔ never a silent pass」.

6. The exemption ledger is declarative AND self-retiring on BOTH rot modes — read at source, not claimed. staleModuleExemptions() reds when no module at that path is in the walked population any more (moved, renamed, deleted) and when the module does export a command class now. Empty today, which is the tree's measured state. ⭐ The dev disclosed the one property it does not have — a wrong reason text is prose nothing can check — rather than letting the claim round up.

7. SOURCE only ⇒ the stale-dist trap is not inherited. Nothing in the new code opens dist, so the gate needs no build. ⭐ That is the right answer to the acceptance item, and it also means a dev sees the red before committing, since the walk reads the filesystem rather than git ls-files.

8. ⛔ Nothing was weakened. All thirteen removed lines are the old walk body, its floor constant and one self-test description. SELF_TEST_BATTERY_FLOOR moved 8 → 10 — a raise, so neither new battery can be deleted back into silence — and the case roster grew 39 → 57.

9. Surface axis, re-measured. One file, scripts/check-cli-command-ids.mjs, a repository gate script: no exports map names it, no barrel re-exports it. Paths under packages/: 0. Under packages/spec/: 0. Under content/docs/releases/: 0. ⇒ Clause-②: no stands, and check-clause2-carriers --pair 17906 returns exit 0.

② Changeset — NONE, and the exemption is the gate's own, verified at source

No published package changes, so there is nothing to grade. The skip-changeset label carries it, and that is a first-class documented exemption rather than a dev invention: .github/workflows/pr-automation.yml:290 spells !contains(github.event.pull_request.labels.*.name, 'skip-changeset'), and :247 calls it 「the author's explicit opt-out」. ⛔ No changeset was invented for a package the diff does not change, and ⛔ no level was raised.

⚠️ One thing to re-read immediately before the flip, and it is in the workflow's own comments (:82:89): the size labeler's PUT has STRIPPED this exact label before, after a confirmed read-back. Present at 23:22Z (['size/l','skip-changeset']). The failure mode is fail-closed — losing it makes the gate enforce rather than silently pass — so it is a red to re-clear, ⛔ never a false green.

③ Boundary flags

a. ⚠️ A latent hole, in the direction this card cares about: .test.tsx. isEmittedCommandModule rejects .test.tsx / .spec.tsx, but packages/cli/tsconfig.build.json excludes only src/**/*.test.ts, src/**/*.spec.ts, src/**/__tests__/** (and one unrelated file). ⇒ a .test.tsx placed under src/commands/ would be emitted by tsc and skipped by the guard — exactly the case the guard exists to catch. ⛔ Not live: zero .tsx files exist anywhere under packages/cli/src. Named here rather than filed, with the carrier stated: the next PR to add a .tsx to this package.

b. ⛔ A falsified claim was removed from the record before landing. The acceptance notes carried 「the ** git pathspec did not reproduce」; five probes say it does — the pathspec selects only subdirectory files and excludes all twenty top-level command modules, and a pattern matching every line does not rescue them. Corrected in place in the PR body, with the retraction kept visible, and measured on the card at 5649347543. ⭐ The lesson banked there is worth more than the trap: a count is not a reading of a set — three instruments made the same mistake on this one card in one day, and only a membership probe (「is build.ts in there?」) separates them.

c. ③ is NOT yet met. At 23:22Z: 31 names, 20 success / 10 skipped / zero failures, with Lint & Repo Gates — the job that actually runs check:cli-command-ids on the real tree, and therefore the one live execution of this change that this seat did not stage — still running. ⛔ The flip waits for it.

Independence pair

Implemented-by: claude/issue-17869-commands-dir-command-class-guard (mode:subagent)
Reviewed-by: os-sales — domain:cli execution seat, issue #6024, session_01TSf4DV7ziu4V5j73e46b7c
Independence: SELF-REVIEW — the implementer is a subagent of the reviewing seat's own session

Tier: default judgment — 「余席条款②复核 = 默认判断档自审加门禁」.

Verdict

PASS. ⛔ No carrier to clear — the claim declared the axis no and the measurement agrees. ⇒ the landing pre-check reduces to ③: every check green on this head, ⛔ not the required subset, and the skip-changeset label re-read in the same breath. Neither is satisfied yet; the flip waits.


Generated by Claude Code

@claude
claude Bot marked this pull request as ready for review September 12, 2026 23:30
@claude
claude Bot added this pull request to the merge queue Sep 12, 2026
Merged via the queue into main with commit a9c6477 Sep 12, 2026
54 checks passed
@claude
claude Bot deleted the claude/issue-17869-commands-dir-command-class-guard branch September 12, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/l skip-changeset PR has no user-facing published change; bypasses the changeset gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant