test: file — File.status() git changed file detection#689
test: file — File.status() git changed file detection#689anandgupta42 wants to merge 1 commit intomainfrom
Conversation
File.status() powers the TUI changed-file indicator and had zero direct test coverage. New tests cover modified/untracked/deleted file detection, binary diff parsing (dash line counts), and path normalization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_01P7m7yBWTz8L7Qz17mK9GPP
![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 and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughA new test suite is added for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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.
🧹 Nitpick comments (1)
packages/opencode/test/file/status.test.ts (1)
106-119: Tighten the path-normalization assertion to avoid false positives.
!path.isAbsolute(file.path)is good, but values like../outside.tswould still pass. Consider asserting the expected relative path (or at least rejecting..prefixes) for stronger safety.Suggested assertion tightening
test("normalizes paths to be relative", async () => { @@ fn: async () => { const changed = await File.status() + expect(changed.some((f) => f.path === "src/app.ts")).toBe(true) for (const file of changed) { // All paths should be relative, not absolute expect(path.isAbsolute(file.path)).toBe(false) + expect(file.path.startsWith("..")).toBe(false) } }, }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/file/status.test.ts` around lines 106 - 119, The test "normalizes paths to be relative" currently only asserts !path.isAbsolute(file.path), which still allows paths like "../outside.ts"; update the assertion inside the Instance.provide callback for File.status so each file.path is not absolute and does not contain or start with parent-dir segments—e.g., assert !path.isAbsolute(file.path) AND that file.path does not start with ".." (or contain ".." + path.sep) or, even better, compare to the expected relative path "src/app.ts" for the created file to ensure strict normalization; modify the check near File.status() handling in the test to reject any ".." prefixes or mismatched expected relative path.
🤖 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/file/status.test.ts`:
- Around line 106-119: The test "normalizes paths to be relative" currently only
asserts !path.isAbsolute(file.path), which still allows paths like
"../outside.ts"; update the assertion inside the Instance.provide callback for
File.status so each file.path is not absolute and does not contain or start with
parent-dir segments—e.g., assert !path.isAbsolute(file.path) AND that file.path
does not start with ".." (or contain ".." + path.sep) or, even better, compare
to the expected relative path "src/app.ts" for the created file to ensure strict
normalization; modify the check near File.status() handling in the test to
reject any ".." prefixes or mismatched expected relative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b07b692-a8b7-48d3-a4f2-440ead98e0b9
📒 Files selected for processing (1)
packages/opencode/test/file/status.test.ts
2 participants
Summary
File.status()—src/file/index.ts:421-502(6 new tests)File.status()powers the TUI changed-file indicator that shows users which files have been modified, added, or deleted in their working tree. It parsesgit diff --numstat HEADoutput, detects untracked files viagit ls-files --others, and identifies deleted files viagit diff --diff-filter=D. Despite being a core user-facing function, it had zero direct test coverage — only a single indirect call buried infsmonitor.test.ts.New coverage includes:
added/removedline count parsing fromgit diff --numstatoutput"added"and line count matchescontent.split("\n").length--diff-filter=Dcode path correctly identifies removed files"-"in numstat output (binary diffs) is correctly parsed as0for bothaddedandremoved— this is a real edge case whereparseInt("-", 10)would returnNaNwithout the explicit checkWhy this matters
If
File.status()breaks, the TUI shows incorrect changed-file data — users see wrong file counts, miss deleted files, or getNaNfor binary modifications. The binary-dash parsing and deleted-file detection paths are especially fragile since they rely on specific git output formatting.Type of change
Issue for this PR
N/A — proactive test coverage via test-discovery
How did you verify your code works?
Marker guard check also passes:
Checklist
https://claude.ai/code/session_01P7m7yBWTz8L7Qz17mK9GPP
Summary by cubic
Add 6 tests for
File.status()to ensure reliable git changed-file detection and accurate TUI indicators. Covers modified numstat parsing, untracked additions, deletions, binary “-” counts parsed as 0, clean tree returning an empty list, and project-relative paths.Written for commit dab9700. Summary will update on new commits.
Summary by CodeRabbit