test: util — findUp, globUp traversal and overlaps boundary#414
test: util — findUp, globUp traversal and overlaps boundary#414anandgupta42 wants to merge 1 commit intomainfrom
Conversation
…ndary findUp/globUp power instruction file discovery (AGENTS.md, CLAUDE.md). If they fail silently or stop early, users in nested project dirs lose all project context. overlaps() governs project boundary detection; incorrect results could cause cross-project context bleeding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_01Hfr4bad38JKSQzQejhHzjC
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.
📝 WalkthroughWalkthroughTest coverage expanded for filesystem utilities with comprehensive validation blocks for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 (1)
packages/opencode/test/util/filesystem.test.ts (1)
632-637: Assert explicit ordering in the multi-levelglobUp()test.The test comment states child-level results should come first, but current assertions only check membership. Since this fixture has one match per level, asserting exact order will better lock traversal-order behavior.
Suggested test tightening
const results = await Filesystem.globUp("*.md", child, tmp.path) - // Child level first, then root level - expect(results.length).toBe(2) - expect(results).toContain(path.join(child, "readme.md")) - expect(results).toContain(path.join(tmp.path, "notes.md")) + // Child level first, then root level + expect(results).toEqual([ + path.join(child, "readme.md"), + path.join(tmp.path, "notes.md"), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/util/filesystem.test.ts` around lines 632 - 637, The test for Filesystem.globUp currently only checks membership but claims child-level results should come first; update the assertions to assert exact ordering by replacing the two .toContain checks and length check with a single equality assertion that results strictly equals [path.join(child, "readme.md"), path.join(tmp.path, "notes.md")] (or the reverse if traversal differs) so the test locks the expected traversal order from Filesystem.globUp.
🤖 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/util/filesystem.test.ts`:
- Around line 632-637: The test for Filesystem.globUp currently only checks
membership but claims child-level results should come first; update the
assertions to assert exact ordering by replacing the two .toContain checks and
length check with a single equality assertion that results strictly equals
[path.join(child, "readme.md"), path.join(tmp.path, "notes.md")] (or the reverse
if traversal differs) so the test locks the expected traversal order from
Filesystem.globUp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e64bcf3-e63d-4ef9-ae8f-dab2f72c295c
📒 Files selected for processing (1)
packages/opencode/test/util/filesystem.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.
Filesystem.findUp()andFilesystem.globUp()—src/util/filesystem.ts(9 new tests)These functions power instruction file discovery (AGENTS.md, CLAUDE.md, CONTEXT.md) by walking directories upward from a starting point. Zero direct tests existed — they were only indirectly exercised through mocked filesystem calls in
instruction.test.ts. If these functions fail silently or stop early, users in nested project directories lose all project-level context from the agent.New coverage includes:
breakplacement)globUpwith wildcard patterns matching multiple files at one levelglobUpwith no matchesglobUprespects stop boundary2.
Filesystem.overlaps()—src/util/filesystem.ts(7 new tests)This function determines whether two directory paths share files (one contains the other). It's used in project detection to determine project boundaries. The implementation uses a non-obvious
||chain onpath.relative()results that could easily regress. Zero direct tests existed.New coverage includes:
/foo/barvs/foo/barbaz— the classic path containment bug)Type of change
Issue for this PR
N/A — proactive test coverage from test-discovery skill
How did you verify your code works?
Checklist
https://claude.ai/code/session_01Hfr4bad38JKSQzQejhHzjC
Summary by CodeRabbit
Note: This release includes internal testing improvements with no user-facing changes.