test: enhance help sync test to verify subcommand documentation#1664
test: enhance help sync test to verify subcommand documentation#1664withsivram wants to merge 2 commits intoeyaltoledano:mainfrom
Conversation
eyaltoledano#1596) Add comprehensive test that verifies not just command existence in help documentation, but also subcommand and option documentation accuracy: - Tags subcommands: verifies all subcommands from tags.command.ts are documented, deprecated standalone commands are not shown, and the list default action is properly documented with its options - List command options: verifies filtering options (--ready, --blocking, --with-subtasks), format options (--json, -f, -c), watch mode (-w), and cross-tag listing (--all-tags) are all documented - General coverage: bidirectional checks ensure no phantom subcommands exist in help and no implemented subcommands are missing from help Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughAdds a Vitest unit test that verifies CLI implementations (legacy and modern) are synchronized with help text from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new Vitest unit test to keep the CLI help documentation (scripts/modules/ui.js) synchronized with the actual CLI command implementations (legacy scripts/modules/commands.js and modern apps/cli/src/commands/*), including checks for tags subcommands and key list options.
Changes:
- Introduces
apps/cli/tests/unit/help-documentation-sync.spec.tswith command-level help/CLI sync checks. - Adds subcommand-level verification for
tags(including default-action handling forlist). - Adds option-level verification for
listhelp documentation against the TypeScript implementation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function extractCommandsFromModernTs(): string[] { | ||
| const commands = new Set<string>(); | ||
|
|
||
| try { | ||
| const files = readdirSync(MODERN_COMMANDS_DIR); | ||
|
|
||
| for (const file of files) { | ||
| if (!file.endsWith('.command.ts') || file.includes('.spec.')) continue; | ||
|
|
||
| const filePath = resolve(MODERN_COMMANDS_DIR, file); | ||
| const content = readFileSync(filePath, 'utf-8'); | ||
|
|
||
| // Match super(name || 'command-name') or super('command-name') patterns | ||
| const superRegex = /super\((?:name \|\| )?['"]([^'"]+)['"]\)/g; | ||
| let match; | ||
| while ((match = superRegex.exec(content)) !== null) { | ||
| commands.add(match[1]); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Directory might not exist in some configurations | ||
| console.warn('Could not read modern commands directory:', error); | ||
| } | ||
|
|
||
| return Array.from(commands).sort(); | ||
| } |
There was a problem hiding this comment.
extractCommandsFromModernTs() only scans top-level apps/cli/src/commands/*.command.ts files and therefore misses commands that are implemented/registered elsewhere (e.g. autopilot is defined in apps/cli/src/commands/autopilot/index.ts and included in apps/cli/src/command-registry.ts). This creates a coverage gap where the command-level sync tests can pass even when help is missing commands that exist in the CLI. Consider sourcing modern commands from the command registry (or recursively scanning subdirectories / additional filename patterns) so the test reflects the actual registration surface.
| const COMMAND_NAME_MAPPINGS: Record<string, string> = { | ||
| // Tags subcommands in help map to legacy CLI commands | ||
| tags: 'tags', // tags list | ||
| // The following are legacy commands being deprecated | ||
| 'add-tag': 'add-tag', | ||
| 'use-tag': 'use-tag', | ||
| 'delete-tag': 'delete-tag', | ||
| 'rename-tag': 'rename-tag', | ||
| 'copy-tag': 'copy-tag' |
There was a problem hiding this comment.
COMMAND_NAME_MAPPINGS currently includes tags: 'tags', which makes the command-level sync checks ignore the tags command entirely (it is filtered out via Object.values(COMMAND_NAME_MAPPINGS) / Object.keys(COMMAND_NAME_MAPPINGS)). That means if tags is accidentally removed from help (or removed from the CLI), these tests won’t catch it. Suggest removing the identity mapping for tags (or changing the filter logic to only exempt mappings where help name differs from CLI name).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/cli/tests/unit/help-documentation-sync.spec.ts (1)
78-81: Drop rawconsole.*diagnostics from this suite.The assertion messages already carry the useful failure context, so these branches mostly add noisy stdout/stderr. Please route them through the shared logger or remove them.
As per coding guidelines, "Do not add direct console.log calls outside the logging utility - use the central log function instead".
Also applies to: 265-272, 294-301, 383-389, 415-425, 453-455, 466-470, 544-548, 580-581, 682-688, 755-763
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/tests/unit/help-documentation-sync.spec.ts` around lines 78 - 81, Replace raw console.* calls in the test file with the project's shared logging utility (or remove them) — specifically change the catch block that currently does console.warn('Could not read modern commands directory:', error) to use the central logger (e.g., logger.warn('Could not read modern commands directory', error) or simply drop the message), and do the same for all other console.log/console.warn/console.error occurrences noted (lines/groups 265-272, 294-301, 383-389, 415-425, 453-455, 466-470, 544-548, 580-581, 682-688, 755-763) so tests no longer emit raw stdout/stderr but route through the shared log API or remove the noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/tests/unit/help-documentation-sync.spec.ts`:
- Around line 366-380: The test currently checks implemented subcommands by
substring-matching helpContent (from getHelpContent) which allows false
positives; update the assertions to parse exact help-entry names from the CLI
help output and compare those names to implementedSubcommands instead of using
helpContent.includes(...). Reuse the existing name-extraction logic used
elsewhere in this spec (the "name:" parsing helper) to produce a list like
parsedHelpNames, then compute missingSubcommands =
implementedSubcommands.filter(subcmd =>
!TAGS_DEFAULT_ACTION_SUBCOMMANDS.includes(subcmd) &&
!parsedHelpNames.includes(`tags ${subcmd}`)); apply the same change to the other
blocks noted (lines ~398-412, 434-463, 714-725) so all tests use exact
help-entry name comparison rather than substring matching.
- Around line 171-193: The test's option extraction in
extractOptionsFromModernTs() silently returns an empty array on read/parse
failure which makes assertions vacuously pass; update the try/catch and
post-parse check so that if options.length === 0 (either because the file
couldn't be read or no flags were parsed) the function throws an Error (or the
test fails) with a clear message including the target command file name (e.g.,
list.command.ts) and the symbol extractOptionsFromModernTs, so tests fail fast
and surface parsing/read problems instead of returning [].
---
Nitpick comments:
In `@apps/cli/tests/unit/help-documentation-sync.spec.ts`:
- Around line 78-81: Replace raw console.* calls in the test file with the
project's shared logging utility (or remove them) — specifically change the
catch block that currently does console.warn('Could not read modern commands
directory:', error) to use the central logger (e.g., logger.warn('Could not read
modern commands directory', error) or simply drop the message), and do the same
for all other console.log/console.warn/console.error occurrences noted
(lines/groups 265-272, 294-301, 383-389, 415-425, 453-455, 466-470, 544-548,
580-581, 682-688, 755-763) so tests no longer emit raw stdout/stderr but route
through the shared log API or remove the noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96fa9218-44b9-474e-8d4a-ece510c5ce8f
📒 Files selected for processing (1)
apps/cli/tests/unit/help-documentation-sync.spec.ts
…se matching - Scan subdirectory index.ts files (e.g. autopilot/index.ts) for top-level commands that were previously missed by flat readdirSync - Remove identity mapping (tags: 'tags') from COMMAND_NAME_MAPPINGS so the tags command is properly validated in both sync directions - Add extractHelpEntryNames() for precise name-based matching instead of substring matching against raw ui.js content (CodeRabbit line 380) - Make extractOptionsFromModernTs() fail-fast on read errors and add sanity-check assertion for extracted options (CodeRabbit line 193) - Remove raw console.log/console.warn calls — assertion messages carry all needed context (CodeRabbit nitpick) - Add autopilot to INTENTIONALLY_UNDOCUMENTED (cloud/development command not yet in legacy help) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/cli/tests/unit/help-documentation-sync.spec.ts (2)
283-290: Consider using a dedicated deprecated commands list for clarity.The identity mappings (
'add-tag': 'add-tag', etc.) work but are semantically awkward. A dedicatedDEPRECATED_COMMANDSarray would more clearly express the intent and simplify the filtering logic.♻️ Optional refactor for clarity
+/** + * Legacy commands being deprecated in favor of 'tags' subcommands. + * Exempted from help validation during migration. + */ +const DEPRECATED_COMMANDS = [ + 'add-tag', + 'use-tag', + 'delete-tag', + 'rename-tag', + 'copy-tag' +]; + -/** - * Commands documented in help that map to different CLI command names. - * ... - */ -const COMMAND_NAME_MAPPINGS: Record<string, string> = { - // The following are legacy commands being deprecated - 'add-tag': 'add-tag', - 'use-tag': 'use-tag', - 'delete-tag': 'delete-tag', - 'rename-tag': 'rename-tag', - 'copy-tag': 'copy-tag' -}; +const COMMAND_NAME_MAPPINGS: Record<string, string> = {};Then update the filters:
const missingFromHelp = cliCommands.filter( (cmd) => !helpCommands.includes(cmd) && !INTENTIONALLY_UNDOCUMENTED.includes(cmd) && - !Object.values(COMMAND_NAME_MAPPINGS).includes(cmd) + !DEPRECATED_COMMANDS.includes(cmd) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/tests/unit/help-documentation-sync.spec.ts` around lines 283 - 290, Replace the identity entries in COMMAND_NAME_MAPPINGS with a dedicated DEPRECATED_COMMANDS array (e.g., const DEPRECATED_COMMANDS = ['add-tag','use-tag','delete-tag','rename-tag','copy-tag']) and update any filtering logic that currently checks COMMAND_NAME_MAPPINGS keys to instead check membership in DEPRECATED_COMMANDS; keep COMMAND_NAME_MAPPINGS for real mappings only and modify tests in help-documentation-sync.spec.ts to reference DEPRECATED_COMMANDS where the intent is to exclude or treat those commands as deprecated.
515-630: Consider extracting the repeatedlistMatchesextraction into a helper.The pattern
helpContent.match(/name:\s*['"]list['"][^}]+}/g)with the subsequent null check is repeated 5 times across the list command tests. This could be extracted into a helper function to reduce duplication.♻️ Optional DRY improvement
/** * Extract all list command sections from help content. * Throws if no list command entries are found. */ function getListCommandSections(helpContent: string): string { const listMatches = helpContent.match(/name:\s*['"]list['"][^}]+}/g); if (!listMatches || listMatches.length === 0) { throw new Error('Could not find list command section in help'); } return listMatches.join('\n'); }Then each test can simply call:
const allListSections = getListCommandSections(helpContent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/tests/unit/help-documentation-sync.spec.ts` around lines 515 - 630, Extract the repeated logic that runs helpContent.match(/name:\s*['"]list['"][^}]+}/g) and the null/length check into a small helper (e.g. getListCommandSections(helpContent)) that throws the existing 'Could not find list command section in help' error when nothing is found and returns the joined string of matches; then replace the repeated blocks using listMatches and allListSections in the tests (those that call getHelpContent(), the tests checking filter/format/watch/--all-tags and the one using extractOptionsFromModernTs) with a single call const allListSections = getListCommandSections(helpContent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/cli/tests/unit/help-documentation-sync.spec.ts`:
- Around line 283-290: Replace the identity entries in COMMAND_NAME_MAPPINGS
with a dedicated DEPRECATED_COMMANDS array (e.g., const DEPRECATED_COMMANDS =
['add-tag','use-tag','delete-tag','rename-tag','copy-tag']) and update any
filtering logic that currently checks COMMAND_NAME_MAPPINGS keys to instead
check membership in DEPRECATED_COMMANDS; keep COMMAND_NAME_MAPPINGS for real
mappings only and modify tests in help-documentation-sync.spec.ts to reference
DEPRECATED_COMMANDS where the intent is to exclude or treat those commands as
deprecated.
- Around line 515-630: Extract the repeated logic that runs
helpContent.match(/name:\s*['"]list['"][^}]+}/g) and the null/length check into
a small helper (e.g. getListCommandSections(helpContent)) that throws the
existing 'Could not find list command section in help' error when nothing is
found and returns the joined string of matches; then replace the repeated blocks
using listMatches and allListSections in the tests (those that call
getHelpContent(), the tests checking filter/format/watch/--all-tags and the one
using extractOptionsFromModernTs) with a single call const allListSections =
getListCommandSections(helpContent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecaddfb6-335e-4a9f-bff3-a5f0943fa50b
📒 Files selected for processing (1)
apps/cli/tests/unit/help-documentation-sync.spec.ts
Summary
apps/cli/tests/unit/help-documentation-sync.spec.ts) that verifies help documentation stays in sync with CLI commands at both the command and subcommand leveltagssubcommands (add, use, remove, rename, copy, list) are correctly documented with the unified subcommand structure, not deprecated standalone commandslistcommand options (--ready, --blocking, --with-subtasks, --json, -w, --all-tags, etc.) are documented in helpTest plan
tags.command.tsand verify against help inui.jslist.command.tsand verify against help inui.jsCloses #1596
🤖 Generated with Claude Code
Summary by CodeRabbit