Skip to content

Commit 6a1a4d9

Browse files
ceilf6ceilf6
committed
refactor: extract context filesystem fact updates
1 parent 9f62050 commit 6a1a4d9

4 files changed

Lines changed: 341 additions & 181 deletions

File tree

‎docs/superpowers/plans/2026-06-08-contextmanager-filesystem-facts.md‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
# ContextManager Filesystem Facts Implementation Plan
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Extract `ContextManager.updateFileSystemFacts` low-level filesystem fact mutation into a focused helper while preserving the public facade and behavior.
6+
7+
**Architecture:** Keep `ContextManager.updateFileSystemFacts(taskId, toolName, params, result)` as the public API and revision-bump owner. Add `packages/core/src/context/filesystem-facts-update.ts` with a pure mutating helper that receives `ProjectFacts`, tool metadata, and tool result, returning whether facts changed. The helper owns filesense deltas, filesense index enrichment, file/list/search tool-result contracts, and set/map mutation semantics.
8+
9+
**Tech Stack:** TypeScript, Vitest, existing `ProjectFacts` types and context helper utilities.
10+
11+
---
12+
13+
### Task 1: Focused Helper Contract Test
14+
15+
**Files:**
16+
- Modify: `packages/core/src/context/context-manager.test.ts`
17+
- Create later: `packages/core/src/context/filesystem-facts-update.ts`
18+
19+
- [ ] **Step 1: Add a failing helper-level test**
20+
21+
Add an import:
22+
23+
```ts
24+
import { updateFilesystemFactsFromToolResult } from './filesystem-facts-update.js';
25+
```
26+
27+
Add this test in `describe('updateFileSystemFacts', ...)`:
28+
29+
```ts
30+
it('keeps filesystem helper mutations independent from revision bumping', () => {
31+
const manager = new ContextManager();
32+
const context = manager.createContext(makeTask({ id: 't1' }));
33+
34+
const changed = updateFilesystemFactsFromToolResult(context.facts, 'search_code', {}, {
35+
success: true,
36+
files: ['src/app.ts'],
37+
});
38+
39+
expect(changed).toBe(true);
40+
expect(context.facts.filesystem.existingFiles.has('src/app.ts')).toBe(true);
41+
expect(context.facts.filesystem.existingDirectories.has('src')).toBe(true);
42+
expect(context.facts.revision).toBe(0);
43+
});
44+
```
45+
46+
- [ ] **Step 2: Run test to verify RED**
47+
48+
Run:
49+
50+
```bash
51+
pnpm --filter @frontagent/core test -- src/context/context-manager.test.ts
52+
```
53+
54+
Expected: FAIL because `./filesystem-facts-update.js` does not exist yet.
55+
56+
### Task 2: Extract Filesystem Facts Helper
57+
58+
**Files:**
59+
- Create: `packages/core/src/context/filesystem-facts-update.ts`
60+
- Modify: `packages/core/src/context/context-manager.ts`
61+
62+
- [ ] **Step 1: Create the helper module**
63+
64+
Move the existing low-level filesystem fact mutation logic into:
65+
66+
```ts
67+
export function updateFilesystemFactsFromToolResult(
68+
facts: ProjectFacts,
69+
toolName: string,
70+
params: Record<string, unknown>,
71+
result: { success?: boolean; error?: string; [key: string]: unknown },
72+
): boolean
73+
```
74+
75+
Use helper-local `addToSet`, `removeFromSet`, and `setStringArrayMap` functions with the same semantics currently used by `ContextManager.updateFileSystemFacts`.
76+
77+
- [ ] **Step 2: Keep the ContextManager facade**
78+
79+
Replace the body of `ContextManager.updateFileSystemFacts` after context lookup with:
80+
81+
```ts
82+
if (updateFilesystemFactsFromToolResult(context.facts, toolName, params, result)) {
83+
this.bumpFactsRevision(context.facts);
84+
}
85+
```
86+
87+
Do not rename `ContextManager.updateFileSystemFacts` and do not change its parameters or return type.
88+
89+
- [ ] **Step 3: Run focused tests**
90+
91+
Run:
92+
93+
```bash
94+
pnpm --filter @frontagent/core test -- src/context/context-manager.test.ts
95+
```
96+
97+
Expected: PASS.
98+
99+
### Task 3: Verification and PR Prep
100+
101+
**Files:**
102+
- Verify all changed files.
103+
104+
- [ ] **Step 1: Run required checks**
105+
106+
Run:
107+
108+
```bash
109+
pnpm --filter @frontagent/core typecheck
110+
npx gitnexus detect_changes --scope all --repo FrontAgent
111+
```
112+
113+
Expected: typecheck exits 0; `detect_changes` reports only the plan, context manager facade, helper module, and focused test scope.
114+
115+
- [ ] **Step 2: Run broader gate if needed**
116+
117+
Because this touches core context fact propagation, run:
118+
119+
```bash
120+
pnpm quality:precommit
121+
```
122+
123+
Expected: exits 0.
124+
125+
- [ ] **Step 3: Open PR**
126+
127+
Push `improve/contextmanager-filesystem-facts` and open a PR to `develop` with `Closes #233` and a GitNexus Impact Summary containing:
128+
129+
```md
130+
Risk level:
131+
Critical skeleton changes:
132+
GitNexus impact:
133+
Verification:
134+
```
135+
136+
Use `detect_changes` with an underscore in the PR body.

‎packages/core/src/context/context-manager.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
mergeProjectFactsUpdate,
88
projectFactsFromSnapshot,
99
} from './facts-merge-helpers.js';
10+
import { updateFilesystemFactsFromToolResult } from './filesystem-facts-update.js';
1011

1112
function makeTask(overrides: Partial<AgentTask> = {}): AgentTask {
1213
return {
@@ -310,6 +311,26 @@ describe('ContextManager', () => {
310311
});
311312

312313
describe('updateFileSystemFacts', () => {
314+
it('keeps filesystem helper mutations independent from revision bumping', () => {
315+
const manager = new ContextManager();
316+
const context = manager.createContext(makeTask({ id: 't1' }));
317+
318+
const changed = updateFilesystemFactsFromToolResult(
319+
context.facts,
320+
'search_code',
321+
{},
322+
{
323+
success: true,
324+
files: ['src/app.ts'],
325+
},
326+
);
327+
328+
expect(changed).toBe(true);
329+
expect(context.facts.filesystem.existingFiles.has('src/app.ts')).toBe(true);
330+
expect(context.facts.filesystem.existingDirectories.has('src')).toBe(true);
331+
expect(context.facts.revision).toBe(0);
332+
});
333+
313334
it('records existing file on create_file success', () => {
314335
const manager = new ContextManager();
315336
manager.createContext(makeTask({ id: 't1' }));

‎packages/core/src/context/context-manager.ts‎

Lines changed: 4 additions & 181 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,8 @@ import {
1515
mergeProjectFactsUpdate,
1616
projectFactsFromSnapshot,
1717
} from './facts-merge-helpers.js';
18-
import {
19-
formatFilesenseNavigationContext,
20-
normalizeFilesenseNavigation,
21-
parentDirectoriesForPath,
22-
} from './helpers.js';
18+
import { updateFilesystemFactsFromToolResult } from './filesystem-facts-update.js';
19+
import { formatFilesenseNavigationContext, normalizeFilesenseNavigation } from './helpers.js';
2320
import { updateModuleDependencyGraphFromToolResult } from './module-dependency-graph.js';
2421

2522
/**
@@ -91,19 +88,6 @@ export class ContextManager {
9188
return set.delete(value);
9289
}
9390

94-
private setStringArrayMap(map: Map<string, string[]>, key: string, value: string[]): boolean {
95-
const previous = map.get(key);
96-
if (
97-
previous &&
98-
previous.length === value.length &&
99-
previous.every((item, idx) => item === value[idx])
100-
) {
101-
return false;
102-
}
103-
map.set(key, value);
104-
return true;
105-
}
106-
10791
/**
10892
* 更新执行计划
10993
*/
@@ -326,169 +310,8 @@ export class ContextManager {
326310
const context = this.contexts.get(taskId);
327311
if (!context) return;
328312

329-
const { facts } = context;
330-
let changed = false;
331-
332-
// Handle filesense navigation results - consume explicit factsDelta instead of guessing result shape.
333-
if (toolName.startsWith('filesense_')) {
334-
const data = result.data as
335-
| { factsDelta?: { existingFiles?: string[]; existingDirectories?: string[] } }
336-
| undefined;
337-
const factsDelta = data?.factsDelta;
338-
if (result.success && factsDelta) {
339-
for (const file of factsDelta.existingFiles ?? []) {
340-
changed = this.addToSet(facts.filesystem.existingFiles, file) || changed;
341-
changed = this.removeFromSet(facts.filesystem.nonExistentPaths, file) || changed;
342-
}
343-
for (const dir of factsDelta.existingDirectories ?? []) {
344-
changed = this.addToSet(facts.filesystem.existingDirectories, dir) || changed;
345-
changed = this.removeFromSet(facts.filesystem.nonExistentPaths, dir) || changed;
346-
}
347-
}
348-
}
349-
350-
// Handle filesense tool results - enrich ProjectFacts from index data
351-
if (
352-
toolName === 'filesense_sync' ||
353-
toolName === 'filesense_sync_and_summarize' ||
354-
toolName === 'filesense_query'
355-
) {
356-
if (result.success && result.data) {
357-
const data = result.data as Record<string, unknown>;
358-
359-
// For query results, extract file/directory existence from the index
360-
const index = (data.index ?? (data as { sync?: unknown }).sync) as
361-
| { children?: Array<{ name: string; path: string; type: string }> }
362-
| undefined;
363-
if (index?.children) {
364-
for (const child of index.children) {
365-
if (child.type === 'file') {
366-
changed = this.addToSet(facts.filesystem.existingFiles, child.path) || changed;
367-
changed =
368-
this.removeFromSet(facts.filesystem.nonExistentPaths, child.path) || changed;
369-
} else if (child.type === 'dir') {
370-
changed = this.addToSet(facts.filesystem.existingDirectories, child.path) || changed;
371-
changed =
372-
this.removeFromSet(facts.filesystem.nonExistentPaths, child.path) || changed;
373-
}
374-
}
375-
}
376-
377-
// For sync results, mark the root as existing directory
378-
const root = data.root as string | undefined;
379-
if (root) {
380-
changed = this.addToSet(facts.filesystem.existingDirectories, root) || changed;
381-
}
382-
}
383-
}
384-
385-
switch (toolName) {
386-
case 'create_file':
387-
case 'apply_patch': {
388-
const path = params.path as string;
389-
if (result.success) {
390-
changed = this.addToSet(facts.filesystem.existingFiles, path) || changed;
391-
changed = this.removeFromSet(facts.filesystem.nonExistentPaths, path) || changed;
392-
} else if (result.error?.includes('not found')) {
393-
changed = this.addToSet(facts.filesystem.nonExistentPaths, path) || changed;
394-
}
395-
break;
396-
}
397-
case 'read_file': {
398-
const path = params.path as string;
399-
// 🔧 修复：检查 skipped 和 exists 字段，正确记录不存在的文件
400-
if (result.success && !result.skipped) {
401-
// 真正成功读取了文件
402-
changed = this.addToSet(facts.filesystem.existingFiles, path) || changed;
403-
changed = this.removeFromSet(facts.filesystem.nonExistentPaths, path) || changed;
404-
} else if (result.skipped && result.exists === false) {
405-
// 步骤被跳过且文件不存在
406-
changed = this.addToSet(facts.filesystem.nonExistentPaths, path) || changed;
407-
changed = this.removeFromSet(facts.filesystem.existingFiles, path) || changed;
408-
} else if (
409-
result.error?.includes('not found') ||
410-
result.error?.includes('does not exist')
411-
) {
412-
// 明确的文件不存在错误
413-
changed = this.addToSet(facts.filesystem.nonExistentPaths, path) || changed;
414-
changed = this.removeFromSet(facts.filesystem.existingFiles, path) || changed;
415-
}
416-
break;
417-
}
418-
case 'list_directory': {
419-
const path = params.path as string;
420-
// 🔧 修复：同样检查 skipped 字段
421-
if (result.success && !result.skipped && Array.isArray(result.entries)) {
422-
changed = this.addToSet(facts.filesystem.existingDirectories, path) || changed;
423-
424-
// 🔧 关键修复：从目录内容推断文件存在性
425-
// list_directory 返回的 entries 是 FileInfo[] 对象数组
426-
// FileInfo = { name: string, path: string, type: 'file' | 'directory', size?, modifiedAt? }
427-
const entries = result.entries as Array<{ name: string; path: string; type: string }>;
428-
429-
// 存储路径字符串用于 directoryContents
430-
changed =
431-
this.setStringArrayMap(
432-
facts.filesystem.directoryContents,
433-
path,
434-
entries.map((e) => e.path),
435-
) || changed;
436-
437-
// 将目录中的文件/子目录添加到相应的集合
438-
for (const entry of entries) {
439-
if (entry.type === 'file') {
440-
changed = this.addToSet(facts.filesystem.existingFiles, entry.path) || changed;
441-
changed =
442-
this.removeFromSet(facts.filesystem.nonExistentPaths, entry.path) || changed;
443-
} else if (entry.type === 'directory') {
444-
changed = this.addToSet(facts.filesystem.existingDirectories, entry.path) || changed;
445-
changed =
446-
this.removeFromSet(facts.filesystem.nonExistentPaths, entry.path) || changed;
447-
}
448-
}
449-
} else if (result.skipped || result.error?.includes('not found')) {
450-
changed = this.addToSet(facts.filesystem.nonExistentPaths, path) || changed;
451-
}
452-
break;
453-
}
454-
case 'search_code': {
455-
if (!result.success) {
456-
break;
457-
}
458-
459-
const files = new Set<string>();
460-
461-
if (Array.isArray(result.files)) {
462-
for (const file of result.files) {
463-
if (typeof file === 'string') {
464-
files.add(file);
465-
}
466-
}
467-
}
468-
469-
if (Array.isArray(result.matches)) {
470-
for (const match of result.matches as Array<{ file?: unknown }>) {
471-
if (typeof match.file === 'string') {
472-
files.add(match.file);
473-
}
474-
}
475-
}
476-
477-
for (const file of files) {
478-
changed = this.addToSet(facts.filesystem.existingFiles, file) || changed;
479-
changed = this.removeFromSet(facts.filesystem.nonExistentPaths, file) || changed;
480-
481-
for (const parent of parentDirectoriesForPath(file)) {
482-
changed = this.addToSet(facts.filesystem.existingDirectories, parent) || changed;
483-
changed = this.removeFromSet(facts.filesystem.nonExistentPaths, parent) || changed;
484-
}
485-
}
486-
break;
487-
}
488-
}
489-
490-
if (changed) {
491-
this.bumpFactsRevision(facts);
313+
if (updateFilesystemFactsFromToolResult(context.facts, toolName, params, result)) {
314+
this.bumpFactsRevision(context.facts);
492315
}
493316
}
494317

0 commit comments

Comments
 (0)