test(readFileTool): add comprehensive test coverage for ReadFileTool#222
Conversation
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ~852 lines of tests expanding ReadFileTool coverage (legacy-format detection and handling, partial/block flows, indentation and slice edge cases, approval messaging, provider-state image limits, and error scenarios) and a small types tweak to detect bare legacy ChangesReadFileTool test coverage expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/tools/__tests__/readFileTool.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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)
src/core/tools/__tests__/readFileTool.spec.ts (1)
771-972: ⚡ Quick winClean up console.warn spies to prevent test leakage.
Multiple tests create
console.warnspies without restoring them after the test completes. Whilevi.clearAllMocks()inbeforeEachclears call history, it doesn't restore original implementations, which can cause spy leakage between tests.The first test (lines 742-749) demonstrates the correct pattern with
consoleWarnSpy.mockRestore(). Apply the same pattern here for consistency and proper test isolation.🧹 Suggested fix: Add spy restoration
For each test creating a console.warn spy, capture the spy and restore it:
it("should handle multiple files in legacy format", async () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() mockTask.ask.mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }) mockedFsReadFile.mockResolvedValueOnce(Buffer.from("file1 content")) mockedFsReadFile.mockResolvedValueOnce(Buffer.from("file2 content")) - vi.spyOn(console, "warn").mockImplementation(() => {}) + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) await readFileTool.execute( { files: [{ path: "file1.ts" }, { path: "file2.ts" }] } as any, mockTask as any, callbacks, ) expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("file1 content")) expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("file2 content")) + consoleWarnSpy.mockRestore() })Apply this pattern to all affected tests at lines 771, 787, 800, 814, 827, 857, 878, 892, 905, 923, 939, 959, and 972.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/readFileTool.spec.ts` around lines 771 - 972, Several tests in readFileTool.spec.ts create console.warn spies via vi.spyOn(console, "warn").mockImplementation(() => {}) but do not restore them, causing leakage; update each affected test that calls vi.spyOn(console, "warn") to assign the spy to a variable (e.g., const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})) and call consoleWarnSpy.mockRestore() at the end of the test (after awaiting readFileTool.execute and assertions) to restore the original implementation; look for usages in tests like "should block rooignore-protected files in legacy format", "should deny legacy file read when user clicks no", "should handle directory path in legacy format", and similar, replacing the raw spy calls with a captured const and restoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/tools/__tests__/readFileTool.spec.ts`:
- Around line 771-972: Several tests in readFileTool.spec.ts create console.warn
spies via vi.spyOn(console, "warn").mockImplementation(() => {}) but do not
restore them, causing leakage; update each affected test that calls
vi.spyOn(console, "warn") to assign the spy to a variable (e.g., const
consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})) and
call consoleWarnSpy.mockRestore() at the end of the test (after awaiting
readFileTool.execute and assertions) to restore the original implementation;
look for usages in tests like "should block rooignore-protected files in legacy
format", "should deny legacy file read when user clicks no", "should handle
directory path in legacy format", and similar, replacing the raw spy calls with
a captured const and restoring it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c21b1200-c403-42b0-afbb-da0f264e09e6
📒 Files selected for processing (1)
src/core/tools/__tests__/readFileTool.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/types/src/tool-params.ts (1)
92-96: 💤 Low valueConsider validating
filesarray element shape for stronger type safety.The type guard returns
params is LegacyReadFileParamswhen afilesproperty exists and is an array, but doesn't verify that array elements match theFileEntryinterface. This could allow malformed inputs to pass the guard.While this is pragmatic for backward compatibility with real legacy chat data (the stated goal), consider adding a lightweight shape check if type safety is a concern:
const hasFilesArray = "files" in params && Array.isArray((params as unknown as Record<string, unknown>).files) && (params as any).files.every((f: any) => f && typeof f.path === 'string')However, given the explicit goal of supporting legacy chat data and the low likelihood of misuse, the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/types/src/tool-params.ts` around lines 92 - 96, The type guard currently treats any array on params.files as LegacyReadFileParams which can let malformed entries through; update the hasFilesArray check in the same function (the variables hasFilesArray and hasLegacyFlag that return params is LegacyReadFileParams) to perform a lightweight shape validation on each element (e.g., ensure each item is an object and has a string path and any other required FileEntry fields) before returning true so that only arrays of valid FileEntry-like objects pass the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/types/src/tool-params.ts`:
- Around line 92-96: The type guard currently treats any array on params.files
as LegacyReadFileParams which can let malformed entries through; update the
hasFilesArray check in the same function (the variables hasFilesArray and
hasLegacyFlag that return params is LegacyReadFileParams) to perform a
lightweight shape validation on each element (e.g., ensure each item is an
object and has a string path and any other required FileEntry fields) before
returning true so that only arrays of valid FileEntry-like objects pass the
guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2e883f2-72c6-4206-969c-504915323a29
📒 Files selected for processing (2)
packages/types/src/tool-params.tssrc/core/tools/__tests__/readFileTool.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Addressed the spy leakage @coderabbitai — added a scoped `afterEach` that restores `console.warn` (guarded by `vi.isMockFunction`) so it no longer leaks across tests, while leaving the suite-level module mocks intact. 75 tests pass locally. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
proyectoauraorg
force-pushed
the
test/add-readfile-tool-tests
branch
2 times, most recently
from
7b97253 to
4d04905
Compare
proyectoauraorg
force-pushed
the
test/add-readfile-tool-tests
branch
from
ed12b73 to
538459c
Compare
|
Conflicts resolved and branch rebased on latest main. CI is passing. Ready for re-review. 🙏 |
| // Restore the console.warn spy that some tests install via vi.spyOn so it | ||
| // doesn't leak into subsequent tests. Scoped to console to avoid restoring | ||
| // the module-level mocks (e.g. ImageMemoryTracker) set up for the suite. | ||
| if (vi.isMockFunction(console.warn)) { |
There was a problem hiding this comment.
The comment says this avoids restoring module-level mocks like ImageMemoryTracker, but vi.restoreAllMocks() only restores vi.spyOn spies — it never touches vi.mock() factories. Would a plain vi.restoreAllMocks() (the pattern in ReadCommandOutputTool.test.ts:81) work here instead?
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous feedback - could more comments to minimize the diff.
| const mockTask = createMockTask({ rooIgnoreAllowed: false }) | ||
| const callbacks = createMockCallbacks() | ||
|
|
||
| vi.spyOn(console, "warn").mockImplementation(() => {}) |
There was a problem hiding this comment.
ReadFileTool.ts no longer has any console.warn calls after the removal in this same PR — these 13 spies are suppressing a warning that never fires. Could they be dropped?
| const callbacks = createMockCallbacks() | ||
|
|
||
| // The new format only handles single files, but approval uses batch logic internally | ||
| // when rooignore allows multiple files. Testing the single file yes flow: |
There was a problem hiding this comment.
The comment inside the first test here concedes "The new format only handles single files". The multi-file branch of requestApproval (which handles per-file permissions and the "individual" third-response option) doesn't appear to be covered anywhere — is that intentionally deferred?
| expect(callbacks.pushToolResult).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should deny single file when user clicks no", async () => { |
There was a problem hiding this comment.
These tests (deny, feedback-on-denial, feedback-on-approval) look like they cover the same scenarios as the existing "approval flow" describe at line 593. Is there something distinct about the "batch" path being exercised here?
… with native path Add unit coverage for ReadFileTool: input validation, rooignore blocking, directory/binary/image handling, image memory limits, approval flow, slice and indentation modes, output structure, and the legacy multi-file format. Source changes: - Drop the temporary "[read_file] Legacy format detected" debug console.warn. - Mirror the native path in the legacy read path: set didToolFailInCurrentTurn on rooignore blocks, directory reads, and read errors so a failed legacy read fails the tool turn consistently. - Recognize re-hydrated bare-`files` calls in isLegacyReadFileParams (history persisted before the _legacyFormat flag existed).
drvaquera
force-pushed
the
test/add-readfile-tool-tests
branch
from
538459c to
23121d6
Compare
|
Thanks for the thorough review! Slimmed the diff and addressed the inline notes:
Squashed to one commit. |
2 participants
Summary
Adds comprehensive test coverage for ReadFileTool, increasing from 33 to 77 tests (+44 new tests).
New Test Coverage
handlePartial error handling:
getStartLine fallback logic:
getLineSnippet edge cases:
Path normalization:
Legacy parameters:
Direct file reading:
Batch approval flow:
params getter:
Test Results
Summary by CodeRabbit