test: consolidate 12 hourly test PRs + fix slugify and hints sort#439
Merged
anandgupta42 merged 2 commits intomainfrom Mar 24, 2026
Merged
test: consolidate 12 hourly test PRs + fix slugify and hints sort#439anandgupta42 merged 2 commits intomainfrom
anandgupta42 merged 2 commits intomainfrom
Conversation
Combines test coverage from 12 redundant `test:` PRs (each containing duplicate `hints.test.ts`) into one cohesive PR with unique tests only. **New test files (10):** - `command/hints.test.ts` — `Command.hints()` placeholder extraction - `skill/fmt.test.ts` — `Skill.fmt()` formatting for TUI and system prompt - `altimate/tools/sql-analyze-tool.test.ts` — AI-5975 success semantics - `patch/seek-sequence.test.ts` — `seekSequence` 4-pass matching - `cli/error-format.test.ts` — `FormatError` all 8 NamedError branches - `cli/is-tool-on-path.test.ts` — tool discovery paths - `tool/batch.test.ts` — `BatchTool` schema + `formatValidationError` - `command/altimate-commands.test.ts` — builtin command registration - `altimate/tools/impact-analysis.test.ts` — DAG traversal + report - `altimate/tools/training-import.test.ts` — markdown parser + `slugify` **Enhanced existing tests (4):** - `finops-formatting.test.ts` — TB/PB units, negative values - `wildcard.test.ts` — star crosses `/`, regex escaping, tail matching - `config/markdown.test.ts` — shell extraction, frontmatter sanitization - `util/filesystem.test.ts` — `findUp`, `globUp`, `overlaps` **Bug fixes (2):** - `slugify`: NFKD normalization for accented chars (café→cafe not caf), `"untitled"` fallback for empty results, truncate-before-trim order - `Command.hints`: numeric sort for multi-digit placeholders ($1,$2,$10 not $1,$10,$2) 269 new tests, 5046 total pass, 0 fail. 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.
3 tasks
This was referenced Mar 24, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughConsolidates many test additions and promotes several internal utilities to exported APIs; fixes slug generation (NFKD normalization, diacritic stripping, trimming, 64-char truncation, "untitled" fallback) and sorts numeric placeholders in Command.hints by numeric value. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This was referenced Mar 24, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/opencode/test/tool/batch.test.ts (1)
66-89: Consider removing redundanttoBeDefined()checks.The
expect(tool.formatValidationError).toBeDefined()assertions at lines 68 and 80 duplicate the dedicated test at line 63. Since eachdescribeblock's tests typically share setup assumptions, these can be removed for brevity.♻️ Proposed cleanup
test("produces readable error message for empty array", async () => { const tool = await getToolInfo() - expect(tool.formatValidationError).toBeDefined() const result = tool.parameters.safeParse({ tool_calls: [] }) expect(result.success).toBe(false) if (!result.success) { const msg = tool.formatValidationError!(result.error) expect(msg).toContain("Invalid parameters for tool 'batch'") expect(msg).toContain("Expected payload format") } }) test("includes field path in type error", async () => { const tool = await getToolInfo() - expect(tool.formatValidationError).toBeDefined() const result = tool.parameters.safeParse({ tool_calls: [{ tool: 123, parameters: {} }], })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/batch.test.ts` around lines 66 - 89, Remove the redundant assertions that check the presence of formatValidationError in the two tests ("produces readable error message for empty array" and "includes field path in type error"): delete the lines calling expect(tool.formatValidationError).toBeDefined() in those tests and rely on the dedicated existence check already present elsewhere; keep the rest of the assertions that call tool.formatValidationError!(...) and validate the returned messages.packages/opencode/test/skill/fmt.test.ts (1)
19-33: Add coverage for file URL conversion on every listed skill.This case builds two filesystem-based skills but only asserts
file://conversion for one. A per-item conversion bug on later items could slip through.Diff to strengthen this test
test("verbose mode returns XML with skill tags", () => { const skills = [ skill({ name: "analyze", description: "Analyze code", location: "/path/to/analyze/SKILL.md" }), skill({ name: "deploy", description: "Deploy app", location: "/path/to/deploy/SKILL.md" }), ] const output = Skill.fmt(skills, { verbose: true }) @@ // File paths get converted to file:// URLs expect(output).toContain("file:///path/to/analyze/SKILL.md") + expect(output).toContain("file:///path/to/deploy/SKILL.md") })🤖 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 only asserts file:// conversion for the first skill; update the "verbose mode returns XML with skill tags" test to assert file URL conversion for every skill in the skills array (e.g., verify that output contains "file:///path/to/analyze/SKILL.md" and also "file:///path/to/deploy/SKILL.md"), by adding a second expect against Skill.fmt(...) output so both skill entries are validated; look for the test block that calls Skill.fmt(skills, { verbose: true }) and the skills array created with skill({ name: "analyze", ... }) and skill({ name: "deploy", ... }) to place the additional assertion.packages/opencode/test/altimate/tools/sql-analyze-tool.test.ts (1)
111-113: Consider extendingmockDispatcherto support rejections for consistency.The direct spy manipulation here works correctly, but diverges from the
mockDispatcherhelper pattern used in other tests. A minor optional improvement would be to extend the helper to accept a rejection option.♻️ Optional: extend mockDispatcher helper
-function mockDispatcher(response: any) { +function mockDispatcher(response: any, reject = false) { dispatcherSpy?.mockRestore() - dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation(async () => response) + dispatcherSpy = reject + ? spyOn(Dispatcher, "call").mockRejectedValue(response) + : spyOn(Dispatcher, "call").mockResolvedValue(response) }Then in the test:
test("dispatcher throws → catch block returns ERROR title", async () => { - dispatcherSpy?.mockRestore() - dispatcherSpy = spyOn(Dispatcher, "call").mockRejectedValue(new Error("native crash")) + mockDispatcher(new Error("native crash"), true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/sql-analyze-tool.test.ts` around lines 111 - 113, The test directly manipulates dispatcherSpy to mock a rejection instead of using the existing mockDispatcher helper; extend mockDispatcher to accept an option (e.g., rejectWith) so it can create a rejected mock via spyOn(Dispatcher, "call").mockRejectedValue(rejectWith) (and keep existing resolve behavior), then update this test to call mockDispatcher({ rejectWith: new Error("native crash") }) and remove the direct dispatcherSpy?.mockRestore()/spyOn calls so all tests use the same helper (function names: mockDispatcher, Dispatcher.call, dispatcherSpy).packages/opencode/test/altimate/tools/impact-analysis.test.ts (1)
167-172: Prefer order-independent assertions for downstream results.A few tests assume exact traversal order (
result[0],result[1], etc.). That makes tests brittle if traversal strategy changes (while behavior remains correct). Assert by set/object containment instead of positional indexes.Example refactor pattern
- expect(result[0]).toMatchObject({ name: "fct_orders", depth: 1 }) - expect(result[1]).toMatchObject({ name: "dim_orders", depth: 2 }) - expect(result[2]).toMatchObject({ name: "report_orders", depth: 3 }) + expect(result).toEqual( + expect.arrayContaining([ + expect.objectContaining({ name: "fct_orders", depth: 1 }), + expect.objectContaining({ name: "dim_orders", depth: 2 }), + expect.objectContaining({ name: "report_orders", depth: 3 }), + ]), + )Also applies to: 180-183, 227-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/impact-analysis.test.ts` around lines 167 - 172, The test is asserting specific traversal order for findDownstream by indexing result[0], result[1], etc., which is brittle; update the assertions to be order-independent by checking that result contains the expected model objects (by name and depth) regardless of position—e.g., assert using array-contains style checks (expect(result).toEqual(expect.arrayContaining(...))) or convert result to a lookup keyed by name and assert each expected entry matches the depth; apply this change in the current block using the result variable for "stg_orders" and the other two similar blocks referenced (around the checks at lines 180-183 and 227-230).packages/opencode/test/altimate/tools/training-import.test.ts (1)
3-4: Consider exporting helpers to avoid code duplication.The comment acknowledges that
slugifyandparseMarkdownSectionsare copied from production code because they aren't exported. This creates a maintenance risk—if the production implementations change, these test copies may silently diverge, leading to false confidence in test coverage.♻️ Suggested approach
Export the helpers from the production module (even if marked as internal):
// In src/altimate/tools/training-import.ts /** `@internal` */ export function slugify(text: string): string { ... } /** `@internal` */ export function parseMarkdownSections(markdown: string): MarkdownSection[] { ... }Then import them directly in tests:
import { slugify, parseMarkdownSections } from "../../../src/altimate/tools/training-import"Alternatively, move these utilities to a shared
util/module if they have broader applicability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/training-import.test.ts` around lines 3 - 4, Tests are duplicating the non-exported helpers slugify and parseMarkdownSections, risking divergence; export these helpers from the production module (e.g., mark them `@internal` and export function slugify(...) and export function parseMarkdownSections(...)) or move them into a shared util module, then update the test to import slugify and parseMarkdownSections from the production source so the test uses the canonical implementations.packages/opencode/test/cli/is-tool-on-path.test.ts (1)
10-22: Consider usingtmpdir({ git: true })instead of custom helper.The
tmpdirGithelper duplicates thegit: trueoption available intmpdir(). If the extra git config (fsmonitor, gpgsign, user settings) is necessary for CI stability, consider documenting this in a comment or proposing to add these configs to the standardgit: truebehavior. Based on learnings: "Use thegit: trueoption when initializing a temporary test directory that needs a git repository with a root commit."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/cli/is-tool-on-path.test.ts` around lines 10 - 22, The tmpdirGit helper duplicates built-in tmpdir({ git: true }) functionality and adds extra git config; update the test to use tmpdir({ git: true }) instead of the custom tmpdirGit helper, or if the additional configs (core.fsmonitor, commit.gpgsign, user.email, user.name, root commit) are required for CI stability, keep the helper but add a brief comment on tmpdirGit explaining why these settings are necessary (or propose elevating those configs into the shared tmpdir implementation); locate and change the tmpdirGit function (and its call sites) accordingly so tests use the built-in git option or are clearly documented.
🤖 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/altimate/tools/impact-analysis.test.ts`:
- Around line 3-5: The test currently contains local copies of the logic for
findDownstream and formatImpactReport, so update the test to exercise the real
production implementations: remove the duplicated implementations from the test
and import findDownstream and formatImpactReport from the production module (or
export and import the pure helper functions from the production file) and use
those imported functions in the assertions; ensure the test no longer defines
its own versions of findDownstream/formatImpactReport and instead calls the real
functions so regressions in the production implementation will be caught.
In `@packages/opencode/test/cli/is-tool-on-path.test.ts`:
- Around line 101-102: The test is hardcoding a Unix ':' PATH separator when
setting process.env.PATH (using savedEnv.PATH and process.env.PATH), which
breaks on Windows; change the concatenation to use Node's platform-specific
separator via path.delimiter (i.e., set process.env.PATH = path.join(tmp.path,
"path-bin") + path.delimiter + (process.env.PATH ?? "")) so the PATH is
constructed correctly across platforms.
In `@packages/opencode/test/patch/seek-sequence.test.ts`:
- Around line 169-172: The test "throws when file does not exist" currently
hard-codes a Unix-specific absolute path; update it so the missing-file path is
constructed from a portable temp directory instead (e.g., use Node's os.tmpdir()
or the test framework's temp-dir helper) and pass that into
Patch.deriveNewContentsFromChunks so the test is platform-agnostic; locate the
assertion in seek-sequence.test.ts around the test named "throws when file does
not exist" and replace the literal "/tmp/..." with a generated non-existent path
under the temp directory.
- Around line 5-32: Replace the manual temp-dir lifecycle (the
beforeEach/afterEach that calls fs.mkdtemp and fs.rm and the tempDir variable)
with the shared fixture pattern: call the tmpdir() fixture with "await using
tmpdir()" to obtain tempDir and remove the manual fs.mkdtemp/fs.rm code; update
any references to tempDir to use that fixture-provided value. Also replace the
hard-coded path string "/tmp/nonexistent-file-12345.txt" with a path built from
the fixture tempDir (e.g. path.join(tempDir, "nonexistent-file.txt")) so the
test no longer depends on system /tmp layout. Ensure you reference the tmpdir()
fixture, the tempDir variable, and remove beforeEach/afterEach helpers
accordingly.
---
Nitpick comments:
In `@packages/opencode/test/altimate/tools/impact-analysis.test.ts`:
- Around line 167-172: The test is asserting specific traversal order for
findDownstream by indexing result[0], result[1], etc., which is brittle; update
the assertions to be order-independent by checking that result contains the
expected model objects (by name and depth) regardless of position—e.g., assert
using array-contains style checks
(expect(result).toEqual(expect.arrayContaining(...))) or convert result to a
lookup keyed by name and assert each expected entry matches the depth; apply
this change in the current block using the result variable for "stg_orders" and
the other two similar blocks referenced (around the checks at lines 180-183 and
227-230).
In `@packages/opencode/test/altimate/tools/sql-analyze-tool.test.ts`:
- Around line 111-113: The test directly manipulates dispatcherSpy to mock a
rejection instead of using the existing mockDispatcher helper; extend
mockDispatcher to accept an option (e.g., rejectWith) so it can create a
rejected mock via spyOn(Dispatcher, "call").mockRejectedValue(rejectWith) (and
keep existing resolve behavior), then update this test to call mockDispatcher({
rejectWith: new Error("native crash") }) and remove the direct
dispatcherSpy?.mockRestore()/spyOn calls so all tests use the same helper
(function names: mockDispatcher, Dispatcher.call, dispatcherSpy).
In `@packages/opencode/test/altimate/tools/training-import.test.ts`:
- Around line 3-4: Tests are duplicating the non-exported helpers slugify and
parseMarkdownSections, risking divergence; export these helpers from the
production module (e.g., mark them `@internal` and export function slugify(...)
and export function parseMarkdownSections(...)) or move them into a shared util
module, then update the test to import slugify and parseMarkdownSections from
the production source so the test uses the canonical implementations.
In `@packages/opencode/test/cli/is-tool-on-path.test.ts`:
- Around line 10-22: The tmpdirGit helper duplicates built-in tmpdir({ git: true
}) functionality and adds extra git config; update the test to use tmpdir({ git:
true }) instead of the custom tmpdirGit helper, or if the additional configs
(core.fsmonitor, commit.gpgsign, user.email, user.name, root commit) are
required for CI stability, keep the helper but add a brief comment on tmpdirGit
explaining why these settings are necessary (or propose elevating those configs
into the shared tmpdir implementation); locate and change the tmpdirGit function
(and its call sites) accordingly so tests use the built-in git option or are
clearly documented.
In `@packages/opencode/test/skill/fmt.test.ts`:
- Around line 19-33: The test currently only asserts file:// conversion for the
first skill; update the "verbose mode returns XML with skill tags" test to
assert file URL conversion for every skill in the skills array (e.g., verify
that output contains "file:///path/to/analyze/SKILL.md" and also
"file:///path/to/deploy/SKILL.md"), by adding a second expect against
Skill.fmt(...) output so both skill entries are validated; look for the test
block that calls Skill.fmt(skills, { verbose: true }) and the skills array
created with skill({ name: "analyze", ... }) and skill({ name: "deploy", ... })
to place the additional assertion.
In `@packages/opencode/test/tool/batch.test.ts`:
- Around line 66-89: Remove the redundant assertions that check the presence of
formatValidationError in the two tests ("produces readable error message for
empty array" and "includes field path in type error"): delete the lines calling
expect(tool.formatValidationError).toBeDefined() in those tests and rely on the
dedicated existence check already present elsewhere; keep the rest of the
assertions that call tool.formatValidationError!(...) and validate the returned
messages.
🪄 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: 836e66bb-c943-44b1-a843-9c96561fb64e
📒 Files selected for processing (16)
packages/opencode/src/altimate/tools/training-import.tspackages/opencode/src/command/index.tspackages/opencode/test/altimate/tools/finops-formatting.test.tspackages/opencode/test/altimate/tools/impact-analysis.test.tspackages/opencode/test/altimate/tools/sql-analyze-tool.test.tspackages/opencode/test/altimate/tools/training-import.test.tspackages/opencode/test/cli/error-format.test.tspackages/opencode/test/cli/is-tool-on-path.test.tspackages/opencode/test/command/altimate-commands.test.tspackages/opencode/test/command/hints.test.tspackages/opencode/test/config/markdown.test.tspackages/opencode/test/patch/seek-sequence.test.tspackages/opencode/test/skill/fmt.test.tspackages/opencode/test/tool/batch.test.tspackages/opencode/test/util/filesystem.test.tspackages/opencode/test/util/wildcard.test.ts
…e, fix portability
- Export `findDownstream`, `formatImpactReport`, `DownstreamModel` from
`impact-analysis.ts` and import in tests instead of copying
- Export `parseMarkdownSections`, `slugify`, `MarkdownSection` from
`training-import.ts` and import in tests instead of copying
- Replace manual `beforeEach`/`afterEach` temp dir lifecycle in
`seek-sequence.test.ts` with shared `tmpdir()` fixture pattern
- Replace `require("fs").writeFileSync` with async `fs.writeFile`
- Replace hardcoded `/tmp/` path with `path.join(tmp.path, ...)`
- Use cross-platform PATH separator in `is-tool-on-path.test.ts`
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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?
Consolidates 12 redundant
test:PRs (each from the hourly test-discovery skill) into a single cohesive PR. Deduplicates overlapping tests (7 PRs had near-identicalhints.test.ts), keeps the best version of each unique test, and fixes 2 bugs found during review.New test files (10):
command/hints.test.ts,skill/fmt.test.ts,altimate/tools/sql-analyze-tool.test.ts,patch/seek-sequence.test.ts,cli/error-format.test.ts,cli/is-tool-on-path.test.ts,tool/batch.test.ts,command/altimate-commands.test.ts,altimate/tools/impact-analysis.test.ts,altimate/tools/training-import.test.tsEnhanced existing tests (4):
finops-formatting.test.ts(TB/PB units),wildcard.test.ts(star crosses/, regex escaping),config/markdown.test.ts(shell extraction, frontmatter),util/filesystem.test.ts(findUp,globUp,overlaps)Bug fixes (2):
slugify: NFKD normalization for accented chars (café→cafenotcaf),"untitled"fallback for empty results, truncate-before-trim order fixCommand.hints: numeric sort for multi-digit placeholders ($1,$2,$10not$1,$10,$2)269 new tests, 5046 total pass, 0 fail.
Supersedes PRs: #430, #408, #437, #435, #423, #412, #436, #431, #425, #414, #427, #374
Type of change
Issue for this PR
Closes #438
How did you verify your code works?
6-model consensus code review (Claude, GPT 5.2 Codex, Gemini 3.1 Pro, Kimi K2.5, MiniMax M2.5, GLM-5) — all approved in 1 convergence round.
Checklist
Summary by CodeRabbit
Bug Fixes
$10after$2).Tests