Skip to content
This repository was archived by the owner on May 15, 2026. It is now read-only.

Commit 198aa6f

Browse files
randomizedcoderrandomizedcoder
committed
fix: update tests for new ollama model discovery interface
1 parent 4e5230d commit 198aa6f

7 files changed

Lines changed: 6096 additions & 17 deletions

File tree

‎docs/design/11208-implementation-progress.md‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
# Implementation Progress: Issue #11208
2+
3+
**Branch**: `fix/11208-retry-and-path-handling`
4+
**Design Doc**: [11208-retry-and-path-handling.md](./11208-retry-and-path-handling.md)
5+
**Started**: 2026-02-05
6+
7+
---
8+
9+
## Phase 1: Fix Issue #11208 (Path Handling + Error Suppression)
10+
11+
### Goal
12+
13+
Fix the specific bug without introducing new infrastructure. This is a targeted fix PR.
14+
15+
### Checklist
16+
17+
- [x] **Step 1.0**: Create shared path normalization helper
18+
19+
- [x] Added to existing `src/utils/pathUtils.ts` (better location)
20+
- [x] Implement `normalizeToolPath()` with 5-step validation
21+
- [x] Export `PathNormalizationResult` interface
22+
23+
- [x] **Step 1.1**: Fix WriteToFileTool path handling
24+
25+
- [x] Add import for `normalizeToolPath`
26+
- [x] Update `execute()` to use shared helper
27+
- [x] Update `handlePartial()` to use shared helper
28+
- [x] Update `hasPathStabilized()` calls to include task parameter
29+
30+
- [x] **Step 1.2**: Add partial error suppression to BaseTool
31+
32+
- [x] Add `PartialExecutionState` discriminated union type
33+
- [x] Add `WeakMap<Task, PartialExecutionState>` for per-task state
34+
- [x] Add `getPartialState()`, `setPartialState()`, `resetPartialState()` methods
35+
- [x] Update `hasPathStabilized()` to take task parameter
36+
- [x] Add `notifyPartialError()` helper method
37+
- [x] Add `shouldSkipDueToPathError()` fast-path guard helper
38+
- [x] Update `handle()` with fast-path guard and error suppression
39+
- [x] Update all tools using these methods:
40+
- [x] WriteToFileTool
41+
- [x] SearchReplaceTool
42+
- [x] SearchAndReplaceTool
43+
- [x] ApplyDiffTool
44+
- [x] EditFileTool
45+
46+
- [x] **Step 1.3**: Add tests for path handling fix
47+
48+
- [x] Create `src/utils/__tests__/pathUtils.spec.ts`
49+
- [x] Add valid path tests (simple, nested, with ./, normalized, absolute within workspace)
50+
- [x] Add invalid path tests (../ traversal, absolute outside workspace)
51+
- [x] Add edge case tests (empty segments, dots, spaces, special chars)
52+
- [x] Add specific test for issue #11208 (/plans path)
53+
54+
- [x] **Step 1.4**: Add tests for error suppression
55+
56+
- [x] Create `src/core/tools/__tests__/BaseTool.spec.ts`
57+
- [x] Test: reports first partial error
58+
- [x] Test: suppresses subsequent identical errors
59+
- [x] Test: logs different error when message changes
60+
- [x] Test: tracks error count
61+
- [x] Test: state recovery after reset
62+
- [x] Test: per-task state isolation
63+
- [x] Test: fast-path guard skips expensive work
64+
- [x] Test: hasPathStabilized behavior
65+
66+
- [x] **Step 1.5**: Run tests and verify
67+
68+
- [x] All existing tests pass (370 files, 5461 tests)
69+
- [x] New tests pass (2 files, 47 tests)
70+
- [ ] Manual verification
71+
72+
**To run new tests only:**
73+
74+
```bash
75+
cd src && pnpm vitest run utils/__tests__/pathUtils.spec.ts core/tools/__tests__/BaseTool.spec.ts
76+
```
77+
78+
**To run all tests:**
79+
80+
```bash
81+
pnpm test # from project root
82+
```
83+
84+
- [ ] **Step 1.6**: Commit changes
85+
- [ ] Stage relevant files
86+
- [ ] Create commit with descriptive message
87+
88+
---
89+
90+
## Progress Log
91+
92+
### 2026-02-05
93+
94+
#### Starting Phase 1 Implementation
95+
96+
Beginning with Step 1.0: Create shared path normalization helper.
97+
98+
### 2026-02-06
99+
100+
#### Completed Step 1.2: Partial Error Suppression in BaseTool
101+
102+
Implemented the full partial error suppression system in `BaseTool.ts`:
103+
104+
1. **PartialExecutionState discriminated union** (lines 14-27):
105+
106+
- `idle`: Initial state
107+
- `streaming`: Active streaming with optional lastSeenPath
108+
- `erroring`: Error state with count, message, firstErrorTime, and isPathValidationError flag
109+
110+
2. **Per-task state isolation via WeakMap** (line 63):
111+
112+
- `private static partialStateByTask = new WeakMap<Task, PartialExecutionState>()`
113+
- Prevents cross-task state pollution
114+
- Memory safe - state is GC'd when Task is GC'd
115+
116+
3. **New methods added**:
117+
118+
- `getPartialState(task)`: Get or initialize state for task
119+
- `setPartialState(task, state)`: Update state for task
120+
- `resetPartialState(task)`: Reset to idle (now takes task parameter)
121+
- `hasPathStabilized(task, path)`: Updated to use per-task state
122+
- `notifyPartialError(task, errorMessage, isPathValidationError)`: Error suppression with logging
123+
- `shouldSkipDueToPathError(task)`: Fast-path guard check
124+
125+
4. **Updated `handle()` method**:
126+
127+
- Added fast-path guard at top of partial handling
128+
- Uses `notifyPartialError()` for error suppression
129+
- Detects path validation errors via message pattern matching
130+
131+
5. **Updated all tools to use new signatures**:
132+
- WriteToFileTool: `hasPathStabilized(task, rawPath)`, `resetPartialState(task)`
133+
- SearchReplaceTool: Same updates
134+
- SearchAndReplaceTool: Same updates
135+
- ApplyDiffTool: Same updates
136+
- EditFileTool: Same updates
137+
138+
#### Dependency Upgrades Required
139+
140+
Initial test runs failed due to pre-existing build issues. Fixed by:
141+
142+
1. **Pulled `flake.nix` and `flake.lock` from `nix` branch**:
143+
144+
```bash
145+
git checkout nix -- flake.nix flake.lock
146+
```
147+
148+
2. **Updated `package.json` (root)**:
149+
150+
- `turbo`: ^2.5.6 → ^2.8.3
151+
- `node` (engines): 20.19.2 → 20.20.0
152+
153+
3. **Updated `src/package.json`**:
154+
155+
- `node` (engines): 20.19.2 → 20.20.0
156+
157+
4. **Updated `writeToFileTool.spec.ts`**:
158+
- Added `normalizeToolPath` to the `pathUtils` mock
159+
- Updated partial error handling test to reflect new error suppression behavior
160+
- Added proper `resetPartialState(mockCline)` call in `beforeEach`
161+
162+
After upgrades, all tests pass:
163+
164+
- 370 test files passed (4 skipped)
165+
- 5461 tests passed (47 skipped)
166+
- 0 failures
167+
168+
#### Completed Step 1.3 & 1.4: Added Tests
169+
170+
Created two new test files:
171+
172+
1. **`src/utils/__tests__/pathUtils.spec.ts`** (26 tests):
173+
174+
- Valid path tests (simple, nested, ./, normalized, absolute within workspace)
175+
- Invalid path tests (../ traversal, absolute paths outside workspace)
176+
- Edge cases (empty segments, dots, spaces, special characters)
177+
- Specific test for issue #11208 (`/plans` path)
178+
179+
2. **`src/core/tools/__tests__/BaseTool.spec.ts`** (21 tests):
180+
- Partial state management (initialization, isolation, reset)
181+
- `hasPathStabilized()` behavior
182+
- `notifyPartialError()` suppression logic
183+
- `shouldSkipDueToPathError()` fast-path guard
184+
- Integration tests for `handle()` error suppression
185+
186+
All 47 new tests pass:
187+
188+
```bash
189+
cd src && pnpm vitest run utils/__tests__/pathUtils.spec.ts core/tools/__tests__/BaseTool.spec.ts
190+
# Test Files 2 passed (2)
191+
# Tests 47 passed (47)
192+
```

‎docs/design/11208-pull-request.md‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
### Related GitHub Issue
2+
3+
Closes: #11208
4+
5+
### Roo Code Task Context (Optional)
6+
7+
N/A
8+
9+
### Description
10+
11+
This PR fixes the EACCES permission denied errors that occur when LLMs provide absolute paths (like `/plans`) in tool calls during streaming.
12+
13+
**Root Cause**: When `path.resolve(task.cwd, "/plans")` is called with an absolute path, it returns `/plans` directly, bypassing the workspace directory entirely. This causes filesystem operations to attempt creating directories at the root level.
14+
15+
**Key Implementation Details**:
16+
17+
1. **Shared path normalization** (`src/utils/pathUtils.ts`):
18+
19+
- Added `normalizeToolPath()` with 5-step validation
20+
- Converts absolute paths to workspace-relative using VS Code API
21+
- Rejects paths that escape workspace via `../` or resolve outside cwd
22+
23+
2. **Error suppression in BaseTool** (`src/core/tools/BaseTool.ts`):
24+
25+
- `PartialExecutionState` discriminated union (idle/streaming/erroring)
26+
- Per-task state isolation via `WeakMap<Task, State>`
27+
- `notifyPartialError()`: logs first error, suppresses identical repeats
28+
- `shouldSkipDueToPathError()`: fast-path guard skips expensive work on known-bad paths
29+
30+
3. **Updated all file-writing tools** to use new per-task signatures:
31+
- `hasPathStabilized(task, path)`
32+
- `resetPartialState(task)`
33+
34+
**Also includes** (required for test infrastructure):
35+
36+
- Upgrade turbo ^2.5.6 → ^2.8.3
37+
- Upgrade node engine 20.19.2 → 20.20.0
38+
- Pull flake.nix/flake.lock from nix branch
39+
40+
### Test Procedure
41+
42+
**New tests added** (47 tests in 2 files):
43+
44+
1. `src/utils/__tests__/pathUtils.spec.ts` - Path normalization tests:
45+
46+
- Valid paths (simple, nested, normalized, absolute within workspace)
47+
- Invalid paths (../ traversal, absolute outside workspace like `/plans`)
48+
- Edge cases (empty segments, dots, spaces, special characters)
49+
50+
2. `src/core/tools/__tests__/BaseTool.spec.ts` - Error suppression tests:
51+
- Per-task state isolation
52+
- Error suppression (first logged, repeats suppressed)
53+
- Fast-path guard behavior
54+
- State recovery after reset
55+
56+
**To run new tests only**:
57+
58+
```bash
59+
cd src && pnpm vitest run utils/__tests__/pathUtils.spec.ts core/tools/__tests__/BaseTool.spec.ts
60+
```
61+
62+
**Full test suite passes**: 370 files, 5461 tests
63+
64+
### Pre-Submission Checklist
65+
66+
- [x] **Issue Linked**: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
67+
- [x] **Scope**: My changes are focused on the linked issue (one major feature/fix per PR).
68+
- [x] **Self-Review**: I have performed a thorough self-review of my code.
69+
- [x] **Testing**: New and/or updated tests have been added to cover my changes (if applicable).
70+
- [x] **Documentation Impact**: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
71+
- [x] **Contribution Guidelines**: I have read and agree to the [Contributor Guidelines](/CONTRIBUTING.md).
72+
73+
### Screenshots / Videos
74+
75+
N/A - No UI changes
76+
77+
### Documentation Updates
78+
79+
- [x] No documentation updates are required.
80+
81+
Internal design docs added in `docs/design/` for future reference:
82+
83+
- `11208-retry-and-path-handling.md` - Design document
84+
- `11208-implementation-progress.md` - Implementation log
85+
86+
### Additional Notes
87+
88+
The fix addresses PR #11209's approach but extends it to also handle the `handlePartial()` streaming path, which is where the repeated errors actually occur during LLM streaming.
89+
90+
### Get in Touch
91+
92+
N/A

0 commit comments

Comments
 (0)