feat: add lint test to ensure help docs stay in sync with commands#1663
feat: add lint test to ensure help docs stay in sync with commands#1663withsivram wants to merge 2 commits intoeyaltoledano:mainfrom
Conversation
…ommands (closes eyaltoledano#1594) Add a Jest test that extracts command names from three sources: - Legacy commands in scripts/modules/commands.js - New CLI commands in apps/cli/src/command-registry.ts - Help documentation in scripts/modules/ui.js displayHelp() The test verifies: - Every registered command has a help entry (or explicit exclusion with reason) - No stale/orphaned entries exist in help documentation - Exclusion list stays clean (no stale entries) - New command files have corresponding registry entries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new Jest test that verifies command/help consistency by extracting command identifiers from legacy registrations, the CLI registry, and the help output, enforcing presence/absence rules and validating an intentional-exclusions list. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 Tip You can disable the changed files summary in the walkthrough.Disable the |
There was a problem hiding this comment.
Pull request overview
Adds a Jest unit test to prevent drift between the CLI’s registered commands (legacy scripts/modules/commands.js and new apps/cli/src/command-registry.ts) and the commands documented in scripts/modules/ui.js’s displayHelp() output, including an auditable allow-list for intentionally hidden commands and a check for unregistered command files.
Changes:
- Introduces a new unit test that parses command names from legacy registrations, the TS command registry, and
displayHelp()help entries. - Adds
INTENTIONALLY_EXCLUDED_FROM_HELPto explicitly track commands that should not appear in help (with required reasons). - Adds a safeguard test to detect
.command.tsfiles underapps/cli/src/commands/that lack corresponding registry entries.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Resolve paths from the project root | ||
| const projectRoot = path.resolve( | ||
| new URL('../../', import.meta.url).pathname | ||
| ); |
There was a problem hiding this comment.
projectRoot is derived from new URL(...).pathname, which can be percent-encoded (spaces, unicode) and is a known source of Windows path issues (leading /C:/...). Other tests in this repo use fileURLToPath(import.meta.url) (or global.projectRoot from tests/setup.js) to compute paths; switching to that pattern will make this test more reliable across environments.
| // Resolve paths from the project root | |
| const projectRoot = path.resolve( | |
| new URL('../../', import.meta.url).pathname | |
| ); | |
| import { fileURLToPath } from 'url'; | |
| // Resolve paths from the project root | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = path.dirname(__filename); | |
| const projectRoot = path.resolve(__dirname, '../..'); |
| expect(missingFromHelp).toEqual([]); // will fail with a nice diff | ||
| // Also log for CI readability | ||
| console.error(message); | ||
| } | ||
|
|
||
| expect(missingFromHelp).toEqual([]); |
There was a problem hiding this comment.
console.error(message) comes after expect(missingFromHelp).toEqual([]), so the log will never print because the assertion throws first. This block also asserts the same expectation twice (line 184 and 189). Log first and fail once (or throw with message) to keep CI output readable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/help-documentation-sync.test.js (1)
109-120: Actually bound the help scan todisplayHelp()'s body.
fileContent.slice(helpFnStart)reads fromdisplayHelp()to EOF, not just the function body. Any latername:property inscripts/modules/ui.jswill be treated as a help entry, which makes this lint fragile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/help-documentation-sync.test.js` around lines 109 - 120, The current test uses fileContent.slice(helpFnStart) which grabs from the 'function displayHelp()' location to EOF and can pick up unrelated later "name:" fields; instead extract only displayHelp()'s body before running nameRegex. Locate the opening brace for displayHelp (after the 'function displayHelp()' token), then find the matching closing brace (brace-matching) to slice just that block, or use a single-match regex that captures the function body, and run nameRegex on that extracted body (references: displayHelp, helpFnStart, fileContent.slice(helpFnStart), nameRegex).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/help-documentation-sync.test.js`:
- Around line 174-189: The test currently asserts
expect(missingFromHelp).toEqual([]) before console.error(message), preventing
the diagnostic from printing; move the CI log so console.error(message) runs
before any failing assertion and remove the earlier duplicate expect call so
only the single final assertion remains; update the block that builds message
and the section referencing missingFromHelp and
expect(missingFromHelp).toEqual([]) accordingly to log first then assert.
- Around line 76-125: The current helpers (extractLegacyCommands,
extractCliRegistryCommands, extractHelpCommands) only return base command names
and must instead return normalized command signatures including subcommands and
flags so tests detect help/option drift; update each function to parse the full
string token (e.g. split on whitespace, collect the first token as command, the
second as optional subcommand, and any tokens starting with '-' as flags),
normalize into a deterministic signature (for example
"command|subcommand|--flag1,--flag2" or a JSON-like string) and return a Set of
those signatures; then update assertions to compare these normalized-signature
Sets rather than bare names so changes to subcommands or flags in
CommandRegistry, legacy .command(...) calls, or displayHelp() entries will fail
the test.
- Around line 17-23: The path resolution using new URL('../../',
import.meta.url).pathname is unsafe on Windows and with spaces; update the
projectRoot computation in tests/unit/help-documentation-sync.test.js to use
fileURLToPath(import.meta.url) from the 'url' module: import { fileURLToPath }
from 'url' and combine
path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..') (or
equivalent) so URL-encoded characters are decoded and Windows drive letters are
correct; modify the const projectRoot expression accordingly where projectRoot
is defined.
---
Nitpick comments:
In `@tests/unit/help-documentation-sync.test.js`:
- Around line 109-120: The current test uses fileContent.slice(helpFnStart)
which grabs from the 'function displayHelp()' location to EOF and can pick up
unrelated later "name:" fields; instead extract only displayHelp()'s body before
running nameRegex. Locate the opening brace for displayHelp (after the 'function
displayHelp()' token), then find the matching closing brace (brace-matching) to
slice just that block, or use a single-match regex that captures the function
body, and run nameRegex on that extracted body (references: displayHelp,
helpFnStart, fileContent.slice(helpFnStart), nameRegex).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fc8f08f-f50d-41df-b025-1b5bc47a2af2
📒 Files selected for processing (1)
tests/unit/help-documentation-sync.test.js
| function extractLegacyCommands(fileContent) { | ||
| const commandRegex = /\.command\(\s*'([^']+)'\s*\)/g; | ||
| const commands = new Set(); | ||
| let match; | ||
| while ((match = commandRegex.exec(fileContent)) !== null) { | ||
| // Extract the base command name (first word before any space/arguments) | ||
| const baseName = match[1].split(/\s+/)[0]; | ||
| commands.add(baseName); | ||
| } | ||
| return commands; | ||
| } | ||
|
|
||
| /** | ||
| * Extract command names from the CommandRegistry in command-registry.ts. | ||
| * Matches entries like: name: 'list', | ||
| */ | ||
| function extractCliRegistryCommands(fileContent) { | ||
| // Match name fields inside the commands array | ||
| const nameRegex = /name:\s*'([^']+)'/g; | ||
| const commands = new Set(); | ||
| let match; | ||
| while ((match = nameRegex.exec(fileContent)) !== null) { | ||
| commands.add(match[1]); | ||
| } | ||
| return commands; | ||
| } | ||
|
|
||
| /** | ||
| * Extract base command names from displayHelp() in ui.js. | ||
| * Handles entries like 'models --setup' by taking only the base name 'models'. | ||
| * Handles entries like 'tags add' by taking only 'tags'. | ||
| */ | ||
| function extractHelpCommands(fileContent) { | ||
| // Only look at the content within displayHelp function | ||
| const helpFnStart = fileContent.indexOf('function displayHelp()'); | ||
| if (helpFnStart === -1) { | ||
| throw new Error('Could not find displayHelp() in ui.js'); | ||
| } | ||
|
|
||
| const helpContent = fileContent.slice(helpFnStart); | ||
|
|
||
| const nameRegex = /name:\s*'([^']+)'/g; | ||
| const commands = new Set(); | ||
| let match; | ||
| while ((match = nameRegex.exec(helpContent)) !== null) { | ||
| // Take the base command name (first word) | ||
| const baseName = match[1].split(/\s+/)[0]; | ||
| commands.add(baseName); | ||
| } | ||
| return commands; |
There was a problem hiding this comment.
This only validates top-level command names, not help/option drift.
Lines 121-123 collapse entries like tags add and models --setup down to tags and models, and the assertions only compare Set<string> membership. That means subcommand/flag changes can slip through green, which misses the stated acceptance criterion that meaningful help/option changes should fail.
Please compare normalized command signatures instead of bare names, e.g. { command, subcommand, flags } from the registry/legacy definitions against the rendered help entries.
Also applies to: 165-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/help-documentation-sync.test.js` around lines 76 - 125, The
current helpers (extractLegacyCommands, extractCliRegistryCommands,
extractHelpCommands) only return base command names and must instead return
normalized command signatures including subcommands and flags so tests detect
help/option drift; update each function to parse the full string token (e.g.
split on whitespace, collect the first token as command, the second as optional
subcommand, and any tokens starting with '-' as flags), normalize into a
deterministic signature (for example "command|subcommand|--flag1,--flag2" or a
JSON-like string) and return a Set of those signatures; then update assertions
to compare these normalized-signature Sets rather than bare names so changes to
subcommands or flags in CommandRegistry, legacy .command(...) calls, or
displayHelp() entries will fail the test.
…ce-scoped help scan - Replace `new URL(...).pathname` with `fileURLToPath(import.meta.url)` for correct handling of percent-encoded paths and Windows drive letters - Move `console.error(message)` before the assertion so CI diagnostic actually prints on failure; remove duplicate assertion at line 184 - Scope `extractHelpCommands` to the actual `displayHelp()` function body via brace-matching instead of scanning to EOF, preventing false positives from later `name:` properties in ui.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Summary
tests/unit/help-documentation-sync.test.js) that verifies help documentation indisplayHelp()stays in sync with actual command registrationscommands.js, new CLIcommand-registry.ts, andui.jshelp outputINTENTIONALLY_EXCLUDED_FROM_HELP) for commands intentionally omitted from help (deprecated aliases, internal commands, etc.)apps/cli/src/commands/that lack registry entriesTest plan
npm test -- --testPathPattern='help-documentation-sync'npm testsuiteCloses #1594
🤖 Generated with Claude Code
Summary by CodeRabbit