test: command hints + skill formatting coverage#430
test: command hints + skill formatting coverage#430anandgupta42 wants to merge 1 commit intomainfrom
Conversation
…rendering Command.hints() and Skill.fmt() are user-facing pure functions with zero test coverage. These 9 tests prevent regressions in command argument hint extraction and skill listing output (both verbose XML and markdown formats). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_011MEvdFUnKtEEi18HBQcJz5
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
📝 WalkthroughWalkthroughTwo new test suites were added: one for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/test/skill/fmt.test.ts (1)
19-33: Consider asserting URL conversion for both listed skills.This test validates conversion for
analyze; adding a check fordeploywould make the multi-item verbose branch coverage tighter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/skill/fmt.test.ts` around lines 19 - 33, The test currently asserts that Skill.fmt converts the analyze skill location to a file:// URL but misses asserting the same for the deploy entry; update the test that calls Skill.fmt(skills, { verbose: true }) to also assert that the output contains "file:///path/to/deploy/SKILL.md" so both skills’ paths are validated; locate the test block in packages/opencode/test/skill/fmt.test.ts (the test named "verbose mode returns XML with skill tags") and add an expect(output).toContain for the deploy file URL next to the existing analyze URL assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/command/hints.test.ts`:
- Around line 18-20: Add a test to verify that Command.hints correctly extracts
numbered placeholders along with the $ARGUMENTS token in the same template:
update the test suite in hints.test.ts to include a case calling Command.hints
with a string containing mixed numbered placeholders and $ARGUMENTS (e.g., "$1
$2 $ARGUMENTS" or mixed-case variants) and assert the returned array preserves
the expected ordering/append contract (contains "$1", "$2", and "$ARGUMENTS" in
the correct order). This ensures Command.hints handles combined placeholders and
the special $ARGUMENTS token together.
---
Nitpick comments:
In `@packages/opencode/test/skill/fmt.test.ts`:
- Around line 19-33: The test currently asserts that Skill.fmt converts the
analyze skill location to a file:// URL but misses asserting the same for the
deploy entry; update the test that calls Skill.fmt(skills, { verbose: true }) to
also assert that the output contains "file:///path/to/deploy/SKILL.md" so both
skills’ paths are validated; locate the test block in
packages/opencode/test/skill/fmt.test.ts (the test named "verbose mode returns
XML with skill tags") and add an expect(output).toContain for the deploy file
URL next to the existing analyze URL assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67d9eb11-bbc5-470a-b3b6-b43819c5d92b
📒 Files selected for processing (2)
packages/opencode/test/command/hints.test.tspackages/opencode/test/skill/fmt.test.ts
| test("appends $ARGUMENTS when present", () => { | ||
| expect(Command.hints("Do something with $ARGUMENTS")).toEqual(["$ARGUMENTS"]) | ||
| }) |
There was a problem hiding this comment.
Add a mixed-case assertion for numbered placeholders + $ARGUMENTS.
Line 18 currently verifies $ARGUMENTS alone, but it does not assert combined behavior in the same template (e.g., $1, $2, and $ARGUMENTS together), which is the higher-value ordering/append contract.
Suggested test addition
test("appends $ARGUMENTS when present", () => {
expect(Command.hints("Do something with $ARGUMENTS")).toEqual(["$ARGUMENTS"])
+ expect(Command.hints("Use $2 with $ARGUMENTS and $1")).toEqual(["$1", "$2", "$ARGUMENTS"])
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("appends $ARGUMENTS when present", () => { | |
| expect(Command.hints("Do something with $ARGUMENTS")).toEqual(["$ARGUMENTS"]) | |
| }) | |
| test("appends $ARGUMENTS when present", () => { | |
| expect(Command.hints("Do something with $ARGUMENTS")).toEqual(["$ARGUMENTS"]) | |
| expect(Command.hints("Use $2 with $ARGUMENTS and $1")).toEqual(["$1", "$2", "$ARGUMENTS"]) | |
| }) |
🤖 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 18 - 20, Add a
test to verify that Command.hints correctly extracts numbered placeholders along
with the $ARGUMENTS token in the same template: update the test suite in
hints.test.ts to include a case calling Command.hints with a string containing
mixed numbered placeholders and $ARGUMENTS (e.g., "$1 $2 $ARGUMENTS" or
mixed-case variants) and assert the returned array preserves the expected
ordering/append contract (contains "$1", "$2", and "$ARGUMENTS" in the correct
order). This ensures Command.hints handles combined placeholders and the special
$ARGUMENTS token together.
|
Superseded by #439 which consolidates all 12 test PRs into one, deduplicates overlapping tests, and fixes bugs found during review. |
2 participants
What does this PR do?
Adds 9 new tests across 2 previously untested pure functions that power user-facing UI elements.
1.
Command.hints()—src/command/index.ts(5 new tests)This function extracts placeholder tokens (
$1,$2,$ARGUMENTS) from command templates and feeds them to the TUI command dialog so users know what arguments to provide. Zero tests existed. New coverage includes:Set$ARGUMENTSis appended independently of numbered placeholders$10) sort lexicographically ($1, $10, $2) — documents the actual behavior so future refactors don't assume numeric ordering2.
Skill.fmt()—src/skill/skill.ts(4 new tests)This function formats the skill listing shown in both the TUI skill browser and the system prompt injected into LLM context. Zero tests existed. New coverage includes:
<available_skills>,<skill>,<name>,<description>,<location>tags## Available Skillsheader and**name**: descriptionbulletsbuiltin:protocol locations are preserved as-is (nofile://conversion) — this is an altimate-specific change that protects embedded skill paths from being mangled bypathToFileURL()Type of change
Issue for this PR
N/A — proactive test coverage from test-discovery rotation (Week 1: skills + tools)
How did you verify your code works?
Test quality review
Tests were reviewed by a critic agent before committing:
Skill.available()permission filtering was too integration-heavy for a unit test run and delegates to already-testedPermissionNext.evaluate()Checklist
https://claude.ai/code/session_011MEvdFUnKtEEi18HBQcJz5
Summary by CodeRabbit