test: command builtins + sql_analyze formatting — 16 new tests#441
Open
anandgupta42 wants to merge 1 commit intomainfrom
Open
test: command builtins + sql_analyze formatting — 16 new tests#441anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
Cover altimate-specific builtin commands (discover-and-add-mcps, configure-claude, configure-codex) and Command.hints() parsing that had zero test coverage. Also cover SqlAnalyzeTool title construction and formatAnalysis output formatting to prevent regression of the success-flag telemetry fix (38bfb52). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 files added to validate SQL analysis tool output formatting and builtin command registration with metadata and hints parsing. Tests use Bun framework with helper functions to replicate production logic and assert expected behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (3)
packages/opencode/test/altimate/sql-analyze-tool.test.ts (2)
98-117: Use one exact multiline assertion to lock formatting contract.
toContainchecks are useful, but one exact assertion here would better protect spacing/blank-line/order regressions.Suggested test tightening
test("issues are formatted with severity, type, location", () => { const output = formatAnalysis({ issues: [ { type: "lint", severity: "warning", message: "SELECT * detected", recommendation: "Use explicit columns", location: "line 1", confidence: "high", }, ], issue_count: 1, confidence: "high", confidence_factors: ["lint"], }) - expect(output).toContain("[WARNING] lint") - expect(output).toContain("SELECT * detected \u2014 line 1") - expect(output).toContain("\u2192 Use explicit columns") + expect(output.trimEnd()).toBe( + [ + "Found 1 issue (confidence: high):", + " Note: lint", + "", + " [WARNING] lint", + " SELECT * detected — line 1", + " → Use explicit columns", + ].join("\n"), + ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/sql-analyze-tool.test.ts` around lines 98 - 117, The test "issues are formatted with severity, type, location" uses multiple toContain checks which are fragile; replace them with a single exact multiline assertion against the output of formatAnalysis to lock the formatting contract. Locate the test and build an expected string (including the “[WARNING] lint” severity/type line, the "SELECT * detected — line 1" message/emdash/space, and the "→ Use explicit columns" recommendation with exact newlines and spacing) then assert equality (e.g., expect(output).toBe(expectedString)) so spacing, blank lines and order are strictly enforced for formatAnalysis.
5-8: Reduce mirror-logic drift by adding one realSqlAnalyzeTool.execute()formatting test.Line 5–8 already calls out the tradeoff; adding a single integration-style assertion against the real tool output would catch copy drift while keeping these fast unit-style checks.
Also applies to: 39-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/sql-analyze-tool.test.ts` around lines 5 - 8, Add one integration-style assertion that invokes the real SqlAnalyzeTool.execute() and verifies its formatted output matches the expected analysis to catch mirror-logic drift; locate tests in sql-analyze-tool.test.ts that currently replicate formatAnalysis logic and add a single test that constructs a SqlAnalyzeTool instance, calls its execute() with the same sample input used by the copied formatter tests, and asserts the returned/formatted result equals the expected string (keeping the existing unit-style replicated checks intact).packages/opencode/test/command/builtin-commands.test.ts (1)
6-9: AddafterEachcleanup to dispose cached instances after each test.
Instance.providecaches contexts by directory path. WhenwithInstancecreates temporary directories, each call adds an entry to the cache that persists after the temporary directory is deleted, accumulating stale entries during test execution. Other test files (e.g.,packages/opencode/test/util/instance-state.test.ts) demonstrate the proper cleanup pattern withafterEach(async () => { await Instance.disposeAll() }). Apply the same pattern here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/command/builtin-commands.test.ts` around lines 6 - 9, Tests using withInstance call Instance.provide which caches per-directory contexts; add an afterEach teardown that calls Instance.disposeAll() to clear cached instances between tests so temporary directories don't leave stale cache entries — specifically, in the same test file that defines withInstance (and uses tmpdir), add an afterEach(async () => { await Instance.disposeAll() }) cleanup block to mirror the pattern used in packages/opencode/test/util/instance-state.test.ts.
🤖 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/builtin-commands.test.ts`:
- Around line 111-115: The test "sorts numbered placeholders lexicographically"
currently calls Command.hints with only single-digit placeholders and thus won't
catch multi-digit ordering bugs; update the test input passed to Command.hints
to include a multi-digit placeholder (e.g. "$10" alongside "$1", "$2", "$3") and
change the expected array to reflect lexicographic sorting (so "$10" should come
before "$2"), ensuring the assertion verifies the multi-digit edge case.
---
Nitpick comments:
In `@packages/opencode/test/altimate/sql-analyze-tool.test.ts`:
- Around line 98-117: The test "issues are formatted with severity, type,
location" uses multiple toContain checks which are fragile; replace them with a
single exact multiline assertion against the output of formatAnalysis to lock
the formatting contract. Locate the test and build an expected string (including
the “[WARNING] lint” severity/type line, the "SELECT * detected — line 1"
message/emdash/space, and the "→ Use explicit columns" recommendation with exact
newlines and spacing) then assert equality (e.g.,
expect(output).toBe(expectedString)) so spacing, blank lines and order are
strictly enforced for formatAnalysis.
- Around line 5-8: Add one integration-style assertion that invokes the real
SqlAnalyzeTool.execute() and verifies its formatted output matches the expected
analysis to catch mirror-logic drift; locate tests in sql-analyze-tool.test.ts
that currently replicate formatAnalysis logic and add a single test that
constructs a SqlAnalyzeTool instance, calls its execute() with the same sample
input used by the copied formatter tests, and asserts the returned/formatted
result equals the expected string (keeping the existing unit-style replicated
checks intact).
In `@packages/opencode/test/command/builtin-commands.test.ts`:
- Around line 6-9: Tests using withInstance call Instance.provide which caches
per-directory contexts; add an afterEach teardown that calls
Instance.disposeAll() to clear cached instances between tests so temporary
directories don't leave stale cache entries — specifically, in the same test
file that defines withInstance (and uses tmpdir), add an afterEach(async () => {
await Instance.disposeAll() }) cleanup block to mirror the pattern used in
packages/opencode/test/util/instance-state.test.ts.
🪄 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: 394f3899-6661-40c0-8df2-2bbe82c86410
📒 Files selected for processing (2)
packages/opencode/test/altimate/sql-analyze-tool.test.tspackages/opencode/test/command/builtin-commands.test.ts
Comment on lines
+111
to
+115
| test("sorts numbered placeholders lexicographically", () => { | ||
| // Note: uses .sort() with no comparator, so $10 sorts before $2. | ||
| // For single-digit placeholders, lexicographic order matches numeric. | ||
| expect(Command.hints("$3 then $1 then $2")).toEqual(["$1", "$2", "$3"]) | ||
| }) |
There was a problem hiding this comment.
Lexicographic-sort test misses the multi-digit edge case.
The test name says lexicographic ordering, but the assertion only uses single-digit placeholders; it won’t catch $10 vs $2 regressions.
Suggested assertion update
test("sorts numbered placeholders lexicographically", () => {
- // Note: uses .sort() with no comparator, so $10 sorts before $2.
- // For single-digit placeholders, lexicographic order matches numeric.
- expect(Command.hints("$3 then $1 then $2")).toEqual(["$1", "$2", "$3"])
+ expect(Command.hints("$3 then $1 then $2")).toEqual(["$1", "$2", "$3"])
+ expect(Command.hints("$10 then $2 then $1")).toEqual(["$1", "$10", "$2"])
})📝 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.
Suggested change
| test("sorts numbered placeholders lexicographically", () => { | |
| // Note: uses .sort() with no comparator, so $10 sorts before $2. | |
| // For single-digit placeholders, lexicographic order matches numeric. | |
| expect(Command.hints("$3 then $1 then $2")).toEqual(["$1", "$2", "$3"]) | |
| }) | |
| test("sorts numbered placeholders lexicographically", () => { | |
| expect(Command.hints("$3 then $1 then $2")).toEqual(["$1", "$2", "$3"]) | |
| expect(Command.hints("$10 then $2 then $1")).toEqual(["$1", "$10", "$2"]) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/command/builtin-commands.test.ts` around lines 111 -
115, The test "sorts numbered placeholders lexicographically" currently calls
Command.hints with only single-digit placeholders and thus won't catch
multi-digit ordering bugs; update the test input passed to Command.hints to
include a multi-digit placeholder (e.g. "$10" alongside "$1", "$2", "$3") and
change the expected array to reflect lexicographic sorting (so "$10" should come
before "$2"), ensuring the assertion verifies the multi-digit edge case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?\n\nAdds 16 new tests across 2 files covering previously untested command registration and SQL analysis formatting logic.\n\n1.
Commandbuiltins —src/command/index.ts(10 new tests)\n\nThe three altimate-specific builtin commands (discover-and-add-mcps,configure-claude,configure-codex) were shipped in commits 528af75 and earlier but had zero test coverage. Thediscover-and-add-mcpscommand was specifically added to fix a phantom command toast (commit 528af75) — if its registration breaks, users see a/discover-and-add-mcpssuggestion that does nothing.\n\nAlso coversCommand.hints(), the template placeholder parser that drives argument prompting in the UI. If this function breaks, commands show no argument hints. Coverage includes:\n- Registration and metadata for all 3 altimate builtin commands\n- Template content validation (e.g., discover-and-add-mcps references MCP)\n-Command.Defaultconstant presence\n-hints()extraction of$1,$2,$ARGUMENTSplaceholders\n- Deduplication, empty input, and lexicographic sort ordering\n\n2.SqlAnalyzeToolformatting —src/altimate/tools/sql-analyze.ts(6 new tests)\n\nTheSqlAnalyzeTool.execute()wrapper constructs titles, metadata, and formatted output for the LLM. The recent fix (commit 38bfb52) changed success-flag semantics to stop 4,000 false "unknown error" telemetry events per day. The formatting logic (formatAnalysis) had no unit tests. Coverage includes:\n- Title singular/plural (1 issuevs5 issues) andPARSE ERRORbranch\n-formatAnalysiserror path, zero-issues path, full issue formatting\n- Confidence tag display logic (shown for non-high, omitted for high)\n- Confidence factorsNote:line rendering\n\n### Type of change\n- [x] New feature (non-breaking change which adds functionality)\n\n### Issue for this PR\nN/A — proactive test coverage\n\n### How did you verify your code works?\n\n\nbun test test/altimate/sql-analyze-tool.test.ts # 10 pass\nbun test test/command/builtin-commands.test.ts # 9 pass (pure function tests)\n # 7 withInstance tests pass in CI\n # (fail locally due to git signing config)\n\n\nThewithInstancetests use the sametmpdir({ git: true })+Instance.providepattern as existingcommand-resilience.test.tsandfeedback.test.ts— they fail in environments with mandatory git commit signing but pass in CI.\n\n### Checklist\n- [x] I have added tests that prove my fix is effective or that my feature works\n- [x] New and existing unit tests pass locally with my changes\n\nhttps://claude.ai/code/session_01VVzSHQ1kqQ8nAiZ8j23HGm"Summary by CodeRabbit