test: command — hints extraction and altimate builtin registration#431
test: command — hints extraction and altimate builtin registration#431anandgupta42 wants to merge 1 commit intomainfrom
Conversation
Command.hints() parses template placeholders for TUI argument hints; zero tests existed. The 3 altimate-specific builtin commands (discover-and-add-mcps, configure-claude, configure-codex) shipped in recent PRs with no registration tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_0135nT8sg3re3D2Zgvo6ipc2
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThese changes add test coverage for command functionality, including validation of default command registration, metadata properties, and template placeholder extraction logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
packages/opencode/test/command/command-resilience.test.ts (1)
142-146: Add an explicit existence assertion before dereferencingcmd.template.Line 145 reads
cmd.templatedirectly; addingexpect(cmd).toBeDefined()in this test gives a clearer failure mode if registration breaks.✅ Small robustness tweak
test("discover-and-add-mcps template references --scope for scope selection", async () => { await withInstance(async () => { const cmd = await Command.get("discover-and-add-mcps") + expect(cmd).toBeDefined() const template = await cmd.template expect(template).toContain("--scope") expect(template).toContain("mcp_discover") }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/command/command-resilience.test.ts` around lines 142 - 146, In the "discover-and-add-mcps template references --scope for scope selection" test, assert the Command.get result exists before accessing its template: after obtaining cmd via Command.get("discover-and-add-mcps") add an existence assertion (e.g., expect(cmd).toBeDefined()) so that dereferencing cmd.template is protected and yields a clear failure if Command registration fails; locate the test and insert this assertion before the line that reads const template = await cmd.template.packages/opencode/test/command/hints.test.ts (1)
5-7: Add a multi-digit placeholder ordering test to lock expected sort semantics.Current cases only use single-digit placeholders. Add one case like
$10+$2so ordering behavior is explicit and protected against regressions.💡 Proposed test addition
describe("Command.hints: template placeholder extraction", () => { @@ test("handles non-sequential numbering", () => { expect(Command.hints("$3 then $7")).toEqual(["$3", "$7"]) }) + + test("handles multi-digit numbered placeholders with numeric order", () => { + expect(Command.hints("$10 then $2 then $1")).toEqual(["$1", "$2", "$10"]) + }) })Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/command/hints.test.ts` around lines 5 - 7, Add a test to lock down sorting semantics for multi-digit placeholders: in the hints.test.ts suite add a case that includes both multi-digit and single-digit placeholders (e.g., call Command.hints("Do $10 then $2 and $1") and assert it returns ["$1","$2","$10"]) so ordering is numeric rather than lexicographic; also update the similar test group that mirrors the single-digit case to include a corresponding multi-digit example to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/test/command/command-resilience.test.ts`:
- Around line 142-146: In the "discover-and-add-mcps template references --scope
for scope selection" test, assert the Command.get result exists before accessing
its template: after obtaining cmd via Command.get("discover-and-add-mcps") add
an existence assertion (e.g., expect(cmd).toBeDefined()) so that dereferencing
cmd.template is protected and yields a clear failure if Command registration
fails; locate the test and insert this assertion before the line that reads
const template = await cmd.template.
In `@packages/opencode/test/command/hints.test.ts`:
- Around line 5-7: Add a test to lock down sorting semantics for multi-digit
placeholders: in the hints.test.ts suite add a case that includes both
multi-digit and single-digit placeholders (e.g., call Command.hints("Do $10 then
$2 and $1") and assert it returns ["$1","$2","$10"]) so ordering is numeric
rather than lexicographic; also update the similar test group that mirrors the
single-digit case to include a corresponding multi-digit example to prevent
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebfc7a34-a71e-4e7a-af8f-431c199cef45
📒 Files selected for processing (2)
packages/opencode/test/command/command-resilience.test.tspackages/opencode/test/command/hints.test.ts
|
Superseded by #439 which consolidates all 12 test PRs into one, deduplicates overlapping tests, and fixes bugs found during review. |
What does this PR do?
1.
Command.hints()—src/command/index.ts:53-61(7 new tests)This pure function parses template strings to extract placeholder variables (
$1,$2,$ARGUMENTS) that determine what argument hints the TUI shows for each command. Zero tests existed. Every command definition relies on this function — if it breaks, users see no argument hints when invoking/commands.New coverage includes:
$1,$2,$3)$ARGUMENTSextraction$ARGUMENTSin same template$3,$7)2. Altimate-specific builtin commands —
src/command/index.ts(6 new tests)Three altimate-specific commands (
discover-and-add-mcps,configure-claude,configure-codex) were shipped in recent PRs (#409 for discover-and-add-mcps) but had zero registration tests. The TUI toast directs users to run/discover-and-add-mcps— if this command isn't registered properly, users hit "command not found" after being told to run it.New coverage includes:
discover-and-add-mcpsregistration with correct name, source, and descriptionconfigure-clauderegistration with correct metadataconfigure-codexregistration with correct metadatadiscover-and-add-mcpstemplate content pins--scopeandmcp_discoverreferencesType of change
Issue for this PR
N/A — proactive test coverage
How did you verify your code works?
Note: The
command-resilience.test.tstests that usetmpdir({ git: true })requirecommit.gpgsign=falseto run in CI environments with mandatory commit signing. This is a pre-existing infrastructure issue affecting all tests in this file, not introduced by this PR.Checklist
https://claude.ai/code/session_0135nT8sg3re3D2Zgvo6ipc2
Summary by CodeRabbit