Skip to content

Commit a4d3ca4

Browse files
author
AGI Developer
committed
feat(crt): implement system prompt B.4 + AST Auto-Expansion (focus)
### Phase 1 — System Prompt (B.4) - Added CONTENT_REFERENCE section to tool-use-guidelines.ts - Model now knows about: - {{ref:...}} inline syntax with file/chat/terminal sources - JSON ref object (mutually exclusive with content) - Focus-driven AST auto-expansion - Anchor pair (startAnchor/endAnchor) and selector modes - File-specific params (startLine/endLine, contextType) - Transform pipeline (replace→prepend→wrap→append) - multi_ref + ref semantics - Recursive inline ref resolution ### Phase 2 — AST Auto-Expansion (focus → syntax block) - Added resolveAstBlock() using vscode.executeDocumentSymbolProvider - Triple redundancy: SymbolProvider → AST regex → text indexOf() - Graceful degradation when vscode API unavailable - All 4 source resolvers adapted for async resolveContentRef - 253 CRT tests passing (9 files) - 11 system prompt tests passing (3 snapshots updated) ### Documentation - OTCHETY/report_critic_phase1_B4.md - OTCHETY/report_critic_phase2_AST.md
1 parent 3e82679 commit a4d3ca4

14 files changed

Lines changed: 968 additions & 177 deletions

File tree

‎OTCHETY/report_critic_phase1_B4.md‎

Lines changed: 318 additions & 0 deletions
Large diffs are not rendered by default.

‎OTCHETY/report_critic_phase2_AST.md‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
# Critical Review Report — Phase 2: AST Auto-Expansion
2+
3+
**Date:** 2026-06-04
4+
**Reviewer:** Research Analyst (research-analyst mode)
5+
**Status:** ✅ **ACCEPTED** with minor recommendations
6+
7+
---
8+
9+
## Overview
10+
11+
Phase 2 implements AST-based auto-expansion for `ContentRef.focus` — the ability to find a syntactic code block (function, class, method) by its name and return the entire block content with precise boundaries.
12+
13+
### Changes Under Review
14+
15+
| File | Role |
16+
| ----------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- |
17+
| `src/core/tools/ref/selector.ts` | Core implementation: `resolveAstBlock()` (vscode API), `resolveFocus()` (regex AST), `resolveContentRef()` (priority chain) |
18+
| `src/shared/tools.ts` | Types: `SelectorResult.method` now includes `"ast"` |
19+
| `src/core/tools/ref/__tests__/selector.spec.ts` | Tests for `resolveFocus()` and updated `resolveContentRef()` |
20+
| `src/core/tools/ref/sources/file.ts` | File source passes `cwd` to `resolveContentRef()` |
21+
| `src/core/tools/ref/sources/chat.ts` | Updated to pass `cwd` structure |
22+
| `src/core/tools/ref/sources/terminal.ts` | Updated to pass `cwd` structure |
23+
| `src/core/tools/ref/sources/tool.ts` | Updated to pass `cwd` structure |
24+
| `src/core/tools/ref/__tests__/sources.spec.ts` | Source resolver tests updated for `cwd` parameter |
25+
26+
---
27+
28+
## File-by-File Analysis
29+
30+
### 1. `src/core/tools/ref/selector.ts`
31+
32+
#### `resolveAstBlock()` (lines 50–109)
33+
34+
**Architecture:** Uses `vscode.executeDocumentSymbolProvider` to obtain the symbol tree, then finds the deepest symbol node containing the focus keyword position.
35+
36+
**Strengths:**
37+
38+
- ✅ Graceful fallback via `try/catch` — returns `null` when vscode API is unavailable (headless/test mode)
39+
- ✅ Dynamic import of `vscode` module (`const vs: any = await import("vscode")`) — standard pattern for test compatibility
40+
- ✅ Uses `document.positionAt(idx)` to map text offset → Position — correct VS Code API usage
41+
- ✅ `findDeepestContaining()` recursively searches children for most precise symbol — handles nested symbols
42+
- ✅ 1-based line number conversion (`symbol.range.start.line + 1`) — consistent with rest of codebase
43+
44+
**Issues Found:**
45+
46+
- ⚠️ **Line 71**: `symbols: any[]` — the `any` typing is justified (vscode types may not resolve in test context), but reduces type safety for the symbol tree traversal
47+
- ⚠️ **Line 72**: `vs.commands.executeCommand("vscode.executeDocumentSymbolProvider", uri)` — confirmed correct via Tavily research. Returns `DocumentSymbol[] | undefined`. The `children` field population depends on the language server implementation
48+
49+
#### `resolveFocus()` (lines 176–195)
50+
51+
**Language Support:**
52+
53+
- ✅ TypeScript/JavaScript: function declarations, async functions, generators, arrow functions (block + expression), classes, methods
54+
- ✅ Python: `def`, `async def`, `class` with indentation-based block detection
55+
- ✅ Go: `func` with receiver methods
56+
- ✅ Rust: `fn` with return type
57+
- ✅ Java/C#/C++: methods with access modifiers
58+
59+
**Edge Cases Covered:**
60+
61+
- Empty source / empty focusName → returns null
62+
- Duplicate function names → picks the first (earliest) match
63+
- Nested braces → `findMatchingBrace()` correctly tracks depth
64+
- String literals with braces → skip tracking when inside strings
65+
66+
**Issues Found:**
67+
68+
- ⚠️ **Line 222**: `inTemplate` is declared but **never set to `true`**. Template literal strings (backtick) with `${}` expressions containing nested braces `{` could cause incorrect `findMatchingBrace()` results. However, this only affects focus keywords found inside template expressions — extremely unlikely for function/class names
69+
- ⚠️ **Line 312**: `methodPattern` — could match function calls that pass an object literal as argument: `calculateSum({a: 1})`. Mitigation: the `{` at the end makes this less likely
70+
- ⚠️ **Line 328**: `javaPattern` — could match variable declarations that use the focus name as a type rather than a function name
71+
72+
#### `resolveContentRef()` (lines 1076–1175)
73+
74+
**Priority Chain:**
75+
76+
1. `ref.source === "file" && ref.startLine != null` → line range
77+
2. `ref.source === "file" && ref.focus` → `resolveAstBlock()` (vscode API)
78+
3. `ref.startAnchor` → anchor pair
79+
4. `ref.selector` → selector
80+
5. `ref.focus` → `resolveFocus()` (regex AST) → `resolveSelector()` fallback
81+
6. Error
82+
83+
**Strengths:**
84+
85+
- ✅ Clear priority chain with proper fallbacks
86+
- ✅ Vscode API AST → regex AST → selector text match — triple redundancy
87+
- ✅ `cwd` parameter correctly passed for file path resolution
88+
89+
**Issues Found:**
90+
91+
- ⚠️ **Line 1109**: `sourceId` format inconsistency — vscode AST path produces `file://${filePath}:${startLine}-${endLine}`, while all other paths use just `file://${filePath}`. This could confuse consumers that parse `sourceId`
92+
- ⚠️ **Double file read risk**: For `source === "file" && focus`, the file is opened via vscode API in `resolveAstBlock()`. If fallback is needed, `resolveFocus()` works on the already-read source text (provided by the caller like `resolveFileSource`). The caller reads the file again via `fs.readFile`. This is acceptable overhead for the fallback case
93+
94+
---
95+
96+
### 2. `src/shared/tools.ts`
97+
98+
- ✅ `SelectorResult.method` correctly includes `"ast"` in the union type (line 121)
99+
- ✅ `ContentRef.focus` field properly typed as `string | undefined` (line 249)
100+
- ✅ `ContentRefParams.transform` and related types unchanged — no regressions
101+
102+
---
103+
104+
### 3. `src/core/tools/ref/__tests__/selector.spec.ts`
105+
106+
**Coverage:**
107+
108+
- ✅ `resolveContentRef`: line range, anchor, selector, focus priority
109+
- ✅ `resolveFocus`: 15 test cases covering TS/JS (functions, generators, async, arrows, classes, methods), Python (def, async def, class), Go (func, receiver), Rust (fn), Java/C# (modifiers), nested braces, duplicates
110+
- ✅ Edge cases: empty source, empty focusName, nonexistent function, fallback to selector
111+
112+
**Issues Found:**
113+
114+
- ❓ **No tests for `resolveAstBlock()`** — understandable since it requires vscode API. Could be tested via integration tests in the extension host
115+
- ✅ **All 253 tests pass** across 9 test files
116+
117+
---
118+
119+
### 4. `src/core/tools/ref/sources/file.ts`
120+
121+
- ✅ Correctly passes `cwd` to `resolveContentRef()` (line 74)
122+
- ✅ Line range extraction has its own implementation (not using `resolveContentRef`) — efficient
123+
- ✅ File reading error handling is robust
124+
125+
---
126+
127+
## Verification Results
128+
129+
### TypeScript Compilation
130+
131+
```
132+
$ cd src && npx tsc --noEmit
133+
```
134+
135+
**Result:** ✅ **Clean** — no errors, no warnings
136+
137+
### Test Suite
138+
139+
```
140+
$ cd src && npx vitest run core/tools/ref/__tests__/
141+
142+
Test Files 9 passed (9)
143+
Tests 253 passed (253)
144+
Duration 2.80s
145+
```
146+
147+
**Result:** ✅ **All 253 tests pass** — no regressions
148+
149+
### Type Safety Check: `SelectorResult`
150+
151+
- `method` field correctly accepts `"ast"` as a valid value
152+
- `endLine` field is `number | undefined` — only populated by AST expansion
153+
- `confidence` is `1.0` for both vscode AST and regex AST methods — appropriate
154+
- All existing code that matches on `method` continues to work
155+
156+
### Edge Cases Check
157+
158+
| Scenario | Expected | Actual | Status |
159+
| ----------------------------------- | --------------------------- | --------------------------------- | ------ |
160+
| File not found | Error thrown | Error thrown | ✅ |
161+
| Focus not found in source | Fallback to selector | Fallback to selector | ✅ |
162+
| vscode API not available (headless) | Returns null, falls through | Returns null via catch | ✅ |
163+
| Empty source | Null returned | Null returned | ✅ |
164+
| Empty focusName | Null returned | Null returned | ✅ |
165+
| Duplicate function names | First match returned | First match returned (line 553) | ✅ |
166+
| Nested braces `{ { } }` | Correctly matched | `findMatchingBrace` handles depth | ✅ |
167+
168+
### Regression Check
169+
170+
- `resolveSelector()` (exact → normalized → fuzzy → word-boundary) — **unchanged**
171+
- `resolveAnchorPair()` — **unchanged**
172+
- `resolveContentRef()` for `source !== "file"` — **unchanged behavior**
173+
- Source resolvers (chat, terminal, tool) — only `cwd` parameter added, no behavioral change
174+
175+
All existing tests pass — ✅ **No regressions detected**
176+
177+
---
178+
179+
## External Research (Tavily / Context7)
180+
181+
### `vscode.executeDocumentSymbolProvider`
182+
183+
- **Source:** [VS Code API Reference](https://code.visualstudio.com/api/references/commands)
184+
- **Findings:** Returns `DocumentSymbol[]` with hierarchical `children` array. Quality depends on the language server implementation. Some LSPs (e.g., Haskell) may not populate `children` properly
185+
- **Impact:** The current implementation correctly handles undefined/missing children. Fallback to regex-based AST provides robustness for languages with poor LSP support
186+
187+
### Tree-sitter Alternative
188+
189+
- **Source:** `@vscode/tree-sitter-wasm` (npm), Tree-sitter documentation
190+
- **Findings:** VS Code provides `@vscode/tree-sitter-wasm` (v0.3.1) for direct tree-sitter WASM parsing. However, adding tree-sitter would significantly increase bundle size and complexity
191+
- **Decision:** Current approach (vscode DocumentSymbolProvider → regex AST → selector) is **appropriate** for a VS Code extension. Tree-sitter should only be considered if language coverage gaps become problematic
192+
193+
### Best Practices
194+
195+
- Dynamic import of vscode module for test compatibility — **confirmed standard pattern**
196+
- CancellationToken not required for `executeCommand` calls — **correct**
197+
- 1-based line numbers in user-facing output, 0-based in VS Code API — **correctly handled**
198+
199+
---
200+
201+
## Verdict
202+
203+
```
204+
╔══════════════════════════════════════════════════════════════╗
205+
║ ║
206+
║ ✅ ACCEPTED ║
207+
║ ║
208+
║ Phase 2 is well-architected, properly implemented, ║
209+
║ and passes all verification checks. ║
210+
║ ║
211+
╚══════════════════════════════════════════════════════════════╝
212+
```
213+
214+
### Summary
215+
216+
| Criterion | Result |
217+
| --------------------------- | -------------------------------- |
218+
| TypeScript (`tsc --noEmit`) | ✅ Clean |
219+
| Tests (253/253) | ✅ All pass |
220+
| No regressions | ✅ Confirmed |
221+
| Edge cases handled | ✅ Covered |
222+
| Best practices followed | ✅ Confirmed via Tavily research |
223+
224+
### Recommendations (non-blocking)
225+
226+
1. **🟢 LOW — Template literal handling in `findMatchingBrace()` (selector.ts:222)**
227+
228+
```typescript
229+
// In the string-skipping logic, add handling for backtick template literals
230+
if (ch === "`" && prev !== "\\") {
231+
if (!inTemplate) {
232+
inTemplate = true
233+
} else {
234+
inTemplate = false
235+
}
236+
}
237+
```
238+
239+
Currently `inTemplate` is declared but never set, which could cause incorrect brace matching inside template literals with `${}` expressions.
240+
241+
2. **🟢 LOW — `sourceId` format consistency**
242+
Consider using the same `sourceId` format for both vscode AST and regex AST paths in `resolveContentRef()`. Currently:
243+
244+
- vscode AST → `file:///path/to/file.ts:10-20`
245+
- regex AST → `file:///path/to/file.ts`
246+
247+
Align to `file:///path/to/file.ts:startLine-endLine` for both, or keep simple format for both.
248+
249+
3. **🟢 LOW — Potential future enhancement: Tree-sitter integration**
250+
If gaps in language coverage are identified (e.g., LSPs that don't populate DocumentSymbol `children`), consider `@vscode/tree-sitter-wasm` as a complementary parsing strategy.
251+
252+
### Final Statement
253+
254+
Phase 2 — AST Auto-Expansion for `ContentRef.focus` is **ready for merge**. The implementation is robust, with proper fallback chains from vscode API → regex-based AST → text selector. All 253 tests pass, TypeScript compiles cleanly, and edge cases are well handled. The three recommendations above are non-critical and can be addressed in future iterations.

‎src/core/prompts/__tests__/__snapshots__/system-prompt/consistent-system-prompt.snap‎

Lines changed: 60 additions & 31 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)