test: consolidate 12 hourly test PRs + fix slugify and hints sort#439
Open
anandgupta42 wants to merge 1 commit intomainfrom
Open
test: consolidate 12 hourly test PRs + fix slugify and hints sort#439anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit 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
📝 WalkthroughWalkthroughThis PR consolidates twelve redundant hourly test PRs into a single comprehensive test suite, deduplicating test files while fixing bugs in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested labels
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
Comment on lines
+3
to
+5
| // Copy of findDownstream and formatImpactReport from | ||
| // src/altimate/tools/impact-analysis.ts (not exported, tested standalone) | ||
|
|
There was a problem hiding this comment.
These tests currently verify copied logic, not the production implementation.
Because findDownstream and formatImpactReport are re-implemented locally in the test file, this suite can pass even if src/altimate/tools/impact-analysis.ts regresses or diverges. Please test the real functions (or extract/export pure helpers from production and import them here).
Suggested direction
-// Copy of findDownstream and formatImpactReport from
-// src/altimate/tools/impact-analysis.ts (not exported, tested standalone)
-
-interface DownstreamModel { ... }
-function findDownstream(...) { ... }
-function formatImpactReport(...) { ... }
+// Import production helpers (after exporting them from src)
+import {
+ findDownstream,
+ formatImpactReport,
+} from "../../../src/altimate/tools/impact-analysis"Also applies to: 13-136
🤖 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 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.
Comment on lines
+101
to
+102
| savedEnv.PATH = process.env.PATH | ||
| process.env.PATH = path.join(tmp.path, "path-bin") + ":" + (process.env.PATH ?? "") |
There was a problem hiding this comment.
PATH separator is hardcoded for Unix; test will fail on Windows.
The colon separator in path.join(...) + ":" + ... is Unix-specific. On Windows, PATH uses semicolons.
🐛 Proposed fix for cross-platform PATH construction
- process.env.PATH = path.join(tmp.path, "path-bin") + ":" + (process.env.PATH ?? "")
+ const sep = process.platform === "win32" ? ";" : ":"
+ process.env.PATH = path.join(tmp.path, "path-bin") + sep + (process.env.PATH ?? "")📝 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
| savedEnv.PATH = process.env.PATH | |
| process.env.PATH = path.join(tmp.path, "path-bin") + ":" + (process.env.PATH ?? "") | |
| savedEnv.PATH = process.env.PATH | |
| const sep = process.platform === "win32" ? ";" : ":" | |
| process.env.PATH = path.join(tmp.path, "path-bin") + sep + (process.env.PATH ?? "") |
🤖 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 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.
Comment on lines
+5
to
+32
| import { tmpdir } from "os" | ||
|
|
||
| /** | ||
| * Tests for Patch.deriveNewContentsFromChunks — the core function that applies | ||
| * update chunks to file content using seekSequence's multi-pass matching. | ||
| * | ||
| * seekSequence tries 4 comparison strategies in order: | ||
| * 1. Exact match | ||
| * 2. Trailing whitespace trimmed (rstrip) | ||
| * 3. Both-end whitespace trimmed (trim) | ||
| * 4. Unicode-normalized + trimmed | ||
| * | ||
| * These tests verify that real-world patch application succeeds even when the | ||
| * LLM-generated patch text has minor whitespace or Unicode differences from | ||
| * the actual file content — a common source of "Failed to find expected lines" | ||
| * errors for users. | ||
| */ | ||
|
|
||
| describe("Patch.deriveNewContentsFromChunks — seekSequence matching", () => { | ||
| let tempDir: string | ||
|
|
||
| beforeEach(async () => { | ||
| tempDir = await fs.mkdtemp(path.join(tmpdir(), "seek-test-")) | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tempDir, { recursive: true, force: true }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify test temp-dir pattern usage and find remaining manual temp-dir lifecycles
rg -n --type=ts 'from "os"|mkdtemp\(|beforeEach\(|afterEach\(' packages/opencode/test
rg -n --type=ts 'await using\s+\w+\s*=\s*await\s+tmpdir\(' packages/opencode/testRepository: AltimateAI/altimate-code
Length of output: 50380
🏁 Script executed:
# Check the full file structure and verify the Line 171 hard-coded path issue
head -n 35 packages/opencode/test/patch/seek-sequence.test.ts
echo "---"
sed -n '165,175p' packages/opencode/test/patch/seek-sequence.test.tsRepository: AltimateAI/altimate-code
Length of output: 1597
🏁 Script executed:
# Count total lines and check how many tests use tempDir vs the hard-coded path
wc -l packages/opencode/test/patch/seek-sequence.test.ts
echo "---"
rg -n 'test\(' packages/opencode/test/patch/seek-sequence.test.ts | wc -l
echo "---"
# Check for hard-coded /tmp paths
rg -n '/tmp/' packages/opencode/test/patch/seek-sequence.test.tsRepository: AltimateAI/altimate-code
Length of output: 214
🏁 Script executed:
# Check the structure of both test suites and their temp dir usage
rg -n 'describe\(|test\(' packages/opencode/test/patch/seek-sequence.test.ts | head -20
echo "---"
# Check if the second describe block also uses tempDir
sed -n '34,80p' packages/opencode/test/patch/seek-sequence.test.tsRepository: AltimateAI/altimate-code
Length of output: 2738
Use fixture tmpdir() + await using instead of manual os.tmpdir lifecycle.
The current setup manually creates/removes temp directories; tests in packages/opencode/test/**/*.test.ts must use the shared fixture pattern with await using tmpdir() for automatic cleanup and consistency.
♻️ Refactor pattern
-import { describe, test, expect, beforeEach, afterEach } from "bun:test"
+import { describe, test, expect } from "bun:test"
import { Patch } from "../../src/patch"
import * as fs from "fs/promises"
import * as path from "path"
-import { tmpdir } from "os"
+import { tmpdir } from "../fixture/fixture"
describe("Patch.deriveNewContentsFromChunks — seekSequence matching", () => {
- let tempDir: string
-
- beforeEach(async () => {
- tempDir = await fs.mkdtemp(path.join(tmpdir(), "seek-test-"))
- })
-
- afterEach(async () => {
- await fs.rm(tempDir, { recursive: true, force: true })
- })
-
- test("exact match: replaces old_lines with new_lines", () => {
+ test("exact match: replaces old_lines with new_lines", async () => {
+ await using tmp = await tmpdir()
+ const filePath = path.join(tmp.path, "exact.txt")
const content = "line1\nline2\nline3\n"
- require("fs").writeFileSync(filePath, content)
+ await fs.writeFile(filePath, content)Also replace the hard-coded /tmp/nonexistent-file-12345.txt at line 171 with a path that doesn't depend on the system temp directory structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/patch/seek-sequence.test.ts` around lines 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.
Comment on lines
+169
to
+172
| test("throws when file does not exist", () => { | ||
| expect(() => | ||
| Patch.deriveNewContentsFromChunks("/tmp/nonexistent-file-12345.txt", [ | ||
| { |
There was a problem hiding this comment.
Avoid platform-specific absolute path in the missing-file test.
Line 171 hard-codes /tmp/..., which is Unix-specific. Prefer a non-existent path under the test temp directory to keep this portable.
💡 Suggested change
- Patch.deriveNewContentsFromChunks("/tmp/nonexistent-file-12345.txt", [
+ Patch.deriveNewContentsFromChunks(path.join(tempDir, "nonexistent-file-12345.txt"), [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/patch/seek-sequence.test.ts` around lines 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.
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
$10now sorts after$2instead of alphabetically).Tests