diff --git a/.github/workflows/windows-ast-read-memory.yml b/.github/workflows/windows-ast-read-memory.yml new file mode 100644 index 0000000000..b2e2f7e92f --- /dev/null +++ b/.github/workflows/windows-ast-read-memory.yml @@ -0,0 +1,65 @@ +name: 'Windows AST Read Memory (issue #3232)' + +on: + pull_request: + paths: + - 'packages/tools/src/tools/ast-edit/**' + - 'packages/tools/src/tools/ast-edit.ts' + - 'packages/tools/src/acquisition/**' + - 'packages/tools/src/utils/ast-grep-utils.ts' + - '.bun-version' + - '.github/workflows/windows-ast-read-memory.yml' + push: + branches: + - 'main' + paths: + - 'packages/tools/src/tools/ast-edit/**' + - 'packages/tools/src/tools/ast-edit.ts' + - 'packages/tools/src/acquisition/**' + - 'packages/tools/src/utils/ast-grep-utils.ts' + - '.bun-version' + - '.github/workflows/windows-ast-read-memory.yml' + workflow_dispatch: + +# Cancel superseded runs of this workflow for the same ref so repeated pushes +# to a PR do not queue redundant Windows jobs. +concurrency: + group: '${{ github.workflow }}-${{ github.ref }}' + cancel-in-progress: true + +jobs: + windows-ast-read-memory: + name: 'Bun ast_read_file memory regression on Windows' + runs-on: 'windows-latest' + timeout-minutes: 20 + permissions: + contents: 'read' + steps: + - name: 'Checkout' + uses: 'actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1' # ratchet:actions/checkout@v7 + with: + fetch-depth: 1 + # The job only reads repository content; retained credentials would + # be an unnecessary secret surface for the spawned test children. + persist-credentials: false + + - name: 'Setup Bun' + uses: 'oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6' # ratchet:oven-sh/setup-bun@v2 + with: + bun-version-file: '.bun-version' + + # Plain `bun install` (NOT --frozen-lockfile): the monorepo lockfile is + # structurally unusable with --frozen-lockfile under Bun re-normalization + # (see AGENTS notes); plain install against the committed lockfile is + # deterministic and sufficient for running the test suite. + - name: 'Install dependencies' + run: 'bun install' + + # Cross-platform memory regression for ast_read_file: a child Bun + # process executes the real tool against a generated Git workspace that + # previously triggered the multi-symbol native findInFiles fan-out, + # samples peak RSS conservatively, and proves the child drains and + # exits. The non-Windows path is covered by the ordinary packages/tools + # Bun test suite in CI, which includes the same test file. + - name: 'Run ast_read_file memory regression (Bun)' + run: 'bun test packages/tools/src/tools/ast-edit/__tests__/ast-read-memory.bun.test.ts' diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-query-extractor-fallback.bun.test.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-query-extractor-fallback.bun.test.ts new file mode 100644 index 0000000000..65109a51ab --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-query-extractor-fallback.bun.test.ts @@ -0,0 +1,150 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Behavioral tests for the ASTQueryExtractor line-scan fallback and its + * bounded variant (issue #3232 remediation). The fallback is reached for + * languages with an ast-grep mapping but no declaration family (ruby, go, + * java, cpp, html, css, json) and whenever a native parse throws. + */ + +import { describe, it, expect } from 'bun:test'; +import { ASTQueryExtractor } from '../ast-query-extractor.js'; + +const extractor = new ASTQueryExtractor(); + +/** Indented fallback-declaration source with known name columns. */ +const INDENTED_SOURCE = [ + 'module Wrapper', + ' class Service', + ' def run', + ' end', + ' end', + 'end', +].join('\n'); + +/** C-shaped source whose declarations only exist in the C family mapping. */ +const C_SHAPED_SOURCE = 'struct Point {\n int x;\n};\n'; + +describe('fallback declaration extraction columns', () => { + it('computes declaration columns against the raw line so indentation is kept', async () => { + const declarations = await extractor.extractDeclarations( + '/repo/wrapper.rb', + INDENTED_SOURCE, + ); + const service = declarations.find((decl) => decl.name === 'Service'); + const run = declarations.find((decl) => decl.name === 'run'); + expect(service?.line).toBe(2); + // ' class Service' → 'Service' starts at raw column 8, not 6. + expect(service?.column).toBe(8); + expect(run?.line).toBe(3); + // ' def run' → 'run' starts at raw column 8, not 4. + expect(run?.column).toBe(8); + expect(service?.range.start.column).toBe(8); + expect(service?.range.end.column).toBe(8 + 'Service'.length); + }); + + it('keeps raw-line columns in the bounded fallback scan', async () => { + const declarations = await extractor.extractDeclarationsBounded( + '/repo/wrapper.rb', + INDENTED_SOURCE, + 10, + ); + const service = declarations.find((decl) => decl.name === 'Service'); + expect(service?.column).toBe(8); + }); +}); + +describe('bounded fallback limit validation', () => { + it('rejects a NaN limit instead of scanning unboundedly', async () => { + await expect( + extractor.extractDeclarationsBounded( + '/repo/wrapper.rb', + INDENTED_SOURCE, + Number.NaN, + ), + ).rejects.toThrow(/limit/); + }); + + it('still permits a positive-infinity limit for the unbounded legacy path', async () => { + const declarations = await extractor.extractDeclarationsBounded( + '/repo/wrapper.rb', + INDENTED_SOURCE, + Number.POSITIVE_INFINITY, + ); + expect(declarations.map((decl) => decl.name)).toEqual(['Service', 'run']); + }); + + it('returns no declarations for a zero limit', async () => { + const declarations = await extractor.extractDeclarationsBounded( + '/repo/wrapper.rb', + INDENTED_SOURCE, + 0, + ); + expect(declarations).toEqual([]); + }); + + it('stops at the limit in document order with output identical to the unbounded scan', async () => { + // A declaration-dense fallback file: the bounded scan must return + // exactly the first declarations of the unbounded scan (names, lines, + // columns, ranges) and never materialize past the limit. + const lines = Array.from({ length: 2000 }, (_, i) => ` def method${i}`); + const source = lines.join('\n'); + const bounded = await extractor.extractDeclarationsBounded( + '/repo/dense.rb', + source, + 3, + ); + const unbounded = await extractor.extractDeclarations( + '/repo/dense.rb', + source, + ); + expect(bounded).toEqual(unbounded.slice(0, 3)); + expect(bounded.map((decl) => decl.name)).toEqual([ + 'method0', + 'method1', + 'method2', + ]); + // Raw-line column is preserved: ' def method0' puts the name at column 6. + expect(bounded.every((decl) => decl.column === 6)).toBe(true); + }); +}); + +describe('declaration family resolution', () => { + it('extracts C declarations for .c but does not default unmapped .cpp to the C family', async () => { + const cDeclarations = await extractor.extractDeclarations( + '/repo/point.c', + C_SHAPED_SOURCE, + ); + expect( + cDeclarations.some( + (decl) => decl.name === 'Point' && decl.type === 'struct', + ), + ).toBe(true); + + // '.cpp' has an ast-grep mapping but no declaration family: the same + // source must go through the keyword line scan (no 'struct' keyword), + // never be silently interpreted with C declaration kinds. + const cppDeclarations = await extractor.extractDeclarations( + '/repo/point.cpp', + C_SHAPED_SOURCE, + ); + expect(cppDeclarations).toEqual([]); + }); + + it('uses the same family resolution in the bounded walk', async () => { + const cDeclarations = await extractor.extractDeclarationsBounded( + '/repo/point.c', + C_SHAPED_SOURCE, + 5, + ); + expect(cDeclarations.map((decl) => decl.name)).toEqual(['Point']); + const cppDeclarations = await extractor.extractDeclarationsBounded( + '/repo/point.cpp', + C_SHAPED_SOURCE, + 5, + ); + expect(cppDeclarations).toEqual([]); + }); +}); diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-acquire.bun.test.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-acquire.bun.test.ts new file mode 100644 index 0000000000..0b03e63d5c --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-acquire.bun.test.ts @@ -0,0 +1,932 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Behavioral tests for bounded ast_read_file acquisition policies (issue #3232). + * Covers REQ-3232-1/2/3/4 discovery, file-count, aggregate-byte, growth, declaration, + * precedence, skipped-only, cancellation, and max-in-flight behavior. + * + * All fixtures are real temporary directories with real Git state, real files, + * and the real ASTReadFileTool / collector / providers. No mocking of the + * component under test: the only wrappers are real subclasses whose public + * behavior delegates to the real implementation. + */ + +import { describe, it, expect, beforeEach } from 'bun:test'; +import { writeFileSync, mkdirSync, rmSync, appendFileSync } from 'node:fs'; +import { join, sep as pathSep } from 'node:path'; +import { ASTEditTool } from '../../ast-edit.js'; +import { ASTContextCollector } from '../context-collector.js'; +import { RepositoryContextProvider } from '../repository-context-provider.js'; +import type { WorkingSetDiscoveryResult } from '../repository-context-provider.js'; +import { + createFakeToolHost, + createTempDir, + useTempDir, +} from './test-helpers.js'; +import { + runRead, + acquireWorkingSet, + gitCheck, + gitInit, + gitCommitAll, + declarationsBody, + paddedDeclarations, + seedAndModify, + writeTarget, + simpleModifiedEntries, + hasCaseInsensitiveFilenames, + createLongPathCandidates, + ObservingExtractor, + writeTrackedModifiedTarget, + MAX_WORKING_SET_FILES, + MAX_WORKING_SET_DECLARATIONS, + WORKING_SET_BYTE_BUDGET, + DISCOVERY_CANDIDATE_CAP, + READ_SENTINEL_BYTES, +} from './ast-read-file-bounded-helpers.js'; +import type { ConnectedFile } from '../types.js'; +// --------------------------------------------------------------------------- +// REQ-3232-1: repository relationship analysis is gone from the read path. +// --------------------------------------------------------------------------- + +describe('REQ-3232-1: enhanced context repository opt-out', () => { + const ctx = useTempDir(); + let target = ''; + + beforeEach(() => { + gitInit(ctx.tempDir); + // A committed dependency file referencing the target symbols: repository + // analysis would eagerly discover relationships in it. + writeFileSync( + join(ctx.tempDir, 'dep.ts'), + 'import { Alpha } from "./target";\nexport function user(): Alpha { return null as Alpha; }\n', + 'utf-8', + ); + gitCommitAll(ctx.tempDir, 'init'); + target = join(ctx.tempDir, 'target.ts'); + writeFileSync( + target, + 'export class Alpha {\n public run(): void {}\n}\nexport function betaWorker(): number { return 1; }\n', + 'utf-8', + ); + }); + + it('collectEnhancedContext skips repository context when the caller opts out', async () => { + const collector = new ASTContextCollector(); + const content = 'export class Alpha {\n public run(): void {}\n}\n'; + const enhanced = await collector.collectEnhancedContext( + target, + content, + ctx.tempDir, + { collectRepositoryContext: false }, + ); + expect(enhanced.repositoryContext).toBeUndefined(); + expect(enhanced.relatedFiles).toBeUndefined(); + expect(enhanced.relatedSymbols).toBeUndefined(); + // Local analysis and snippets are preserved. + expect(enhanced.declarations.length).toBeGreaterThan(0); + expect(enhanced.relevantSnippets).toBeDefined(); + }); + + it('collectEnhancedContext still collects repository context by default', async () => { + const collector = new ASTContextCollector(); + const content = 'export class Alpha {\n public run(): void {}\n}\n'; + const enhanced = await collector.collectEnhancedContext( + target, + content, + ctx.tempDir, + ); + expect(enhanced.repositoryContext).toBeDefined(); + expect(enhanced.repositoryContext?.rootPath).toBe(ctx.tempDir); + }); + + it('ast_read_file keeps local context and working set while opting out', async () => { + seedAndModify(ctx.tempDir, [ + { + name: 'other.ts', + seed: 'export function helper(): void {}\n', + modified: 'export function helper(): void {\n return;\n}\n', + }, + ]); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(true); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('LLXPRT READ: '); + expect(output).toContain('CONTEXT ANALYSIS:'); + expect(output).toContain('RELEVANT SNIPPETS:'); + expect(output).toContain('WORKING SET CONTEXT:'); + expect(output).toContain('other.ts'); + }); + + it('ast_edit preview still renders repository context', async () => { + const tool = new ASTEditTool(createFakeToolHost(ctx.tempDir)); + const result = await tool + .build({ + file_path: target, + old_string: 'public run(): void {}', + new_string: 'public runFast(): void {}', + force: false, + }) + .execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + expect(String(result.llmContent)).toContain('- Repository:'); + }); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-2: bounded Git discovery (finite count + one-over sentinel). +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: bounded working-set Git discovery', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + it('caps discovery at a finite candidate count with a one-over sentinel', async () => { + seedAndModify( + ctx.tempDir, + simpleModifiedEntries(DISCOVERY_CANDIDATE_CAP + 9, 'dc'), + ); + const target = writeTarget(ctx.tempDir); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: DISCOVERY_CANDIDATE_CAP, + excludePath: target, + }, + ); + expect(discovery.candidates).toHaveLength(DISCOVERY_CANDIDATE_CAP); + expect(discovery.outcome).toBe('truncated'); + }); + + // Seeding 3000 long-path files and committing them dominates the runtime; + // the discovery run itself stays bounded by the provider's Git timeout. + it('never exceeds the candidate cap when buffered stdout trails the kill', async () => { + // The listing emits well over a megabyte of NUL-delimited names, so git + // keeps writing buffered stdout after the cap is hit and the exact child + // is killed. Those late events must never add a candidate beyond the cap. + createLongPathCandidates(ctx.tempDir, 3000); + const target = writeTarget(ctx.tempDir); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: 3, + excludePath: target, + }, + ); + expect(discovery.candidates).toHaveLength(3); + expect(discovery.outcome).toBe('truncated'); + }, 120_000); + + it('reports aborted when the signal fires before a phase attaches its listener', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(3, 'aa')); + const target = writeTarget(ctx.tempDir); + const controller = new AbortController(); + const pending = new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: DISCOVERY_CANDIDATE_CAP, + excludePath: target, + signal: controller.signal, + }, + ); + // The abort lands after the entry check ran synchronously but before the + // first Git child attaches its abort listener: a listener added to an + // already-aborted signal never fires, so discovery must check the flag + // itself instead of relying on the event. + controller.abort(); + const discovery: WorkingSetDiscoveryResult = await pending; + expect(discovery.outcome).toBe('aborted'); + expect(discovery.candidates).toHaveLength(0); + }); + + it('reports a Git discovery error, not a one-over claim, when listing output overflows', async () => { + // No candidate cap is hit: the bound that stops this run is the finite + // output allowance of the listing itself. Reporting that as candidate + // truncation would claim "at least N eligible files", which was never + // observed; it is a Git/discovery failure instead. + const names = createLongPathCandidates(ctx.tempDir, 3000); + const target = writeTarget(ctx.tempDir); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: names.length, + excludePath: target, + }, + ); + expect(discovery.outcome).toBe('git-error'); + expect(discovery.gitError).toBeDefined(); + expect(String(discovery.gitError)).toContain('output'); + }, 120_000); // Same 3000-file long-path fixture: seeding dominates the runtime. + + it('excludes the read target under case-insensitive path semantics', async () => { + if (!hasCaseInsensitiveFilenames(ctx.tempDir)) { + return; + } + // Git reports the tracked name with its literal case while the caller + // may hold an equivalently-spelled path with different casing. On a + // case-insensitive filesystem those are the same file and must exclude. + // The target is tracked and modified so the diff genuinely lists it. + seedAndModify(ctx.tempDir, simpleModifiedEntries(1, 'ci')); + const target = writeTrackedModifiedTarget(ctx.tempDir); + const cased = join( + ctx.tempDir, + target.split(pathSep).pop()?.toUpperCase() ?? 'TARGET.TS', + ); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: DISCOVERY_CANDIDATE_CAP, + excludePath: cased, + }, + ); + expect(discovery.outcome).toBe('complete'); + expect( + discovery.candidates.some((candidate) => candidate.endsWith('target.ts')), + ).toBe(false); + // Exclusion removed only the target: the other candidate is retained. + expect( + discovery.candidates.some((candidate) => candidate.endsWith('ci000.ts')), + ).toBe(true); + }); + it('reports a below-cap working set as a complete discovery', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(30, 'dc')); + const target = writeTarget(ctx.tempDir); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: DISCOVERY_CANDIDATE_CAP, + excludePath: target, + }, + ); + expect(discovery.candidates).toHaveLength(30); + expect(discovery.outcome).toBe('complete'); + }); + + it('dedupes candidates across Git phases and honors the exclude path', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(1, 'dd')); + // Stage the modified file so the staged phase lists it too, while the + // recent-commit phase lists it from the seed commit. + gitCheck(ctx.tempDir, ['add', 'dd000.ts']); + // The target is tracked and modified, so the unstaged diff genuinely + // lists it as a candidate that the exclude path must remove. + const target = writeTrackedModifiedTarget(ctx.tempDir); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: DISCOVERY_CANDIDATE_CAP, + excludePath: target, + }, + ); + expect(discovery.outcome).toBe('complete'); + expect(discovery.candidates).toHaveLength(1); + expect(discovery.candidates[0]).toBe(join(ctx.tempDir, 'dd000.ts')); + }); + + it('handles paths with spaces and newlines via NUL-delimited Git output', async () => { + // Windows filenames cannot contain a newline, so the literal-newline half + // of this coverage is POSIX-only; the space-path half must run everywhere. + const weird = + process.platform === 'win32' + ? 'weird name with space.ts' + : 'weird\nname with space.ts'; + seedAndModify(ctx.tempDir, [ + { + name: weird, + seed: declarationsBody(1, 'ws_'), + modified: declarationsBody(2, 'wm_'), + }, + ]); + const target = writeTarget(ctx.tempDir); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect( + acquisition.files.some( + (f: ConnectedFile) => f.filePath === join(ctx.tempDir, weird), + ), + ).toBe(true); + }); + + it.skipIf(process.platform === 'win32')( + 'keeps NUL-delimited discovery correct for a literal newline filename', + async () => { + // Only POSIX filesystems permit a newline inside a filename; the point + // of this fixture is that newline-delimited parsing would split the + // name in two while NUL-delimited parsing keeps it whole. + const withNewline = 'split\nname.ts'; + seedAndModify(ctx.tempDir, [ + { + name: withNewline, + seed: declarationsBody(1, 'nl_'), + modified: declarationsBody(2, 'nm_'), + }, + ]); + const target = writeTarget(ctx.tempDir); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { maxCandidates: DISCOVERY_CANDIDATE_CAP, excludePath: target }, + ); + expect(discovery.outcome).toBe('complete'); + expect(discovery.candidates).toEqual([join(ctx.tempDir, withNewline)]); + }, + ); + + it('reports a corrupted repository as a Git error, not an empty complete set', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(2, 'dc')); + const target = writeTarget(ctx.tempDir); + // Corrupt HEAD: the staged-diff phase needs it and must fail loudly. + writeFileSync(join(ctx.tempDir, '.git', 'HEAD'), 'garbage\n', 'utf-8'); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { + maxCandidates: DISCOVERY_CANDIDATE_CAP, + excludePath: target, + }, + ); + expect(discovery.outcome).toBe('git-error'); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('git-error'); + }); + + it('reports a directory outside any Git work tree as no working set', async () => { + const outside = createTempDir('llxprt-3232-nogit-'); + try { + writeFileSync( + join(outside.dir, 'plain.ts'), + 'export function one(): number { return 1; }\n', + 'utf-8', + ); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + outside.dir, + { maxCandidates: DISCOVERY_CANDIDATE_CAP }, + ); + expect(discovery.outcome).toBe('no-working-set'); + expect(discovery.candidates).toHaveLength(0); + } finally { + outside.cleanup(); + } + }); + + it('observes a fresh repository without commits as a complete empty discovery', async () => { + writeFileSync( + join(ctx.tempDir, 'plain.ts'), + 'export function one(): number { return 1; }\n', + 'utf-8', + ); + const discovery: WorkingSetDiscoveryResult = + await new RepositoryContextProvider().discoverWorkingSetFiles( + ctx.tempDir, + { maxCandidates: DISCOVERY_CANDIDATE_CAP }, + ); + expect(discovery.outcome).toBe('complete'); + expect(discovery.candidates).toHaveLength(0); + }); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-2: bounded working-set file-count policy. +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: bounded working-set file-count policy', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + it('reports a below-limit working set as complete with no partial marker', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(3, 'ws')); + const target = writeTarget(ctx.tempDir); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('WORKING SET CONTEXT:'); + expect(output).not.toContain('(partial'); + expect(output).toContain('ws000.ts'); + expect(output).toContain('ws002.ts'); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(true); + expect(acquisition.status.retainedFiles).toBe(3); + }); + + it('reports an exactly-at-limit working set as complete', async () => { + seedAndModify( + ctx.tempDir, + simpleModifiedEntries(MAX_WORKING_SET_FILES, 'ex'), + ); + const target = writeTarget(ctx.tempDir); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('WORKING SET CONTEXT:'); + expect(output).not.toContain('(partial'); + expect(output).toContain('ex049.ts'); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(true); + expect(acquisition.status.retainedFiles).toBe(MAX_WORKING_SET_FILES); + expect(acquisition.status.eligibleFiles).toBe(MAX_WORKING_SET_FILES); + expect(acquisition.status.traversalComplete).toBe(true); + }); + + it('marks one-over as partial with the file-count reason and never acquires the 51st file', async () => { + seedAndModify( + ctx.tempDir, + simpleModifiedEntries(MAX_WORKING_SET_FILES + 1, 'ov'), + ); + const target = writeTarget(ctx.tempDir); + const observing = new ObservingExtractor(); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('file-count'); + expect(acquisition.status.retainedFiles).toBe(MAX_WORKING_SET_FILES); + expect(acquisition.status.eligibleFiles).toBe(DISCOVERY_CANDIDATE_CAP); + expect(acquisition.status.traversalComplete).toBe(false); + // The 51st candidate was observed as the one-over sentinel but never + // read or parsed: exactly 50 real acquisitions happened. + expect(observing.extractionEnters).toHaveLength(MAX_WORKING_SET_FILES); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + const output = String(result.llmContent); + expect(output).toContain( + 'WORKING SET CONTEXT (partial: stopped at the file-count limit', + ); + expect(output).toContain('at least 51'); + expect(output).not.toContain('ov050.ts'); + expect(output).toContain('ov000.ts'); + }); + + it('bounds a far-over working set to observing only 51 candidates', async () => { + seedAndModify( + ctx.tempDir, + simpleModifiedEntries(MAX_WORKING_SET_FILES * 3, 'far'), + ); + const target = writeTarget(ctx.tempDir); + const observing = new ObservingExtractor(); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('file-count'); + expect(acquisition.status.retainedFiles).toBe(MAX_WORKING_SET_FILES); + // Discovery is bounded: only 51 of the 150 modified files are observed. + expect(acquisition.status.eligibleFiles).toBe(DISCOVERY_CANDIDATE_CAP); + expect(observing.extractionEnters).toHaveLength(MAX_WORKING_SET_FILES); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + const output = String(result.llmContent); + expect(output).toContain('at least 51'); + expect(output).not.toContain('far149.ts'); + expect(output).toContain('far000.ts'); + }); + + it('renders working-set files in deterministic sorted order', async () => { + // Genuinely mixed working-set sources whose Git enumeration order + // (unstaged diff, then staged diff, then recent-commit log) differs from + // sorted order: a-staged is modified-and-staged after its commit, so only + // the staged phase lists it; b-committed is a recent commit with an + // unstaged modification; c-unstaged is only ever unstaged. + writeFileSync( + join(ctx.tempDir, 'c-unstaged.ts'), + declarationsBody(1, 'c_'), + 'utf-8', + ); + writeFileSync( + join(ctx.tempDir, 'a-staged.ts'), + declarationsBody(1, 'a_'), + 'utf-8', + ); + gitCommitAll(ctx.tempDir, 'seed'); + writeFileSync( + join(ctx.tempDir, 'a-staged.ts'), + declarationsBody(2, 'a2_'), + 'utf-8', + ); + gitCheck(ctx.tempDir, ['add', 'a-staged.ts']); + writeFileSync( + join(ctx.tempDir, 'b-committed.ts'), + declarationsBody(1, 'b1_'), + 'utf-8', + ); + gitCommitAll(ctx.tempDir, 'add b'); + writeFileSync( + join(ctx.tempDir, 'b-committed.ts'), + declarationsBody(2, 'b2_'), + 'utf-8', + ); + writeFileSync( + join(ctx.tempDir, 'c-unstaged.ts'), + declarationsBody(2, 'c2_'), + 'utf-8', + ); + + const target = writeTarget(ctx.tempDir); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + const output = String(result.llmContent); + const a = output.indexOf('a-staged.ts'); + const b = output.indexOf('b-committed.ts'); + const c = output.indexOf('c-unstaged.ts'); + expect(a).toBeGreaterThanOrEqual(0); + expect(b).toBeGreaterThan(a); + expect(c).toBeGreaterThan(b); + }); + + it('retains a multi-chunk working set completely and in sorted order', async () => { + // Nine eligible candidates span three policy-sized planning/acquisition + // chunks: every candidate must still be planned exactly once and the + // retained order must stay sorted, proving the chunked planning of + // stats changes nothing observable. + seedAndModify(ctx.tempDir, simpleModifiedEntries(9, 'mc')); + const target = writeTarget(ctx.tempDir); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(true); + expect(acquisition.status.retainedFiles).toBe(9); + expect(acquisition.status.eligibleFiles).toBe(9); + const retainedNames = acquisition.files.map( + (file: ConnectedFile) => file.filePath, + ); + const expected = Array.from({ length: 9 }, (_, i) => + join(ctx.tempDir, `mc${String(i).padStart(3, '0')}.ts`), + ); + expect(retainedNames).toEqual(expected); + }); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-2: bounded working-set aggregate-byte policy. +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: bounded working-set aggregate-byte policy', () => { + const ctx = useTempDir(); + const quarter = WORKING_SET_BYTE_BUDGET / 4; + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + function sizedEntries( + names: readonly string[], + sizeBytes: number, + ): ReadonlyArray<{ name: string; seed: string; modified: string }> { + return names.map((name, i) => ({ + name, + seed: paddedDeclarations(1, `s${i}_`, sizeBytes), + modified: paddedDeclarations(1, `m${i}_`, sizeBytes), + })); + } + + it('reports an exactly-at-budget working set as complete', async () => { + seedAndModify( + ctx.tempDir, + sizedEntries(['big0.ts', 'big1.ts', 'big2.ts', 'big3.ts'], quarter), + ); + const target = writeTarget(ctx.tempDir); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('WORKING SET CONTEXT:'); + expect(output).not.toContain('(partial'); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(true); + expect(acquisition.status.retainedFiles).toBe(4); + expect(acquisition.status.retainedSourceBytes).toBe( + WORKING_SET_BYTE_BUDGET, + ); + }); + + it('marks one-over-budget as partial without reading the over file', async () => { + seedAndModify( + ctx.tempDir, + sizedEntries(['big0.ts', 'big1.ts', 'big2.ts', 'big3.ts'], quarter), + ); + seedAndModify(ctx.tempDir, [ + { + name: 'zz-extra.ts', + seed: declarationsBody(1, 'zs_'), + modified: declarationsBody(2, 'zm_'), + }, + ]); + const target = writeTarget(ctx.tempDir); + const observing = new ObservingExtractor(); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('source-bytes'); + expect(acquisition.status.retainedSourceBytes).toBe( + WORKING_SET_BYTE_BUDGET, + ); + expect(acquisition.status.retainedFiles).toBe(4); + // The authoritative stop happened before the over-budget file was read. + expect(observing.extractionEnters).toHaveLength(4); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + const output = String(result.llmContent); + expect(output).toContain( + 'WORKING SET CONTEXT (partial: stopped at the aggregate source-byte budget', + ); + expect(output).not.toContain('zz-extra.ts'); + }); + + it('bounds concurrent in-flight acquisition bytes to the aggregate budget', async () => { + // Four files each individually inside the budget but jointly ~3.5x over + // it: at most the first may be admitted, so concurrent materialization + // never approaches four independent full budgets. + const bigBytes = WORKING_SET_BYTE_BUDGET - 512 * 1024; + seedAndModify( + ctx.tempDir, + sizedEntries(['b0.ts', 'b1.ts', 'b2.ts', 'b3.ts'], bigBytes), + ); + const target = writeTarget(ctx.tempDir); + const observing = new ObservingExtractor({ delayMs: 25 }); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.partialReason).toBe('source-bytes'); + expect(acquisition.status.retainedFiles).toBe(1); + expect(observing.extractionEnters).toHaveLength(1); + expect(observing.peakActiveContentBytes).toBeLessThanOrEqual( + WORKING_SET_BYTE_BUDGET, + ); + }); + + it('skips an oversized single file and keeps the target read healthy', async () => { + seedAndModify( + ctx.tempDir, + sizedEntries(['huge.ts'], WORKING_SET_BYTE_BUDGET + 1024), + ); + seedAndModify(ctx.tempDir, [ + { + name: 'small-a.ts', + seed: declarationsBody(1, 'ss_'), + modified: declarationsBody(2, 'sm_'), + }, + ]); + const target = writeTarget(ctx.tempDir); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('WORKING SET CONTEXT (partial'); + expect(output).toContain('1 oversized'); + expect(output).not.toContain('huge.ts:'); + expect(output).toContain('small-a.ts'); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('skipped-files'); + expect(acquisition.status.oversizedFiles).toBe(1); + expect(acquisition.status.retainedFiles).toBe(1); + expect(acquisition.status.traversalComplete).toBe(true); + }); + + it('skips a working-set path that became a directory (cross-platform unreadable)', async () => { + // Deterministic platform-neutral unreadable case: the tracked path is + // replaced by a directory, so it exists but can never be read as a file. + writeFileSync( + join(ctx.tempDir, 'locked.ts'), + declarationsBody(1, 'lk_'), + 'utf-8', + ); + writeFileSync( + join(ctx.tempDir, 'open.ts'), + declarationsBody(1, 'op_'), + 'utf-8', + ); + gitCommitAll(ctx.tempDir, 'seed'); + rmSync(join(ctx.tempDir, 'locked.ts')); + mkdirSync(join(ctx.tempDir, 'locked.ts')); + writeFileSync( + join(ctx.tempDir, 'open.ts'), + declarationsBody(2, 'op2_'), + 'utf-8', + ); + const target = writeTarget(ctx.tempDir); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('WORKING SET CONTEXT (partial'); + expect(output).toContain('1 unreadable'); + expect(output).toContain('open.ts'); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('skipped-files'); + expect(acquisition.status.skippedFiles).toBe(1); + expect(acquisition.status.retainedFiles).toBe(1); + }); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-2: stat/read growth handling with bounded reads. +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: bounded reads handle files that grow after stat', () => { + const ctx = useTempDir(); + const SMALL_GROWTH_BYTES = 200; + const LARGE_GROWTH_BYTES = READ_SENTINEL_BYTES + 4096; + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + function setupGrowthFixture(): { + target: string; + readonly expectedRetainedBytes: number; + } { + const grownEntries = [ + { + name: 'g1.ts', + seed: paddedDeclarations(1, 'gs1_', 2048), + modified: paddedDeclarations(1, 'gm1_', 2048), + }, + { + name: 'g2.ts', + seed: paddedDeclarations(1, 'gs2_', 2048), + modified: paddedDeclarations(1, 'gm2_', 2048), + }, + ]; + const anchorEntries = [0, 1, 2, 3].map((i) => ({ + name: `a${i}.ts`, + seed: declarationsBody(1, `as${i}_`), + modified: declarationsBody(2, `am${i}_`), + })); + seedAndModify(ctx.tempDir, [...anchorEntries, ...grownEntries]); + const target = writeTarget(ctx.tempDir); + const anchorsBytes = anchorEntries.reduce( + (sum, entry) => sum + Buffer.byteLength(entry.modified), + 0, + ); + return { + target, + expectedRetainedBytes: anchorsBytes + (2048 + SMALL_GROWTH_BYTES) + 2048, + }; + } + + it('charges actual grown bytes when growth stays inside the read sentinel', async () => { + const fixture = setupGrowthFixture(); + const observing = new ObservingExtractor({ + onFirstExtraction: () => { + appendFileSync( + join(ctx.tempDir, 'g1.ts'), + 'x'.repeat(SMALL_GROWTH_BYTES), + ); + }, + }); + const acquisition = await acquireWorkingSet( + fixture.target, + ctx.tempDir, + observing, + ); + const retained = acquisition.files.map((f: ConnectedFile) => f.filePath); + expect(retained).toContain(join(ctx.tempDir, 'g1.ts')); + // The aggregate charge is the actual (grown) byte length, not the stale + // size captured before the concurrent growth. + expect(acquisition.status.retainedSourceBytes).toBe( + fixture.expectedRetainedBytes, + ); + expect(acquisition.status.complete).toBe(true); + }); + + it('skips a file that grows past its bounded read window as oversized', async () => { + const fixture = setupGrowthFixture(); + const observing = new ObservingExtractor({ + onFirstExtraction: () => { + appendFileSync( + join(ctx.tempDir, 'g2.ts'), + 'x'.repeat(LARGE_GROWTH_BYTES), + ); + }, + }); + const acquisition = await acquireWorkingSet( + fixture.target, + ctx.tempDir, + observing, + ); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('skipped-files'); + expect(acquisition.status.oversizedFiles).toBe(1); + expect( + acquisition.files.some((f: ConnectedFile) => + f.filePath.endsWith('g2.ts'), + ), + ).toBe(false); + expect( + acquisition.files.some((f: ConnectedFile) => + f.filePath.endsWith('g1.ts'), + ), + ).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-2: retained-declaration policy with bounded extraction. +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: bounded working-set retained-declaration policy', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + function setup(count: number, perFile: number, prefix = 'd'): string { + seedAndModify( + ctx.tempDir, + Array.from({ length: count }, (_, i) => { + const name = `${prefix}${String(i).padStart(3, '0')}.ts`; + return { + name, + seed: declarationsBody(perFile - 1, `s${i}_`), + modified: declarationsBody(perFile, `m${i}_`), + }; + }), + ); + return writeTarget(ctx.tempDir); + } + + it('reports exactly-at-limit retained declarations as complete', async () => { + const target = setup(MAX_WORKING_SET_DECLARATIONS / 20, 20); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + expect(String(result.llmContent)).not.toContain('(partial'); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(true); + expect(acquisition.status.retainedDeclarations).toBe( + MAX_WORKING_SET_DECLARATIONS, + ); + }); + + it('marks one-over retained declarations as partial', async () => { + const target = setup(26, 20); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain( + 'WORKING SET CONTEXT (partial: stopped at the retained-declaration limit', + ); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('declarations'); + expect(acquisition.status.retainedDeclarations).toBe( + MAX_WORKING_SET_DECLARATIONS, + ); + expect(acquisition.status.retainedFiles).toBe(25); + expect(output).not.toContain('d025.ts'); + }); + + it('observes the true 501st declaration as the one-over sentinel', async () => { + // 25 files x 20 declarations = exactly 500; the final candidate holds + // the literal 501st declaration and must not be retained. The sentinel + // sorts after all d-prefixed fixtures so it is the last chunk acquired. + const target = setup(25, 20, 'x'); + seedAndModify(ctx.tempDir, [ + { + name: 'zz-last.ts', + seed: 'export const last = 1;\n', + modified: declarationsBody(1, 'last_'), + }, + ]); + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.partialReason).toBe('declarations'); + expect(acquisition.status.retainedDeclarations).toBe( + MAX_WORKING_SET_DECLARATIONS, + ); + expect(acquisition.status.retainedFiles).toBe(25); + expect( + acquisition.files.some((f: ConnectedFile) => + f.filePath.endsWith('zz-last.ts'), + ), + ).toBe(false); + }); + + it('acquires at most remaining+1 declarations from a declaration-dense file', async () => { + // One first candidate with 3000 declarations: the bounded extractor may + // materialize at most 501 (one-over sentinel), never the full array. + const dense = declarationsBody(3000, 'dense_'); + seedAndModify(ctx.tempDir, [ + { name: 'dense.ts', seed: dense, modified: dense }, + ]); + const target = writeTarget(ctx.tempDir); + + const observing = new ObservingExtractor(); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.partialReason).toBe('declarations'); + expect(acquisition.status.retainedDeclarations).toBe(0); + expect(acquisition.status.retainedFiles).toBe(0); + expect(observing.boundedLengths).toEqual([ + MAX_WORKING_SET_DECLARATIONS + 1, + ]); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + const output = String(result.llmContent); + expect(output).toContain('stopped at the retained-declaration limit (500)'); + expect(output).not.toContain('dense.ts:'); + }); +}); diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-cancel.bun.test.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-cancel.bun.test.ts new file mode 100644 index 0000000000..f3e6a9d6ee --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-cancel.bun.test.ts @@ -0,0 +1,462 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Behavioral tests for ast_read_file cancellation, max-in-flight observation, + * and skipped-only/failed contexts (issue #3232, REQ-3232-3/4, Finding 8). + */ + +import { describe, it, expect, beforeEach } from 'bun:test'; +import { writeFileSync, rmSync, chmodSync, mkdirSync } from 'node:fs'; +import { join } from 'node:path'; +import { ASTQueryExtractor } from '../ast-query-extractor.js'; +import { ASTContextCollector } from '../context-collector.js'; +import { ASTReadFileToolInvocation } from '../ast-read-file-invocation.js'; +import { RepositoryContextProvider } from '../repository-context-provider.js'; +import { enrichWithWorkingSetContext } from '../workspace-context-provider.js'; +import { createFakeToolHost, useTempDir } from './test-helpers.js'; +import { + runRead, + acquireWorkingSet, + gitInit, + gitCommitAll, + declarationsBody, + paddedDeclarations, + seedAndModify, + writeTarget, + simpleModifiedEntries, + ObservingExtractor, + MAX_WORKING_SET_FILES, + MAX_WORKING_SET_DECLARATIONS, + WORKING_SET_CONCURRENCY, + WORKING_SET_BYTE_BUDGET, +} from './ast-read-file-bounded-helpers.js'; +// --------------------------------------------------------------------------- +// REQ-3232-2: competing-limit reason precedence. +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: competing-limit reason precedence', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + it('reports file-count when count, bytes, and declarations trip together', async () => { + // 50 retained files filling the byte budget and declaration cap exactly; + // the 51st candidate violates all three limits at once. The file-count + // limit is evaluated first, before any read of the 51st file. + const perFileBytes = Math.floor(WORKING_SET_BYTE_BUDGET / 50); + seedAndModify( + ctx.tempDir, + Array.from({ length: MAX_WORKING_SET_FILES + 1 }, (_, i) => { + const isSentinel = i === MAX_WORKING_SET_FILES; + const size = isSentinel ? 4096 : perFileBytes; + const decls = isSentinel ? 1 : 10; + return { + name: `c${String(i).padStart(3, '0')}.ts`, + seed: paddedDeclarations(decls, `s${i}_`, size), + modified: paddedDeclarations(decls, `m${i}_`, size), + }; + }), + ); + const target = writeTarget(ctx.tempDir); + + const observing = new ObservingExtractor(); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.partialReason).toBe('file-count'); + expect(acquisition.status.retainedFiles).toBe(MAX_WORKING_SET_FILES); + expect(acquisition.status.retainedDeclarations).toBe( + MAX_WORKING_SET_DECLARATIONS, + ); + expect(acquisition.status.retainedSourceBytes).toBe( + perFileBytes * MAX_WORKING_SET_FILES, + ); + expect(observing.extractionEnters).toHaveLength(MAX_WORKING_SET_FILES); + }); + + it('reports source-bytes before declarations when both would trip', async () => { + // 25 files x 20 declarations fill the declaration cap exactly and leave + // fewer remaining bytes than the next file's size: the byte admission + // stops acquisition before declaration extraction could run. + const perFileBytes = 167721; // 25 x 167721 = budget - 1279 bytes + seedAndModify( + ctx.tempDir, + Array.from({ length: 26 }, (_, i) => { + const isOver = i === 25; + const size = isOver ? 2048 : perFileBytes; + const decls = isOver ? 2 : 20; + return { + name: `p${String(i).padStart(3, '0')}.ts`, + seed: paddedDeclarations(decls, `s${i}_`, size), + modified: paddedDeclarations(decls, `m${i}_`, size), + }; + }), + ); + const target = writeTarget(ctx.tempDir); + + const observing = new ObservingExtractor(); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.partialReason).toBe('source-bytes'); + expect(acquisition.status.retainedFiles).toBe(25); + expect(acquisition.status.retainedDeclarations).toBe( + MAX_WORKING_SET_DECLARATIONS, + ); + // The over-budget file was never read or parsed. + expect(observing.extractionEnters).toHaveLength(25); + }); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-4 / accurate completeness: skipped-only and failed contexts. +// --------------------------------------------------------------------------- + +describe('REQ-3232-4: accurate completeness and skipped-only contexts', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + it('renders partial accounting when the only eligible file went missing', async () => { + writeFileSync( + join(ctx.tempDir, 'gone.ts'), + declarationsBody(1, 'gone_'), + 'utf-8', + ); + gitCommitAll(ctx.tempDir, 'seed'); + rmSync(join(ctx.tempDir, 'gone.ts')); + const target = writeTarget(ctx.tempDir); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.files).toHaveLength(0); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('skipped-files'); + expect(acquisition.status.missingFiles).toBe(1); + expect(acquisition.status.eligibleFiles).toBe(1); + expect(acquisition.status.traversalComplete).toBe(true); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('WORKING SET CONTEXT (partial'); + expect(output).toContain('no working-set files retained'); + expect(output).toContain('1 missing'); + }); + + it('renders partial accounting when the only eligible file is oversized', async () => { + seedAndModify(ctx.tempDir, [ + { + name: 'only.ts', + seed: paddedDeclarations(1, 'os_', WORKING_SET_BYTE_BUDGET + 512), + modified: paddedDeclarations(1, 'om_', WORKING_SET_BYTE_BUDGET + 512), + }, + ]); + const target = writeTarget(ctx.tempDir); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.files).toHaveLength(0); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('skipped-files'); + expect(acquisition.status.oversizedFiles).toBe(1); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + const output = String(result.llmContent); + expect(output).toContain('no working-set files retained'); + expect(output).toContain('1 oversized'); + }); + + it('renders a Git error to the LLM instead of an empty complete set', async () => { + const sha = seedAndModify(ctx.tempDir, simpleModifiedEntries(1, 'ge')); + const target = writeTarget(ctx.tempDir); + // Corrupt the commit object that HEAD points to: the repository probe + // passes, the unstaged diff phase succeeds (it does not need HEAD), but + // the staged diff phase fails when it tries to resolve the index tree + // against the corrupt commit. Discovery keeps its earlier candidate + // and surfaces the error. + const objectPath = join( + ctx.tempDir, + '.git', + 'objects', + sha.slice(0, 2), + sha.slice(2), + ); + chmodSync(objectPath, 0o644); + writeFileSync(objectPath, 'garbage', 'utf-8'); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('git-error'); + // Candidates discovered before the failing phase are still retained. + expect(acquisition.status.retainedFiles).toBe(1); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('Git working-set discovery failed'); + expect(output).toContain('ge000.ts'); + }); + + it('renders Git-error eligible counts as lower bounds', async () => { + // Candidates were observed before the failing phase, but the true + // eligible set was never exhausted: the rendered count must read + // "at least N" instead of an exact total, exactly like truncation. + const sha = seedAndModify(ctx.tempDir, simpleModifiedEntries(2, 'lb')); + const target = writeTarget(ctx.tempDir); + const objectPath = join( + ctx.tempDir, + '.git', + 'objects', + sha.slice(0, 2), + sha.slice(2), + ); + chmodSync(objectPath, 0o644); + writeFileSync(objectPath, 'garbage', 'utf-8'); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect(acquisition.status.partialReason).toBe('git-error'); + expect(acquisition.status.eligibleFiles).toBe(2); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('at least 2'); + expect(output).not.toContain(' of 2 '); + expect(output).toContain('lb000.ts'); + expect(output).toContain('lb001.ts'); + }); +}); + +// --------------------------------------------------------------------------- + +// --------------------------------------------------------------------------- +// REQ-3232-2: acquisition never rejects on a single bad file (read boundary). +// --------------------------------------------------------------------------- + +describe('REQ-3232-2: acquisition never rejects on a mid-read fault', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + it('counts a candidate whose read fails after planning as unreadable, never rejecting', async () => { + // Eight eligible files: the first chunk (four files) is acquired while a + // deliberate real fault replaces a second-chunk candidate with a + // directory. Its stat already ran, so the failure lands at the open/read + // boundary of acquisition — the exact contract under test. + seedAndModify(ctx.tempDir, simpleModifiedEntries(8, 'fl')); + const victim = join(ctx.tempDir, 'fl006.ts'); + const target = writeTarget(ctx.tempDir); + const observing = new ObservingExtractor({ + delayMs: 100, + onFirstExtraction: () => { + rmSync(victim); + mkdirSync(victim); + }, + }); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('skipped-files'); + expect(acquisition.status.skippedFiles).toBe(1); + // The other seven candidates were still retained. + expect(acquisition.status.retainedFiles).toBe(7); + + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('1 unreadable'); + expect(output).toContain('fl000.ts'); + }); + + it('charges the exact raw bytes read for a file containing invalid UTF-8', async () => { + // Four bytes of 0x80 are invalid UTF-8: re-encoding the decoded text + // back to UTF-8 would replace them with U+FFFD sequences (12 bytes) and + // mis-charge the budget. The authoritative charge is the raw read count. + const body = 'export function bad(): void {}\n'; + const raw = Buffer.concat([ + Buffer.from(body, 'utf-8'), + Buffer.from([0x80, 0x80, 0x80, 0x80]), + ]); + writeFileSync(join(ctx.tempDir, 'invalid.ts'), raw); + gitCommitAll(ctx.tempDir, 'seed invalid'); + writeFileSync( + join(ctx.tempDir, 'anchor.ts'), + 'export const a = 1;\n', + 'utf-8', + ); + gitCommitAll(ctx.tempDir, 'seed anchor'); + writeFileSync(join(ctx.tempDir, 'invalid.ts'), raw); + writeFileSync( + join(ctx.tempDir, 'anchor.ts'), + 'export const a = 2;\n', + 'utf-8', + ); + const target = writeTarget(ctx.tempDir); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + expect( + acquisition.files.some( + (file) => file.filePath === join(ctx.tempDir, 'invalid.ts'), + ), + ).toBe(true); + const anchorBytes = Buffer.byteLength('export const a = 2;\n'); + expect(acquisition.status.retainedSourceBytes).toBe( + raw.length + anchorBytes, + ); + }); +}); +// REQ-3232-3: cancellation threading. +// --------------------------------------------------------------------------- + +describe('REQ-3232-3: invocation signal threading', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + seedAndModify(ctx.tempDir, [ + { + name: 'ws-one.ts', + seed: declarationsBody(2, 'w1s_'), + modified: declarationsBody(3, 'w1m_'), + }, + ]); + }); + + it('a pre-aborted signal schedules no working-set acquisition', async () => { + const target = writeTarget(ctx.tempDir); + const controller = new AbortController(); + controller.abort(); + const result = await runRead( + createFakeToolHost(ctx.tempDir), + target, + controller.signal, + ); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('LLXPRT READ: '); + expect(output).toContain('CONTEXT ANALYSIS:'); + expect(output).not.toContain('ws-one.ts'); + expect(output).toContain('WORKING SET CONTEXT (partial: cancelled'); + const acquisition = await acquireWorkingSet( + target, + ctx.tempDir, + new ASTQueryExtractor(), + controller.signal, + ); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('cancelled'); + expect(acquisition.status.retainedFiles).toBe(0); + expect(acquisition.status.eligibleFiles).toBe(0); + }); + + it('aborting during discovery terminates the Git child and reports cancelled', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(40, 'ab')); + const target = writeTarget(ctx.tempDir); + const controller = new AbortController(); + const pending = acquireWorkingSet( + target, + ctx.tempDir, + new ASTQueryExtractor(), + controller.signal, + ); + // The abort lands while the async Git discovery child is still running; + // it must terminate that exact child and surface cancellation. + controller.abort(); + const acquisition = await pending; + expect(acquisition.files).toHaveLength(0); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('cancelled'); + }); + + it('mid-collection abort stops scheduling new work after in-flight items finish', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(20, 'mi')); + const target = writeTarget(ctx.tempDir); + + const controller = new AbortController(); + const observing = new ObservingExtractor({ + onFirstExtraction: () => { + controller.abort(); + }, + }); + const acquisition = await enrichWithWorkingSetContext( + target, + ctx.tempDir, + new RepositoryContextProvider(), + observing, + controller.signal, + ); + // In-flight chunk items finish (bounded by the concurrency policy)... + expect(acquisition.files).toHaveLength(WORKING_SET_CONCURRENCY); + expect(acquisition.files.length).toBeLessThan(20); + expect(acquisition.status.complete).toBe(false); + expect(acquisition.status.partialReason).toBe('cancelled'); + expect(acquisition.status.retainedFiles).toBe(WORKING_SET_CONCURRENCY); + // The beforeEach working-set file is also eligible. + expect(acquisition.status.eligibleFiles).toBe(21); + // ...and no additional acquisition starts after the abort: exactly the + // first chunk was read and parsed, nothing beyond it. + expect(observing.extractionEnters).toHaveLength(WORKING_SET_CONCURRENCY); + }); + + it('renders cancelled working-set accounting with a lower-bound eligible count', async () => { + // A mid-collection abort retains only the in-flight chunk while more + // eligible files were already observed: cancellation proves a lower + // bound, never an exact total, so the rendered header must read + // "at least N" exactly like discovery truncation. + seedAndModify(ctx.tempDir, simpleModifiedEntries(20, 'rc')); + const target = writeTarget(ctx.tempDir); + + const controller = new AbortController(); + const observing = new ObservingExtractor({ + onFirstBoundedExtraction: () => { + controller.abort(); + }, + }); + const invocation = new ASTReadFileToolInvocation( + createFakeToolHost(ctx.tempDir), + { file_path: target }, + new ASTContextCollector(observing), + ); + const result = await invocation.execute(controller.signal); + expect(result.error).toBeUndefined(); + const output = String(result.llmContent); + expect(output).toContain('partial: cancelled before completion'); + // The beforeEach working-set file makes the observed eligible set 21. + expect(output).toContain( + `retained ${WORKING_SET_CONCURRENCY} of at least 21 files`, + ); + expect(output).not.toContain(' of 21 '); + }); +}); + +// --------------------------------------------------------------------------- +// Finding 8: genuine max-in-flight acquisition observation. +// --------------------------------------------------------------------------- + +describe('max-in-flight acquisition stays within the concurrency policy', () => { + const ctx = useTempDir(); + + beforeEach(() => { + gitInit(ctx.tempDir); + }); + + it('observes peak active real acquisitions at or below the policy', async () => { + seedAndModify(ctx.tempDir, simpleModifiedEntries(12, 'kk')); + const target = writeTarget(ctx.tempDir); + + // Deterministic chunk barrier: every extraction holds until the + // policy-sized chunk has fully entered, so overlap is observed because + // of the concurrency policy itself, not a timing window. A regression + // that serializes acquisition is released by the barrier's bounded + // failure timer and fails the peak assertion below instead of hanging. + const observing = new ObservingExtractor({ + barrierWidth: WORKING_SET_CONCURRENCY, + }); + const acquisition = await acquireWorkingSet(target, ctx.tempDir, observing); + expect(acquisition.status.complete).toBe(true); + expect(observing.peakActive).toBe(WORKING_SET_CONCURRENCY); + }); +}); diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-display.bun.test.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-display.bun.test.ts new file mode 100644 index 0000000000..4f534a7c48 --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-display.bun.test.ts @@ -0,0 +1,136 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Behavioral tests for ast_read_file display and metadata compatibility + * (issue #3232, Finding 6). + */ + +import { describe, it, expect } from 'bun:test'; +import { writeFileSync, existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { createFakeToolHost, useTempDir } from './test-helpers.js'; +import { + gitInit, + runRead, + recordOf, + seedAndModify, + simpleModifiedEntries, + writeTarget, + acquireWorkingSet, +} from './ast-read-file-bounded-helpers.js'; +import type { + WorkingSetAcquisitionStatus, + WorkingSetPartialReason, +} from '../types.js'; + +// --------------------------------------------------------------------------- +// Item: WorkingSetAcquisitionStatus is a complete/partial discriminated union. +// --------------------------------------------------------------------------- + +type Expect = T; +type Equal = + (() => T extends X ? 1 : 2) extends () => T extends Y ? 1 : 2 + ? true + : false; + +type CompleteStatus = Extract; +type PartialStatus = Extract; + +/** A complete acquisition may not carry any partial reason. */ +type CompleteForbidsReason = Expect< + Equal +>; +/** An incomplete acquisition must carry exactly one reason. */ +type PartialRequiresReason = Expect< + Equal +>; + +// Compile-time witnesses (fail to compile if the union regresses). +const completeForbidsReason: CompleteForbidsReason = true; +const partialRequiresReason: PartialRequiresReason = true; +void completeForbidsReason; +void partialRequiresReason; + +/** + * Narrow a runtime status to its complete variant, failing loudly (never + * conditionally) when acquisition was partial. Keeps the expects outside any + * branch so the behavioral assertions stay unconditional. + */ +function assumeComplete(status: WorkingSetAcquisitionStatus): CompleteStatus { + if (status.complete) { + return status; + } + throw new Error( + `expected a complete acquisition, got: ${String(status.partialReason)}`, + ); +} + +/** Narrow a runtime status to its partial variant, failing loudly otherwise. */ +function assumePartial(status: WorkingSetAcquisitionStatus): PartialStatus { + if (!status.complete) { + return status; + } + throw new Error('expected a partial acquisition, got a complete one'); +} + +describe('REQ-3232-4: display and metadata compatibility', () => { + const ctx = useTempDir(); + + it('keeps returnDisplay metadata exactly {language, declarationsCount}', async () => { + gitInit(ctx.tempDir); + const target = join(ctx.tempDir, 'plain.ts'); + writeFileSync( + target, + 'export function one(): number { return 1; }\nexport function two(): number { return 2; }\n', + 'utf-8', + ); + const result = await runRead(createFakeToolHost(ctx.tempDir), target); + expect(result.error).toBeUndefined(); + const display = recordOf(result.returnDisplay, 'returnDisplay'); + expect(display.fileName).toBe('plain.ts'); + expect(String(display.content)).toContain('export function one()'); + const metadata = recordOf(display.metadata, 'metadata'); + // The public metadata contract is exactly these two fields — no + // working-set accounting leaks into the display payload. + expect(metadata).toEqual({ + language: 'typescript', + declarationsCount: 2, + }); + const output = String(result.llmContent); + expect(output).not.toContain('WORKING SET CONTEXT'); + expect(existsSync(target)).toBe(true); + }); +}); + +describe('REQ-3232-4: acquisition status discrimination', () => { + const ctx = useTempDir(); + + it('carries no partial reason on a complete acquisition', async () => { + gitInit(ctx.tempDir); + seedAndModify(ctx.tempDir, simpleModifiedEntries(2, 'cd')); + const target = writeTarget(ctx.tempDir); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + const complete = assumeComplete(acquisition.status); + // Narrowed to the complete variant: no reason key may exist at all. + expect('partialReason' in complete).toBe(false); + expect(complete.retainedFiles).toBe(2); + }); + + it('requires exactly one partial reason on an incomplete acquisition', async () => { + gitInit(ctx.tempDir); + // One retained candidate plus one that vanishes before acquisition: the + // run is traversal-complete but partial because a file was skipped. + seedAndModify(ctx.tempDir, simpleModifiedEntries(2, 'pd')); + const target = writeTarget(ctx.tempDir); + rmSync(join(ctx.tempDir, 'pd001.ts')); + + const acquisition = await acquireWorkingSet(target, ctx.tempDir); + const partial = assumePartial(acquisition.status); + expect(partial.partialReason).toBe('skipped-files'); + expect(partial.missingFiles).toBe(1); + expect(partial.traversalComplete).toBe(true); + }); +}); diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-helpers.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-helpers.ts new file mode 100644 index 0000000000..1369ce854f --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-file-bounded-helpers.ts @@ -0,0 +1,401 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Behavioral tests for bounded ast_read_file acquisition (issue #3232). + * + * REQ-3232-1: ast_read_file opts out of repository relationship analysis + * (repository context, symbol index, related files/symbols) that never + * reaches its model-facing or display output, while ast_edit keeps it. + * REQ-3232-2: working-set Git discovery is bounded (finite candidate count + * with a one-over sentinel, AbortSignal support, exact-child termination) + * and acquisition enforces finite file-count, aggregate-source-byte, + * retained-declaration, and concurrency policies before over-budget + * reads/parses start, with bounded reads that validate actual bytes. + * REQ-3232-3: the invocation AbortSignal is threaded through discovery and + * collection; pre-abort schedules no acquisition, mid-collection abort + * stops scheduling and is never reported complete. + * REQ-3232-4: bounded working-set context renders an explicit partial + * marker/reason/accounting while complete output stays compatible, including + * when zero files are retained; public display metadata stays exactly + * {language, declarationsCount}. + * + * All fixtures are real temporary directories with real Git state, real + * files, and the real ASTReadFileTool / collector / providers. No mocking of + * the component under test: the only wrappers are real subclasses whose + * public behavior delegates to the real implementation and whose side effect + * is AbortController timing or real acquisition observation. + */ + +import { writeFileSync, mkdirSync, existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import type { ToolResult } from '../../tools.js'; +import { ASTReadFileTool } from '../../ast-edit.js'; +import { ASTQueryExtractor } from '../ast-query-extractor.js'; +import { RepositoryContextProvider } from '../repository-context-provider.js'; +import { enrichWithWorkingSetContext } from '../workspace-context-provider.js'; +import type { EnhancedDeclaration, WorkingSetAcquisition } from '../types.js'; +import type { createFakeToolHost } from './test-helpers.js'; +import { gitCheck, gitInit, gitCommitAll } from './ast-read-git-fixtures.js'; + +// Policy contract under test (kept as literals so the tests are the spec). +const MAX_WORKING_SET_FILES = 50; +const MAX_WORKING_SET_DECLARATIONS = 500; +const WORKING_SET_CONCURRENCY = 4; +const WORKING_SET_BYTE_BUDGET = 4 * 1024 * 1024; +const DISCOVERY_CANDIDATE_CAP = MAX_WORKING_SET_FILES + 1; +const READ_SENTINEL_BYTES = 4096; + +function isRecord(value: unknown): value is Record { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return false; + } + const prototype = Object.getPrototypeOf(value); + return prototype === null || prototype === Object.prototype; +} + +/** + * Narrow an unknown result part to a record or fail the test loudly. Used + * instead of `if (isRecord(...))` wrappers around expects so expectations are + * never conditional. + */ +function recordOf(value: unknown, what: string): Record { + if (!isRecord(value)) { + throw new Error(`expected ${what} to be a record, got: ${typeof value}`); + } + return value; +} + +async function runRead( + host: ReturnType, + filePath: string, + signal?: AbortSignal, +): Promise { + return new ASTReadFileTool(host) + .build({ file_path: filePath }) + .execute(signal ?? new AbortController().signal); +} + +/** Run a real bounded working-set acquisition and return its result. */ +async function acquireWorkingSet( + target: string, + root: string, + extractor: ASTQueryExtractor = new ASTQueryExtractor(), + signal?: AbortSignal, +): Promise { + return enrichWithWorkingSetContext( + target, + root, + new RepositoryContextProvider(), + extractor, + signal, + ); +} + +// The checked Git fixture wrappers (gitCheck/gitInit/gitCommitAll) live in +// ast-read-git-fixtures.ts so child-process fixtures can reuse them without +// importing bun:test. They are re-exported below for the existing suites. + +/** A TypeScript file body with `count` exported function declarations. */ +function declarationsBody(count: number, prefix: string): string { + const lines: string[] = []; + for (let i = 0; i < count; i++) { + lines.push(`export function ${prefix}${i}(): number {`); + lines.push(` return ${i};`); + lines.push('}'); + lines.push(''); + } + return lines.join('\n'); +} + +/** A TypeScript file of exactly `sizeBytes` bytes containing `count` decls. */ +function paddedDeclarations( + count: number, + prefix: string, + sizeBytes: number, +): string { + const base = declarationsBody(count, prefix); + const padNeeded = sizeBytes - Buffer.byteLength(base) - 1; + if (padNeeded < 2) { + throw new Error( + `fixture of ${sizeBytes} bytes cannot hold ${count} declarations`, + ); + } + return `${base}//${'x'.repeat(padNeeded - 2)}\n`; +} + +/** + * Seed then modify real tracked files so every name appears in the unstaged + * working set. Seed and modified contents differ (same size where a size is + * given) so git diff actually reports each file. + */ +function seedAndModify( + dir: string, + entries: ReadonlyArray<{ + readonly name: string; + readonly seed: string; + readonly modified: string; + }>, +): string { + for (const entry of entries) { + writeFileSync(join(dir, entry.name), entry.seed, 'utf-8'); + } + const sha = gitCommitAll(dir, 'seed'); + for (const entry of entries) { + writeFileSync(join(dir, entry.name), entry.modified, 'utf-8'); + } + return sha; +} + +function writeTarget(dir: string): string { + const target = join(dir, 'target.ts'); + writeFileSync(target, 'export function readTarget(): void {}\n', 'utf-8'); + return target; +} + +/** Simple seed/modify pairs of `count` generated declaration files. */ +function simpleModifiedEntries( + count: number, + prefix: string, +): ReadonlyArray<{ name: string; seed: string; modified: string }> { + return Array.from({ length: count }, (_, i) => { + const name = `${prefix}${String(i).padStart(3, '0')}.ts`; + return { + name, + seed: declarationsBody(1, `s${i}_`), + modified: declarationsBody(2, `m${i}_`), + }; + }); +} + +/** + * Fixture floor for trailing Git stdout after a discovery kill: one full + * MiB of NUL-delimited names, far beyond the 64 KiB a child pipe typically + * delivers in its first chunk, so buffered late data events are guaranteed. + */ +const GIT_TRAILING_OUTPUT_FLOOR_BYTES = 1024 * 1024; + +const LONG_PATH_SEED_BODY = 'export const seedValue = 1;' + '\n'; +const LONG_PATH_MODIFIED_BODY = 'export const modifiedValue = 2;' + '\n'; + +/** + * Seed and modify `count` tracked files whose relative paths are long enough + * that one Git listing phase emits well over two stdout chunks of + * NUL-delimited names. Returns the generated relative names. + */ +function createLongPathCandidates(dir: string, count: number): string[] { + const segment = 'd'.repeat(170); + const relativeDir = [segment, segment, segment].join('/'); + mkdirSync(join(dir, relativeDir), { recursive: true }); + const names: string[] = []; + for (let i = 0; i < count; i++) { + const name = `${relativeDir}/f${String(i).padStart(40, '0')}.ts`; + writeFileSync(join(dir, name), LONG_PATH_SEED_BODY, 'utf-8'); + names.push(name); + } + gitCommitAll(dir, 'long-path seed'); + for (const name of names) { + writeFileSync(join(dir, name), LONG_PATH_MODIFIED_BODY, 'utf-8'); + } + const totalNulBytes = names.reduce((sum, name) => sum + name.length + 1, 0); + if (totalNulBytes < GIT_TRAILING_OUTPUT_FLOOR_BYTES) { + throw new Error(`long-path fixture emitted only ${totalNulBytes} bytes`); + } + return names; +} + +/** + * True when the filesystem treats filenames case-insensitively (macOS, + * Windows). Probed at runtime from real filesystem behavior so tests need no + * platform sniffing: a file written as lowercase is visible under uppercase. + */ +function hasCaseInsensitiveFilenames(dir: string): boolean { + const probe = join(dir, `case-probe-${process.pid}.txt`); + writeFileSync(probe, '', 'utf-8'); + const caseInsensitive = existsSync(probe.toUpperCase()); + rmSync(probe, { force: true }); + return caseInsensitive; +} + +/** + * Commit then modify the read target so `git diff --name-only` genuinely + * lists it as a candidate that exclusion must remove. An untracked target + * never appears in any diff phase, so exclusion tests that only write an + * untracked target prove nothing about the exclude path. + */ +function writeTrackedModifiedTarget( + dir: string, + name: string = 'target.ts', +): string { + const target = join(dir, name); + writeFileSync(target, 'export function readTargetSeed(): void {}\n', 'utf-8'); + gitCommitAll(dir, 'target seed'); + writeFileSync( + target, + 'export function readTarget(): number { return 1; }\n', + 'utf-8', + ); + return target; +} + +/** Failure release for the chunk barrier: a chunk that never fills must fail + * an assertion instead of hanging the suite, so waiters give up after this. */ +const BARRIER_FAILURE_RELEASE_MS = 5000; + +/** + * Real extractor wrapper that observes genuine acquisition activity while + * delegating every extraction to the real implementation. Two mechanisms: + * the optional delay widens the real extraction window, and the optional + * chunk barrier deterministically holds each extraction until a full + * policy-sized chunk has entered (released by the last worker of the chunk, + * with a bounded failure release when the chunk never fills). + */ +class ObservingExtractor extends ASTQueryExtractor { + readonly extractionEnters: string[] = []; + readonly boundedLengths: number[] = []; + active = 0; + peakActive = 0; + activeContentBytes = 0; + peakActiveContentBytes = 0; + private readonly delayMs: number; + private readonly barrierWidth: number; + private readonly onFirstExtraction?: () => void; + private readonly onFirstBoundedExtraction?: () => void; + private firstExtractionSeen = false; + private firstBoundedExtractionSeen = false; + private barrierWaiters: Array<() => void> = []; + + constructor(options?: { + delayMs?: number; + onFirstExtraction?: () => void; + onFirstBoundedExtraction?: () => void; + barrierWidth?: number; + }) { + super(); + this.delayMs = options?.delayMs ?? 0; + this.onFirstExtraction = options?.onFirstExtraction; + this.onFirstBoundedExtraction = options?.onFirstBoundedExtraction; + this.barrierWidth = options?.barrierWidth ?? 0; + } + + private enter(filePath: string, content: string): void { + if (!this.firstExtractionSeen) { + this.firstExtractionSeen = true; + this.onFirstExtraction?.(); + } + this.active += 1; + this.peakActive = Math.max(this.peakActive, this.active); + this.activeContentBytes += Buffer.byteLength(content); + this.peakActiveContentBytes = Math.max( + this.peakActiveContentBytes, + this.activeContentBytes, + ); + this.extractionEnters.push(filePath); + this.releaseBarrierIfChunkFull(); + } + + /** Release every held worker once the chunk has fully entered. */ + private releaseBarrierIfChunkFull(): void { + if ( + this.barrierWidth > 0 && + this.active >= this.barrierWidth && + this.barrierWaiters.length > 0 + ) { + for (const release of this.barrierWaiters.splice(0)) { + release(); + } + } + } + + private exit(content: string): void { + this.active -= 1; + this.activeContentBytes -= Buffer.byteLength(content); + } + + override async extractDeclarations( + filePath: string, + content: string, + ): Promise { + this.enter(filePath, content); + try { + await this.settle(); + return await super.extractDeclarations(filePath, content); + } finally { + this.exit(content); + } + } + + override async extractDeclarationsBounded( + filePath: string, + content: string, + limit: number, + ): Promise { + if (!this.firstBoundedExtractionSeen) { + this.firstBoundedExtractionSeen = true; + this.onFirstBoundedExtraction?.(); + } + this.enter(filePath, content); + try { + await this.settle(); + const declarations = await super.extractDeclarationsBounded( + filePath, + content, + limit, + ); + this.boundedLengths.push(declarations.length); + return declarations; + } finally { + this.exit(content); + } + } + + private async settle(): Promise { + if (this.barrierWidth > 0) { + if (this.active < this.barrierWidth) { + await new Promise((resolve) => { + const failureRelease = setTimeout( + resolve, + BARRIER_FAILURE_RELEASE_MS, + ); + this.barrierWaiters.push(() => { + clearTimeout(failureRelease); + resolve(); + }); + }); + } + return; + } + if (this.delayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, this.delayMs)); + } + } +} + +// Shared helpers exported for the split bounded-acquisition suite. +export { + isRecord, + recordOf, + runRead, + acquireWorkingSet, + gitCheck, + gitInit, + gitCommitAll, + declarationsBody, + paddedDeclarations, + seedAndModify, + writeTarget, + simpleModifiedEntries, + createLongPathCandidates, + writeTrackedModifiedTarget, + hasCaseInsensitiveFilenames, + ObservingExtractor, +}; +export { + MAX_WORKING_SET_FILES, + MAX_WORKING_SET_DECLARATIONS, + WORKING_SET_CONCURRENCY, + WORKING_SET_BYTE_BUDGET, + DISCOVERY_CANDIDATE_CAP, + READ_SENTINEL_BYTES, +}; diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-git-fixtures.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-git-fixtures.ts new file mode 100644 index 0000000000..216d96a783 --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-git-fixtures.ts @@ -0,0 +1,70 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Bun-test-free checked Git fixture helpers for the ast_read_file suites. + * + * This module deliberately imports nothing from `bun:test` so that child + * processes (for example the memory-regression fixture generator) can reuse + * the exact same checked wrapper instead of a weaker duplicate. + */ + +import { spawnSync, type SpawnSyncReturns } from 'node:child_process'; +import { isAbsolute } from 'node:path'; + +/** Bounded fixture Git timeout and output allowance. */ +export const GIT_FIXTURE_TIMEOUT_MS = 30_000; +export const GIT_FIXTURE_MAX_BUFFER = 64 * 1024 * 1024; + +/** Render every failure mode of a checked fixture Git run. */ +function describeGitFailure( + args: readonly string[], + result: SpawnSyncReturns, +): string { + const details = [ + `status=${String(result.status)}`, + `signal=${String(result.signal)}`, + `error=${result.error instanceof Error ? result.error.message : String(result.error)}`, + `stderr=${String(result.stderr).trim().slice(0, 2000)}`, + ]; + return `git ${args.join(' ')} failed (${details.join('; ')})`; +} + +/** + * The single shared checked Git fixture helper. Fails fixture setup loudly on + * any nonzero status, signal death, or spawn error so a broken fixture can + * never masquerade as behavior. The timeout and maxBuffer are explicit: a + * large fixture (thousands of long paths) otherwise exceeds the runtime's + * small default buffer allowance and is killed mid-write. + * Returns captured stdout for callers that need fixture data back. + */ +export function gitCheck(dir: string, args: string[]): string { + if (!isAbsolute(dir)) { + throw new Error(`fixture git dir must be absolute, got: ${dir}`); + } + const result = spawnSync('git', ['-C', dir, ...args], { + encoding: 'utf-8', + stdio: 'pipe', + timeout: GIT_FIXTURE_TIMEOUT_MS, + maxBuffer: GIT_FIXTURE_MAX_BUFFER, + }); + if (result.error !== undefined || result.status !== 0) { + throw new Error(describeGitFailure(args, result)); + } + return result.stdout; +} + +/** Initializes a real Git repository with a stable identity. */ +export function gitInit(dir: string): void { + gitCheck(dir, ['init']); + gitCheck(dir, ['config', 'user.email', 'test@example.com']); + gitCheck(dir, ['config', 'user.name', 'Test']); +} + +/** Stages and commits every fixture change; returns the resulting commit SHA. */ +export function gitCommitAll(dir: string, message: string): string { + gitCheck(dir, ['add', '-A']); + gitCheck(dir, ['commit', '-m', message]); + return gitCheck(dir, ['rev-parse', 'HEAD']).trim(); +} diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-memory-child.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-memory-child.ts new file mode 100644 index 0000000000..22b5e498a9 --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-memory-child.ts @@ -0,0 +1,112 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Child-process fixture for the ast_read_file memory regression (issue + * #3232). Invoked by ast-read-memory.bun.test.ts with a generated workspace + * directory. Runs the real ASTReadFileTool (one bounded sequential read plus + * three parallel reads), samples process RSS throughout, and prints a + * tool-result marker after every invocation, then samples for a quiet + * window to prove no native traversal or pending callback kept the process + * alive after all tool results resolved. + */ + +import { join } from 'node:path'; +import { ASTReadFileTool } from '../../ast-edit.js'; +import { createAstReadToolHost } from './ast-read-tool-host.js'; + +interface MemoryReport { + readonly ok: boolean; + readonly sequentialOk: boolean; + readonly parallelOk: boolean; + readonly llmHasWorkingSet: boolean; + readonly peakRssBytes: number; + readonly finalRssBytes: number; + readonly postResultRssGrowthBytes: number; + readonly quietWindowSamples: number; +} + +interface ReadOutcome { + readonly error?: { readonly message: string } | undefined; + readonly llmContent: string; +} + +if (process.argv.length < 3) { + process.stderr.write('usage: ast-read-memory-child.ts \n'); + process.exit(2); +} +const workspaceRoot = process.argv[2]; + +let peakRssBytes = process.memoryUsage.rss(); +function sampleRss(): number { + const rss = process.memoryUsage.rss(); + peakRssBytes = Math.max(peakRssBytes, rss); + return rss; +} + +const sampler = setInterval(() => { + sampleRss(); +}, 20); + +async function executeRead(): Promise { + const tool = new ASTReadFileTool(createAstReadToolHost(workspaceRoot)); + const invocation = tool.build({ + file_path: join(workspaceRoot, 'target.ts'), + limit: 5, + }); + const result = await invocation.execute(new AbortController().signal); + // Emit a tool-result marker after every result so the parent can correlate + // the timing of native fan-out activity (if any survived) with sampling. + process.stdout.write('AST_READ_TOOL_RESULT\n'); + return { error: result.error, llmContent: String(result.llmContent) }; +} + +const sequential = await executeRead(); +const sequentialOk = sequential.error === undefined; +const llmHasWorkingSet = sequential.llmContent.includes('WORKING SET CONTEXT'); + +const parallel = await Promise.all([ + executeRead(), + executeRead(), + executeRead(), +]); +const parallelOk = parallel.every((result) => result.error === undefined); + +// Post-result quiet/drain window: sample RSS for a fixed interval after all +// tool results have resolved. The old code's native findInFiles fan-out +// continued past each tool result, so RSS would keep climbing here; the +// bounded acquisition resolves all its work before returning, so the tail +// growth stays flat. +const QUIET_WINDOW_MS = 1500; +const SAMPLE_INTERVAL_MS = 25; +const preQuietRss = sampleRss(); +let postResultRssGrowthBytes = 0; +let quietWindowSamples = 0; +const quietStart = Date.now(); +while (Date.now() - quietStart < QUIET_WINDOW_MS) { + const rss = sampleRss(); + quietWindowSamples += 1; + postResultRssGrowthBytes = Math.max( + postResultRssGrowthBytes, + rss - preQuietRss, + ); + await new Promise((resolve) => setTimeout(resolve, SAMPLE_INTERVAL_MS)); +} + +clearInterval(sampler); +const finalRssBytes = process.memoryUsage.rss(); +peakRssBytes = Math.max(peakRssBytes, finalRssBytes); + +const report: MemoryReport = { + ok: sequentialOk && parallelOk, + sequentialOk, + parallelOk, + llmHasWorkingSet, + peakRssBytes, + finalRssBytes, + postResultRssGrowthBytes, + quietWindowSamples, +}; +process.stdout.write('AST_READ_QUIET_DONE\n'); +process.stdout.write(`AST_READ_MEMORY_REPORT ${JSON.stringify(report)}\n`); diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-memory.bun.test.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-memory.bun.test.ts new file mode 100644 index 0000000000..401900e320 --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-memory.bun.test.ts @@ -0,0 +1,361 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Cross-platform memory regression for ast_read_file (issue #3232). + * + * Spawns a child Bun process (ast-read-memory-child.ts) that executes the + * REAL ASTReadFileTool against a generated Git workspace whose committed + * dependency files reference every prioritizable target symbol — the fixture + * shape that previously triggered five concurrent whole-workspace native + * findInFiles traversals per read. The child conservatively samples peak RSS + * while one sequential and three parallel reads execute, then samples a + * post-result quiet window to prove no native traversal or pending callback + * kept the process alive after all tool results resolved. + * + * The old repository fan-out is unobservable from tool output by design, so + * the distinguishing behavioral evidence lives in the bounded-acquisition + * suite; this test is the permanent conservative memory ceiling and drain + * gate on both Windows and non-Windows paths. + */ + +import { describe, it, expect } from 'bun:test'; +import { spawnSync, type SpawnSyncReturns } from 'node:child_process'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + mkdtempSync, + mkdirSync, + rmSync, + writeFileSync, + readFileSync, + existsSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { gitCheck, gitInit } from './ast-read-git-fixtures.js'; + +const PEAK_RSS_CEILING_BYTES = 768 * 1024 * 1024; // 768 MiB +/** Post-result tail growth must stay below this margin. */ +const POST_RESULT_TAIL_CEILING_BYTES = 64 * 1024 * 1024; // 64 MiB +/** + * Fixture size calibrated so the former repository fan-out (five concurrent + * native findInFiles traversals per read, three parallel reads) exceeds the + * peak-RSS ceiling by a wide margin — the old wiring measured ~2.5 GiB peak — + * while the bounded opt-out path stays well under 300 MiB. This gives concrete + * RED evidence against the old wiring without approaching a destructive + * multi-gigabyte workload. + */ +const DEP_FILE_COUNT = 1500; +const REFERENCE_LINES_PER_FILE = 60; +const WORKING_SET_MODIFIED_FILES = 30; +const CHILD_TIMEOUT_MS = 180_000; + +interface MemoryReport { + readonly ok: boolean; + readonly sequentialOk: boolean; + readonly parallelOk: boolean; + readonly llmHasWorkingSet: boolean; + readonly peakRssBytes: number; + readonly finalRssBytes: number; + readonly postResultRssGrowthBytes: number; + readonly quietWindowSamples: number; +} + +function isMemoryReport(value: unknown): value is MemoryReport { + if (typeof value !== 'object' || value === null) { + return false; + } + const record = value as Record; + if ( + typeof record.ok !== 'boolean' || + typeof record.sequentialOk !== 'boolean' || + typeof record.parallelOk !== 'boolean' + ) { + return false; + } + if ( + typeof record.llmHasWorkingSet !== 'boolean' || + typeof record.peakRssBytes !== 'number' + ) { + return false; + } + return ( + typeof record.finalRssBytes === 'number' && + typeof record.postResultRssGrowthBytes === 'number' && + typeof record.quietWindowSamples === 'number' + ); +} + +// The checked Git fixture wrappers come from the shared bun-test-free +// module (gitCheck/gitInit/gitCommitAll): a generation failure fails loudly +// with full status/signal/stderr reporting instead of producing a silently +// broken fixture. + +function symbolNames(): string[] { + return [ + 'AlphaService', + 'BetaRegistry', + 'GammaFactory', + 'DeltaHandler', + 'EpsilonStore', + 'ZetaWorker', + ]; +} + +function depFileContent(symbols: string[]): string { + const lines: string[] = []; + for (let i = 0; i < REFERENCE_LINES_PER_FILE; i++) { + const symbol = symbols[i % symbols.length]; + lines.push( + `export const ref${i} = ${symbol}.instance${i} + ${symbol}.counter;`, + ); + } + return `${lines.join('\n')}\n`; +} + +function targetFileContent(symbols: string[]): string { + const blocks = symbols.map( + (symbol, index) => + `export class ${symbol} {\n public static instance${index}: number = ${index};\n public static counter: number = ${index};\n public process(input: string): string {\n return input;\n }\n}\n`, + ); + return `// Target fixture for the ast_read_file memory regression.\n${blocks.join('\n')}\nexport function regressionEntry(): void {}\n`; +} + +function generateWorkspace(): string { + const dir = mkdtempSync(join(tmpdir(), 'llxprt-3232-mem-')); + try { + gitInit(dir); + + const symbols = symbolNames(); + const depsDir = join(dir, 'deps'); + mkdirSync(depsDir, { recursive: true }); + for (let i = 0; i < DEP_FILE_COUNT; i++) { + writeFileSync( + join(depsDir, `dep${String(i).padStart(3, '0')}.ts`), + depFileContent(symbols), + 'utf-8', + ); + } + writeFileSync(join(dir, 'target.ts'), targetFileContent(symbols), 'utf-8'); + gitCheck(dir, ['add', '.']); + gitCheck(dir, ['commit', '-m', 'fixture']); + + // Working set: unstaged modifications of tracked dependency files, all + // referencing target symbols, so bounded discovery has real candidates. + for (let i = 0; i < WORKING_SET_MODIFIED_FILES; i++) { + const depPath = join(depsDir, `dep${String(i).padStart(3, '0')}.ts`); + writeFileSync( + depPath, + `${depFileContent(symbols)}export const modified${i} = true;\n`, + 'utf-8', + ); + } + return dir; + } catch (error) { + // A half-generated workspace must never leak into the temp directory. + rmSync(dir, { recursive: true, force: true }); + throw error; + } +} + +function parseReport(stdout: string): MemoryReport { + const marker = 'AST_READ_MEMORY_REPORT '; + const line = stdout + .split('\n') + .find((candidate) => candidate.startsWith(marker)); + if (!line) { + throw new Error(`child did not emit a report: ${stdout.slice(-2000)}`); + } + const parsed: unknown = JSON.parse(line.slice(marker.length)); + if (!isMemoryReport(parsed)) { + throw new Error( + `child report failed schema guard: ${line.slice(marker.length)}`, + ); + } + return parsed; +} + +const CHILD_SCRIPT_PATH = fileURLToPath( + new URL('./ast-read-memory-child.ts', import.meta.url), +); + +/** Render every failure mode of a memory-child run for loud diagnostics. */ +function describeChildFailure(child: SpawnSyncReturns): string { + const details = [ + `status=${String(child.status)}`, + `signal=${String(child.signal)}`, + `error=${child.error instanceof Error ? child.error.message : String(child.error)}`, + `stderr=${String(child.stderr).slice(0, 2000)}`, + ]; + return `memory child failed (${details.join('; ')})`; +} + +/** + * Drop the leading `-C ` argv pair from a logged Git invocation + * so the subcommand itself is comparable. The literal known prefix is + * stripped (rather than splitting on spaces) so a workspace path containing + * spaces cannot corrupt the remaining subcommand text. + */ +function stripGitDirPrefix(line: string, workspace: string): string { + const prefix = `-C ${workspace} `; + return line.startsWith(prefix) ? line.slice(prefix.length) : line; +} + +describe('REQ-3232-5: ast_read_file memory regression', () => { + it('keeps peak RSS bounded and drains after real reads of a fanned-out workspace', () => { + const workspace = generateWorkspace(); + try { + const child = spawnSync( + process.execPath, + [CHILD_SCRIPT_PATH, workspace], + { + encoding: 'utf-8', + stdio: 'pipe', + timeout: CHILD_TIMEOUT_MS, + maxBuffer: 16 * 1024 * 1024, + }, + ); + // Report the full spawn outcome before asserting so a failed child is + // diagnosable from the test log rather than a bare "expected 0". + if (child.status !== 0) { + throw new Error(describeChildFailure(child)); + } + // The child must emit both markers, proving the quiet window ran. + expect(child.stdout).toContain('AST_READ_TOOL_RESULT'); + expect(child.stdout).toContain('AST_READ_QUIET_DONE'); + const report = parseReport(child.stdout); + + expect(report.ok).toBe(true); + expect(report.sequentialOk).toBe(true); + expect(report.parallelOk).toBe(true); + expect(report.llmHasWorkingSet).toBe(true); + expect(report.peakRssBytes).toBeGreaterThan(0); + expect(report.peakRssBytes).toBeLessThan(PEAK_RSS_CEILING_BYTES); + expect(report.finalRssBytes).toBeLessThan(PEAK_RSS_CEILING_BYTES); + // The post-result quiet window proves native traversal drained: RSS + // growth after all tool results resolved stays below the calibrated + // ceiling. The old code's fan-out kept allocating past the result. + expect(report.quietWindowSamples).toBeGreaterThan(0); + expect(report.postResultRssGrowthBytes).toBeLessThan( + POST_RESULT_TAIL_CEILING_BYTES, + ); + } finally { + rmSync(workspace, { recursive: true, force: true }); + } + }, 240_000); +}); + +// --------------------------------------------------------------------------- +// REQ-3232-5: invocation-wiring canary. +// +// The bounded working-set phase issues `rev-parse --is-inside-work-tree`, +// `rev-parse --verify --quiet HEAD`, `diff`, and `log`. Only the repository +// relationship phase issues `remote get-url origin` and +// `branch --show-current`. A PATH shim records every Git argv the real tool +// spawns during a read, so a regression back to the old +// `collectRepositoryContext: true` wiring is detected deterministically — +// no memory threshold involved. +// --------------------------------------------------------------------------- + +describe('REQ-3232-5: ast_read_file repository wiring canary', () => { + // Windows resolves `git` through a .cmd shim that Node cannot spawn without + // a shell, so the PATH interception technique is POSIX-only there. + it.skipIf(process.platform === 'win32')( + 'spawns no repository-relationship Git commands during a real read', + () => { + const spyRoot = mkdtempSync(join(tmpdir(), 'llxprt-3232-spy-')); + const shimDir = mkdtempSync(join(tmpdir(), 'llxprt-3232-shim-')); + // A workspace directory whose name contains spaces exercises the real + // canary against the path shape that defeats space-splitting parsers. + const workspace = join(spyRoot, 'work space'); + try { + mkdirSync(workspace, { recursive: true }); + gitInit(workspace); + writeFileSync( + join(workspace, 'dep.ts'), + 'export const ref0 = Alpha.counter;\n', + 'utf-8', + ); + gitCheck(workspace, ['add', '-A']); + gitCheck(workspace, ['commit', '-m', 'fixture']); + writeFileSync( + join(workspace, 'ws.ts'), + 'export const modified = true;\n', + 'utf-8', + ); + writeFileSync( + join(workspace, 'target.ts'), + 'export class Alpha {\n public static counter: number = 1;\n}\n', + 'utf-8', + ); + + const resolved = spawnSync('sh', ['-c', 'command -v git'], { + encoding: 'utf-8', + }); + const realGit = resolved.stdout.trim(); + if (resolved.status !== 0 || realGit === '') { + throw new Error( + `could not resolve real git for the shim (status=${String( + resolved.status, + )})`, + ); + } + const logPath = join(shimDir, 'git-invocations.log'); + writeFileSync( + join(shimDir, 'git'), + [ + '#!/bin/sh', + `printf '%s\\n' "$*" >> ${JSON.stringify(logPath)}`, + `exec ${JSON.stringify(realGit)} "$@"`, + '', + ].join('\n'), + { mode: 0o755 }, + ); + + const child = spawnSync( + process.execPath, + [CHILD_SCRIPT_PATH, workspace], + { + encoding: 'utf-8', + stdio: 'pipe', + timeout: CHILD_TIMEOUT_MS, + maxBuffer: 16 * 1024 * 1024, + env: { + ...process.env, + PATH: `${shimDir}:${process.env.PATH ?? ''}`, + }, + }, + ); + if (child.status !== 0) { + throw new Error(describeChildFailure(child)); + } + const invocations = existsSync(logPath) + ? readFileSync(logPath, 'utf-8') + .split('\n') + .filter((line) => line.length > 0) + .map((line) => stripGitDirPrefix(line, workspace)) + : []; + // The read genuinely used Git (bounded discovery ran) ... + expect(invocations.length).toBeGreaterThan(0); + // ... and the exact `-C ` prefix stripped cleanly even + // with the spaced path: the logged subcommands parse intact. + expect(invocations.some((line) => line.startsWith('rev-parse '))).toBe( + true, + ); + expect(invocations.some((line) => line.startsWith('diff '))).toBe(true); + // ... but never the repository-relationship subcommands. + expect(invocations.some((line) => line.startsWith('remote '))).toBe( + false, + ); + expect(invocations.some((line) => line.startsWith('branch '))).toBe( + false, + ); + } finally { + rmSync(spyRoot, { recursive: true, force: true }); + rmSync(shimDir, { recursive: true, force: true }); + } + }, + 240_000, + ); +}); diff --git a/packages/tools/src/tools/ast-edit/__tests__/ast-read-tool-host.ts b/packages/tools/src/tools/ast-edit/__tests__/ast-read-tool-host.ts new file mode 100644 index 0000000000..741b78c41f --- /dev/null +++ b/packages/tools/src/tools/ast-edit/__tests__/ast-read-tool-host.ts @@ -0,0 +1,43 @@ +/** + * @license + * Copyright 2026 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + * + * Bun-test-free IToolHost stub for the ast_read_file suites. + * + * This module deliberately imports nothing from `bun:test` so that child + * processes (for example the memory-regression fixture) can construct the + * exact same host as the in-process tests instead of duplicating the stub. + */ + +import type { IToolHost } from '../../../interfaces/IToolHost.js'; + +/** Build the minimal real IToolHost used by every ast_read_file fixture. */ +export function createAstReadToolHost(targetDir: string): IToolHost { + return { + getTargetDir: () => targetDir, + getWorkspaceRoots: () => [targetDir], + getApprovalMode: () => 'auto', + setApprovalMode: () => {}, + isInteractive: () => false, + hasFeatureFlag: () => false, + getFileService: () => ({ + shouldGitIgnoreFile: () => false, + shouldLlxprtIgnoreFile: () => false, + shouldIgnoreFile: () => false, + filterFiles: (paths: string[]) => paths, + }), + getFileFilteringOptions: () => ({ + respectGitIgnore: true, + respectLlxprtIgnore: true, + }), + getFileExclusions: () => [], + getReadManyFilesExclusions: () => [], + getFileFilteringRespectLlxprtIgnore: () => true, + getLlxprtIgnoreFilePath: () => null, + recordFileRead: () => {}, + getLlxprtIgnorePatterns: () => [], + getEphemeralSettings: () => ({}), + getDebugMode: () => false, + }; +} diff --git a/packages/tools/src/tools/ast-edit/__tests__/test-helpers.ts b/packages/tools/src/tools/ast-edit/__tests__/test-helpers.ts index 628f040687..d0bdb2d19f 100644 --- a/packages/tools/src/tools/ast-edit/__tests__/test-helpers.ts +++ b/packages/tools/src/tools/ast-edit/__tests__/test-helpers.ts @@ -19,6 +19,7 @@ import { beforeEach, afterEach } from 'bun:test'; import type { IToolHost } from '../../../interfaces/IToolHost.js'; import type { ASTEditTool, ASTEditToolParams } from '../../ast-edit.js'; import type { ToolResult } from '../../tools.js'; +import { createAstReadToolHost } from './ast-read-tool-host.js'; export function createTempDir(prefix = 'llxprt-ast-edit-test-'): { dir: string; @@ -70,33 +71,13 @@ export function useTempDir(): { readonly tempDir: string } { }; } +/** + * The single IToolHost stub for ast-edit fixtures. The implementation lives + * in the bun-test-free ast-read-tool-host.ts so child-process fixtures build + * the identical host; this keeps the in-process and child code paths honest. + */ export function createFakeToolHost(targetDir: string): IToolHost { - return { - getTargetDir: () => targetDir, - getWorkspaceRoots: () => [targetDir], - getApprovalMode: () => 'auto', - setApprovalMode: () => {}, - isInteractive: () => false, - hasFeatureFlag: () => false, - getFileService: () => ({ - shouldGitIgnoreFile: () => false, - shouldLlxprtIgnoreFile: () => false, - shouldIgnoreFile: () => false, - filterFiles: (paths: string[]) => paths, - }), - getFileFilteringOptions: () => ({ - respectGitIgnore: true, - respectLlxprtIgnore: true, - }), - getFileExclusions: () => [], - getReadManyFilesExclusions: () => [], - getFileFilteringRespectLlxprtIgnore: () => true, - getLlxprtIgnoreFilePath: () => null, - recordFileRead: () => {}, - getLlxprtIgnorePatterns: () => [], - getEphemeralSettings: () => ({}), - getDebugMode: () => false, - }; + return createAstReadToolHost(targetDir); } /** diff --git a/packages/tools/src/tools/ast-edit/ast-query-extractor.ts b/packages/tools/src/tools/ast-edit/ast-query-extractor.ts index 1f15fd4c51..2e212ee813 100644 --- a/packages/tools/src/tools/ast-edit/ast-query-extractor.ts +++ b/packages/tools/src/tools/ast-edit/ast-query-extractor.ts @@ -16,6 +16,110 @@ import { KEYWORDS, COMMENT_PREFIXES } from './constants.js'; /** * ASTQueryExtractor handles AST parsing with @ast-grep/napi and declaration extraction. */ +type SgNode = ReturnType['root']>; + +/** Declaration-bearing AST node kinds per supported language family. */ +const JS_DECLARATION_KINDS = [ + 'function_declaration', + 'method_definition', + 'class_declaration', + 'variable_declarator', + 'import_statement', +] as const; + +const PY_DECLARATION_KINDS = [ + 'function_definition', + 'class_definition', +] as const; + +const RS_DECLARATION_KINDS = [ + 'function_item', + 'struct_item', + 'trait_item', + 'enum_item', + 'impl_item', +] as const; + +const C_DECLARATION_KINDS = [ + 'function_definition', + 'declaration', + 'struct_specifier', + 'union_specifier', + 'enum_specifier', + 'type_definition', +] as const; + +/** + * Declaration kinds for an extension, or null when the extension has no + * declaration family and extraction falls back to line scanning. + */ +function declarationKindsFor(extension: string): ReadonlySet | null { + const family = familyOfExtension(extension); + return family === null + ? null + : new Set(DECLARATION_KINDS_BY_FAMILY[family]); +} + +/** Declaration-mapping family names for extensions with AST mappings. */ +type DeclarationFamily = 'js' | 'py' | 'rs' | 'c'; + +const DECLARATION_KINDS_BY_FAMILY: Readonly< + Record +> = { + js: JS_DECLARATION_KINDS, + py: PY_DECLARATION_KINDS, + rs: RS_DECLARATION_KINDS, + c: C_DECLARATION_KINDS, +}; + +/** + * Resolve a file extension to its declaration-mapping family, or null when + * the extension has none. An extension with an ast-grep mapping but no + * family (cpp, ruby, go, java, ...) resolves to null so it is never + * silently interpreted with another family's declaration kinds. + */ +function familyOfExtension(extension: string): DeclarationFamily | null { + if (JAVASCRIPT_FAMILY_EXTENSIONS.includes(extension)) return 'js'; + if (extension === 'py') return 'py'; + if (extension === 'rs') return 'rs'; + if (extension === 'c' || extension === 'h') return 'c'; + return null; +} + +const DECLARATION_FAMILIES: readonly DeclarationFamily[] = [ + 'js', + 'py', + 'rs', + 'c', +]; + +/** Narrow a string to a declaration family name. */ +function isDeclarationFamily(value: string): value is DeclarationFamily { + return (DECLARATION_FAMILIES as readonly string[]).includes(value); +} + +/** + * Narrow an ast-grep node kind (typed generically by the napi bindings) to a + * string before testing membership in the declaration-kind set. + */ +function kindIn(kinds: ReadonlySet, kind: unknown): boolean { + return typeof kind === 'string' && kinds.has(kind); +} + +/** Map a Rust struct/trait/enum node kind to its declaration type label. */ +function rustKindToType(kind: string): Declaration['type'] { + if (kind === 'struct_item') return 'struct'; + if (kind === 'trait_item') return 'trait'; + return 'enum'; +} + +/** Map a C struct/union/enum node kind to its declaration type label. */ +function cKindToType(kind: string): Declaration['type'] { + if (kind === 'struct_specifier') return 'struct'; + if (kind === 'union_specifier') return 'union'; + return 'enum'; +} + export class ASTQueryExtractor { constructor() {} @@ -39,240 +143,352 @@ export class ASTQueryExtractor { const declarations: EnhancedDeclaration[] = []; const sgRoot = root.root(); - if (JAVASCRIPT_FAMILY_EXTENSIONS.includes(extension)) { - this.extractJsFamilyDeclarations(sgRoot, declarations); - } else if (extension === 'py') { - this.extractPythonDeclarations(sgRoot, declarations); - } else if (extension === 'rs') { - this.extractRustDeclarations(sgRoot, declarations); - } else if (extension === 'c' || extension === 'h') { - this.extractCDeclarations(sgRoot, declarations); - } else { - return this.fallbackExtraction(content, extension); + const family = familyOfExtension(extension); + if (family === null) { + return this.fallbackExtraction(content); } + this.extractFamilyDeclarations(family, sgRoot, declarations); return declarations; } catch { - return this.fallbackExtraction(content, extension); + return this.fallbackExtraction(content); } } - private extractJsFamilyDeclarations( - sgRoot: ReturnType['root']>, - declarations: EnhancedDeclaration[], - ): void { - // Functions - sgRoot.findAll({ rule: { kind: 'function_declaration' } }).forEach((n) => { - const nameNode = n.field('name'); - const paramsNode = n.field('parameters'); - const returnTypeNode = n.field('return_type'); - if (nameNode != null) { - const signature = this.buildSignature(paramsNode, returnTypeNode); - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'function', signature), - ); + /** + * Bounded declaration acquisition with one-over sentinel semantics. + * + * Acquires declarations in document order but stops as soon as `limit` + * have been found, so at most `limit` declarations are ever materialized + * (no unbounded wrapper array is built and truncated afterwards). Callers + * pass remaining+1 to detect the first over-limit declaration via + * `result.length === limit`. + */ + async extractDeclarationsBounded( + filePath: string, + content: string, + limit: number, + ): Promise { + // Positive Infinity stays valid (the legacy unbounded fallback needs it); + // every other non-finite value, NaN above all, would silently disable + // the limit and materialize the whole file. + if (!Number.isFinite(limit) && limit !== Number.POSITIVE_INFINITY) { + throw new Error( + `extractDeclarationsBounded limit must be a number or Infinity, got: ${String(limit)}`, + ); + } + const boundedLimit = Math.max(0, Math.floor(limit)); + if (boundedLimit === 0) { + return []; + } + const extension = stringOrDefault( + filePath.split('.').pop(), + '', + ).toLowerCase(); + const lang = LANGUAGE_MAP[extension]; + if (!lang) { + return []; + } + const kinds = declarationKindsFor(extension); + try { + const sgRoot = parse(lang, content).root(); + if (kinds === null) { + return this.fallbackScan(content, boundedLimit); } - }); + return this.walkDeclarationsBounded( + sgRoot, + kinds, + extension, + boundedLimit, + ); + } catch { + return this.fallbackScan(content, boundedLimit); + } + } - // Methods - sgRoot.findAll({ rule: { kind: 'method_definition' } }).forEach((n) => { - const nameNode = n.field('name'); - const paramsNode = n.field('parameters'); - const returnTypeNode = n.field('return_type'); - if (nameNode != null) { - const signature = this.buildSignature(paramsNode, returnTypeNode); - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'function', signature), - ); + /** + * Explicit-stack pre-order walk that early-exits at the limit and cannot + * overflow the JS stack on deeply nested input. Extracted to keep the + * caller's nesting depth within the lint policy. + */ + private walkDeclarationsBounded( + sgRoot: SgNode, + kinds: ReadonlySet, + extension: string, + boundedLimit: number, + ): readonly EnhancedDeclaration[] { + const declarations: EnhancedDeclaration[] = []; + const stack: SgNode[] = [sgRoot]; + while (stack.length > 0 && declarations.length < boundedLimit) { + const node = stack.pop(); + if (node === undefined) { + break; } - }); - - // Classes - sgRoot.findAll({ rule: { kind: 'class_declaration' } }).forEach((n) => { - const nameNode = n.field('name'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'class')); + if (kindIn(kinds, node.kind())) { + const declaration = this.declarationForNode(extension, node); + if (declaration !== null) { + declarations.push(declaration); + } } - }); - - // Variables - sgRoot.findAll({ rule: { kind: 'variable_declarator' } }).forEach((n) => { - const nameNode = n.field('name'); - if (nameNode != null) { - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'variable'), - ); + const children = node.children(); + for (let index = children.length - 1; index >= 0; index--) { + stack.push(children[index]); } - }); + } + return declarations; + } - // Imports - sgRoot.findAll({ rule: { kind: 'import_statement' } }).forEach((n) => { - const sourceNode = n.field('source'); - declarations.push( - this.nodeToDeclaration( - n, - sourceNode != null ? sourceNode.text() : 'import', - 'import', - ), - ); - }); + private extractFamilyDeclarations( + family: DeclarationFamily, + sgRoot: SgNode, + declarations: EnhancedDeclaration[], + ): void { + this.collectAllByKind( + family, + sgRoot, + DECLARATION_KINDS_BY_FAMILY[family], + declarations, + ); } - private extractPythonDeclarations( - sgRoot: ReturnType['root']>, + /** + * Unbounded extraction used by paths that need every declaration: visits + * kinds in the established per-family order (which the bounded walk's + * document order intentionally does not need to match). + */ + private collectAllByKind( + family: 'js' | 'py' | 'rs' | 'c', + sgRoot: SgNode, + kinds: readonly string[], declarations: EnhancedDeclaration[], ): void { - sgRoot.findAll({ rule: { kind: 'function_definition' } }).forEach((n) => { - const nameNode = n.field('name'); - const paramsNode = n.field('parameters'); - const returnTypeNode = n.field('return_type'); - if (nameNode != null) { - const signature = this.buildPythonSignature(paramsNode, returnTypeNode); - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'function', signature), - ); + for (const kind of kinds) { + for (const node of sgRoot.findAll({ rule: { kind } })) { + const declaration = this.declarationForNode(family, node); + if (declaration !== null) { + declarations.push(declaration); + } } - }); + } + } - sgRoot.findAll({ rule: { kind: 'class_definition' } }).forEach((n) => { - const nameNode = n.field('name'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'class')); - } - }); + /** Map one AST node to a declaration, or null when it is not one. */ + private declarationForNode( + familyOrExtension: string, + node: SgNode, + ): EnhancedDeclaration | null { + if (isDeclarationFamily(familyOrExtension)) { + return this.familyDeclarationForNode(familyOrExtension, node); + } + const family = familyOfExtension(familyOrExtension); + return family === null ? null : this.familyDeclarationForNode(family, node); } - private extractRustDeclarations( - sgRoot: ReturnType['root']>, - declarations: EnhancedDeclaration[], - ): void { - sgRoot.findAll({ rule: { kind: 'function_item' } }).forEach((n) => { - const nameNode = n.field('name'); - const paramsNode = n.field('parameters'); - const returnTypeNode = n.field('return_type'); - if (nameNode != null) { - const signature = this.buildPythonSignature(paramsNode, returnTypeNode); - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'function', signature), + /** + * Map one AST node to a declaration for a resolved family. Exhaustive by + * construction: every family is handled and an unreachable value yields + * no declaration rather than an implicit fallthrough. + */ + private familyDeclarationForNode( + family: DeclarationFamily, + node: SgNode, + ): EnhancedDeclaration | null { + switch (family) { + case 'js': + return this.jsDeclarationForNode(node); + case 'py': + return this.pythonDeclarationForNode(node); + case 'rs': + return this.rustDeclarationForNode(node); + case 'c': + return this.cDeclarationForNode(node); + default: + return null; + } + } + + private jsDeclarationForNode(node: SgNode): EnhancedDeclaration | null { + switch (node.kind()) { + case 'function_declaration': + case 'method_definition': { + const nameNode = node.field('name'); + if (nameNode === null) return null; + const signature = this.buildSignature( + node.field('parameters'), + node.field('return_type'), + ); + return this.nodeToDeclaration( + node, + nameNode.text(), + 'function', + signature, ); } - }); - - sgRoot.findAll({ rule: { kind: 'struct_item' } }).forEach((n) => { - const nameNode = n.field('name'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'struct')); + case 'class_declaration': { + const nameNode = node.field('name'); + return nameNode !== null + ? this.nodeToDeclaration(node, nameNode.text(), 'class') + : null; } - }); - - sgRoot.findAll({ rule: { kind: 'trait_item' } }).forEach((n) => { - const nameNode = n.field('name'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'trait')); + case 'variable_declarator': { + const nameNode = node.field('name'); + return nameNode !== null + ? this.nodeToDeclaration(node, nameNode.text(), 'variable') + : null; } - }); - - sgRoot.findAll({ rule: { kind: 'enum_item' } }).forEach((n) => { - const nameNode = n.field('name'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'enum')); + case 'import_statement': { + const sourceNode = node.field('source'); + return this.nodeToDeclaration( + node, + sourceNode !== null ? sourceNode.text() : 'import', + 'import', + ); } - }); + default: + return null; + } + } - sgRoot.findAll({ rule: { kind: 'impl_item' } }).forEach((n) => { - const nameNode = n.field('type'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'impl')); + private pythonDeclarationForNode(node: SgNode): EnhancedDeclaration | null { + switch (node.kind()) { + case 'function_definition': { + const nameNode = node.field('name'); + if (nameNode === null) return null; + const signature = this.buildPythonSignature( + node.field('parameters'), + node.field('return_type'), + ); + return this.nodeToDeclaration( + node, + nameNode.text(), + 'function', + signature, + ); } - }); + case 'class_definition': { + const nameNode = node.field('name'); + return nameNode !== null + ? this.nodeToDeclaration(node, nameNode.text(), 'class') + : null; + } + default: + return null; + } } - private extractCDeclarations( - sgRoot: ReturnType['root']>, - declarations: EnhancedDeclaration[], - ): void { - sgRoot.findAll({ rule: { kind: 'function_definition' } }).forEach((n) => { - const fdec = n.find({ rule: { kind: 'function_declarator' } }); - const nameNode = fdec?.find({ rule: { kind: 'identifier' } }); - const paramsNode = fdec?.find({ rule: { kind: 'parameter_list' } }); - if (nameNode != null) { - const signature = paramsNode != null ? paramsNode.text() : '()'; - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'function', signature), + private rustDeclarationForNode(node: SgNode): EnhancedDeclaration | null { + switch (node.kind()) { + case 'function_item': { + const nameNode = node.field('name'); + if (nameNode === null) return null; + const signature = this.buildPythonSignature( + node.field('parameters'), + node.field('return_type'), + ); + return this.nodeToDeclaration( + node, + nameNode.text(), + 'function', + signature, ); } - }); - - // Function prototypes: `void init(Vec *v);` parse as `declaration` nodes - // containing a `function_declarator` child (no body). Function-pointer - // variables (`void (*fp)(int);`) also contain a `function_declarator`, but - // its name sits inside a `parenthesized_declarator` rather than a direct - // `identifier` child — those are variables, not prototypes, so skip them. - sgRoot.findAll({ rule: { kind: 'declaration' } }).forEach((n) => { - const fdec = n.find({ rule: { kind: 'function_declarator' } }); - if (fdec == null) return; - const nameNode = fdec.children().find((c) => c.kind() === 'identifier'); - if (nameNode == null) return; - const paramsNode = fdec.find({ rule: { kind: 'parameter_list' } }); - const signature = paramsNode != null ? paramsNode.text() : '()'; - declarations.push( - this.nodeToDeclaration(n, nameNode.text(), 'function', signature), - ); - }); - - sgRoot.findAll({ rule: { kind: 'struct_specifier' } }).forEach((n) => { - const nameNode = n.children().find((c) => c.kind() === 'type_identifier'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'struct')); + case 'struct_item': + case 'trait_item': + case 'enum_item': { + const nameNode = node.field('name'); + if (nameNode === null) return null; + const type = rustKindToType(String(node.kind())); + return this.nodeToDeclaration(node, nameNode.text(), type); } - }); - - sgRoot.findAll({ rule: { kind: 'union_specifier' } }).forEach((n) => { - const nameNode = n.children().find((c) => c.kind() === 'type_identifier'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'union')); + case 'impl_item': { + const nameNode = node.field('type'); + return nameNode !== null + ? this.nodeToDeclaration(node, nameNode.text(), 'impl') + : null; } - }); + default: + return null; + } + } - sgRoot.findAll({ rule: { kind: 'enum_specifier' } }).forEach((n) => { - const nameNode = n.children().find((c) => c.kind() === 'type_identifier'); - if (nameNode != null) { - declarations.push(this.nodeToDeclaration(n, nameNode.text(), 'enum')); + private cDeclarationForNode(node: SgNode): EnhancedDeclaration | null { + switch (node.kind()) { + case 'function_definition': + return this.cFunctionForNode(node); + case 'declaration': + // Function prototypes: `void init(Vec *v);` parse as `declaration` + // nodes containing a `function_declarator` child (no body). + // Function-pointer variables (`void (*fp)(int);`) also contain a + // `function_declarator`, but its name sits inside a + // `parenthesized_declarator` rather than a direct `identifier` + // child — those are variables, not prototypes, so skip them. + return this.cPrototypeForNode(node); + case 'struct_specifier': + case 'union_specifier': + case 'enum_specifier': { + const nameNode = node + .children() + .find((child) => child.kind() === 'type_identifier'); + if (nameNode == null) return null; + const type = cKindToType(String(node.kind())); + return this.nodeToDeclaration(node, nameNode.text(), type); } - }); - - sgRoot.findAll({ rule: { kind: 'type_definition' } }).forEach((n) => { - const name = findCTypedefName(n); - if (name !== null) { - declarations.push(this.nodeToDeclaration(n, name, 'typedef')); + case 'type_definition': { + const name = findCTypedefName(node); + return name !== null + ? this.nodeToDeclaration(node, name, 'typedef') + : null; } - }); + default: + return null; + } + } + + private cFunctionForNode(node: SgNode): EnhancedDeclaration | null { + const declarator = node.find({ rule: { kind: 'function_declarator' } }); + const nameNode = declarator?.find({ rule: { kind: 'identifier' } }); + if (nameNode == null) return null; + const paramsNode = declarator?.find({ rule: { kind: 'parameter_list' } }); + const signature = paramsNode != null ? paramsNode.text() : '()'; + return this.nodeToDeclaration(node, nameNode.text(), 'function', signature); + } + + private cPrototypeForNode(node: SgNode): EnhancedDeclaration | null { + const declarator = node.find({ rule: { kind: 'function_declarator' } }); + if (declarator === null) return null; + const nameNode = declarator + .children() + .find((child) => child.kind() === 'identifier'); + if (nameNode == null) return null; + const paramsNode = declarator.find({ rule: { kind: 'parameter_list' } }); + const signature = paramsNode !== null ? paramsNode.text() : '()'; + return this.nodeToDeclaration(node, nameNode.text(), 'function', signature); } private buildSignature( - paramsNode: ReturnType['root']> | null, - returnTypeNode: ReturnType['root']> | null, + paramsNode: SgNode | null, + returnTypeNode: SgNode | null, ): string { - let signature = paramsNode != null ? paramsNode.text() : '()'; - if (returnTypeNode != null) { + let signature = paramsNode !== null ? paramsNode.text() : '()'; + if (returnTypeNode !== null) { signature += returnTypeNode.text(); } return signature; } private buildPythonSignature( - paramsNode: ReturnType['root']> | null, - returnTypeNode: ReturnType['root']> | null, + paramsNode: SgNode | null, + returnTypeNode: SgNode | null, ): string { - let signature = paramsNode != null ? paramsNode.text() : '()'; - if (returnTypeNode != null) { + let signature = paramsNode !== null ? paramsNode.text() : '()'; + if (returnTypeNode !== null) { signature += ` -> ${returnTypeNode.text()}`; } return signature; } private nodeToDeclaration( - n: ReturnType['root']>, + n: SgNode, name: string, type: Declaration['type'], signature?: string, @@ -292,37 +508,29 @@ export class ASTQueryExtractor { }; } - private fallbackExtraction( - content: string, - _language: string, - ): EnhancedDeclaration[] { - // Keep the regex-based fallback for robustness - const declarations: Declaration[] = []; - const lines = content.split('\n'); + private fallbackExtraction(content: string): EnhancedDeclaration[] { + return this.fallbackScan(content, Number.POSITIVE_INFINITY); + } - lines.forEach((line, index) => { - const trimmed = line.trim(); - const isComment = COMMENT_PREFIXES.some((prefix) => - trimmed.startsWith(prefix), - ); - if (!trimmed || isComment) return; - - if ( - line.includes(KEYWORDS.FUNCTION) || - line.includes(KEYWORDS.DEF) || - line.includes(KEYWORDS.CLASS) - ) { - const name = this.extractNameBasic(trimmed); - const column = Math.max(0, line.indexOf(name)); - declarations.push({ - name, - type: trimmed.includes(KEYWORDS.CLASS) ? 'class' : 'function', - line: index + 1, - column, - signature: this.extractSignatureBasic(trimmed), - }); + /** + * Line-scan fallback bounded by a declaration limit: stops scanning as + * soon as the limit is reached so over-limit inputs never materialize + * fully (used by the bounded working-set path and, unbounded, as the + * legacy fallback). + */ + private fallbackScan(content: string, limit: number): EnhancedDeclaration[] { + const lines = content.split('\n'); + const declarations: Declaration[] = []; + for (const [index, line] of lines.entries()) { + if (declarations.length >= limit) { + // The declaration limit is reached: stop iterating instead of + // scanning every remaining line only to skip it. + break; } - }); + if (this.isDeclarationLine(line.trim())) { + this.pushFallbackDeclaration(declarations, line, index); + } + } return declarations.map((decl) => ({ ...decl, @@ -335,6 +543,46 @@ export class ASTQueryExtractor { })); } + /** + * True when a trimmed line is a non-blank, non-comment line that contains + * a declaration keyword (function, def, or class). Extracted to keep the + * scan loop's break/continue count within the lint policy. + */ + private isDeclarationLine(trimmed: string): boolean { + if (!trimmed) { + return false; + } + const isComment = COMMENT_PREFIXES.some((prefix) => + trimmed.startsWith(prefix), + ); + if (isComment) { + return false; + } + return ( + trimmed.includes(KEYWORDS.FUNCTION) || + trimmed.includes(KEYWORDS.DEF) || + trimmed.includes(KEYWORDS.CLASS) + ); + } + + /** Push one fallback declaration from a scanned declaration line. */ + private pushFallbackDeclaration( + declarations: Declaration[], + line: string, + index: number, + ): void { + const name = this.extractNameBasic(line.trim()); + declarations.push({ + name, + type: line.includes(KEYWORDS.CLASS) ? 'class' : 'function', + line: index + 1, + // Column must reflect the raw line: trimming first would report the + // name's offset inside the trimmed text and lose the indentation. + column: Math.max(0, line.indexOf(name)), + signature: this.extractSignatureBasic(line.trim()), + }); + } + private extractNameBasic(line: string): string { // Use string scanning instead of regex to avoid polynomial backtracking. for (const keyword of ['function', 'def', 'class']) { diff --git a/packages/tools/src/tools/ast-edit/ast-read-file-invocation.ts b/packages/tools/src/tools/ast-edit/ast-read-file-invocation.ts index 328ca17c3a..1a2379155c 100644 --- a/packages/tools/src/tools/ast-edit/ast-read-file-invocation.ts +++ b/packages/tools/src/tools/ast-edit/ast-read-file-invocation.ts @@ -24,9 +24,83 @@ import { import type { IToolHost } from '../../interfaces/index.js'; import type { LiveOutputUpdate } from '../../utils/terminalSerializer.js'; -import type { ASTReadFileToolParams } from './types.js'; +import type { + ASTReadFileToolParams, + WorkingSetAcquisitionStatus, + WorkingSetPartialReason, +} from './types.js'; import { ASTConfig } from './ast-config.js'; import type { ASTContextCollector } from './context-collector.js'; +import { + MAX_WORKING_SET_DECLARATIONS, + MAX_WORKING_SET_FILES, +} from './workspace-context-provider.js'; + +/** + * Rendered phrase per partial reason. The Record over the full + * WorkingSetPartialReason union is compile-time exhaustive: a future union + * member without a phrase fails typecheck instead of silently rendering a + * generic fallback at runtime. + */ +const WORKING_SET_PARTIAL_PHRASES: Readonly< + Record +> = { + 'file-count': `stopped at the file-count limit (${MAX_WORKING_SET_FILES})`, + 'source-bytes': 'stopped at the aggregate source-byte budget', + declarations: `stopped at the retained-declaration limit (${MAX_WORKING_SET_DECLARATIONS})`, + cancelled: 'cancelled before completion', + 'git-error': 'stopped because Git working-set discovery failed', + 'discovery-limit': 'stopped at the working-set discovery limit', + 'skipped-files': 'partial because eligible working-set files were skipped', +}; + +function workingSetPartialPhrase( + reason: WorkingSetPartialReason | undefined, +): string { + if (reason === undefined) { + return 'stopped early'; + } + return WORKING_SET_PARTIAL_PHRASES[reason]; +} + +/** + * Eligible-file count for the partial header. When discovery was truncated, + * the file-count policy stopped acquisition, Git discovery failed after + * candidates were observed, or cancellation stopped acquisition mid-run, the + * counted candidates are only a lower bound on the true eligible set. + */ +function eligibleFilesPhrase(status: WorkingSetAcquisitionStatus): string { + const isLowerBound = + status.discoveryTruncated || + status.partialReason === 'file-count' || + status.partialReason === 'git-error' || + status.partialReason === 'cancelled'; + return isLowerBound + ? `at least ${status.eligibleFiles}` + : String(status.eligibleFiles); +} + +/** Bounded skip accounting rendered to the model alongside partial context. */ +function skipAccountingLines( + status: WorkingSetAcquisitionStatus | undefined, +): string[] { + if (status === undefined) { + return []; + } + const skippedParts: string[] = []; + if (status.oversizedFiles > 0) { + skippedParts.push(`${status.oversizedFiles} oversized`); + } + if (status.skippedFiles > 0) { + skippedParts.push(`${status.skippedFiles} unreadable`); + } + if (status.missingFiles > 0) { + skippedParts.push(`${status.missingFiles} missing`); + } + return skippedParts.length > 0 + ? [`- (skipped: ${skippedParts.join(', ')})`] + : []; +} export class ASTReadFileToolInvocation implements ToolInvocation @@ -54,7 +128,7 @@ export class ASTReadFileToolInvocation } async execute( - _signal?: AbortSignal, + signal?: AbortSignal, _updateOutput?: (update: LiveOutputUpdate) => void, _terminalColumns?: number, _terminalRows?: number, @@ -100,6 +174,13 @@ export class ASTReadFileToolInvocation this.params.file_path, content, workspaceRoot, + { + // The read path renders none of the repository/related-symbol + // results, so their native whole-workspace searches are opted + // out; local analysis and the working set are still collected. + collectRepositoryContext: false, + signal, + }, ); const readLlmContent = this.buildReadLlmContent( @@ -190,16 +271,27 @@ export class ASTReadFileToolInvocation >, workspaceRoot: string, ): string[] { - if ( - !enhancedContext.connectedFiles || - enhancedContext.connectedFiles.length === 0 - ) { + const status = enhancedContext.workingSetStatus; + const connectedFiles = enhancedContext.connectedFiles ?? []; + if (connectedFiles.length === 0) { + if (status !== undefined && !status.complete) { + return [ + '', + `WORKING SET CONTEXT (partial: ${workingSetPartialPhrase(status.partialReason)}; no working-set files retained):`, + ...skipAccountingLines(status), + ]; + } return []; } + const header = + status !== undefined && !status.complete + ? `WORKING SET CONTEXT (partial: ${workingSetPartialPhrase(status.partialReason)}; retained ${status.retainedFiles} of ${eligibleFilesPhrase(status)} files, ${status.retainedDeclarations} declarations, ${status.retainedSourceBytes} source bytes):` + : 'WORKING SET CONTEXT:'; return [ '', - 'WORKING SET CONTEXT:', - ...enhancedContext.connectedFiles + header, + ...skipAccountingLines(status), + ...connectedFiles .map((file) => { const relPath = makeRelative(file.filePath, workspaceRoot); if (file.declarations.length === 0) diff --git a/packages/tools/src/tools/ast-edit/context-collector.ts b/packages/tools/src/tools/ast-edit/context-collector.ts index ffd2ed4122..8e35cd6da4 100644 --- a/packages/tools/src/tools/ast-edit/context-collector.ts +++ b/packages/tools/src/tools/ast-edit/context-collector.ts @@ -68,6 +68,24 @@ function scoreDeclaration(decl: EnhancedDeclaration): number { return score; } +/** + * Caller-specific enhanced-context collection options. + */ +export interface EnhancedContextOptions { + /** Collect Git working-set context (default true). */ + collectWorkingSet?: boolean; + /** + * Collect repository relationship context: repository metadata, the + * workspace symbol index, related files, and native whole-workspace + * related-symbol searches (default true). Callers whose output never + * consumes these results (ast_read_file) opt out deliberately: the + * searches are otherwise unobservable dead work with native memory cost. + */ + collectRepositoryContext?: boolean; + /** Cancellation signal threaded through LLxprt-owned acquisition. */ + signal?: AbortSignal; +} + /** * Orchestrates all AST context gathering: local analysis, working set, and cross-file relationships. */ @@ -76,8 +94,13 @@ export class ASTContextCollector { private repoProvider: RepositoryContextProvider; private relationshipAnalyzer: CrossFileRelationshipAnalyzer; - constructor() { - this.astExtractor = new ASTQueryExtractor(); + /** + * The extractor defaults to the real production instance; passing a real + * subclass (for example one that observes bounded acquisition timing) + * keeps every other behavior of the collector intact. + */ + constructor(astExtractor: ASTQueryExtractor = new ASTQueryExtractor()) { + this.astExtractor = astExtractor; this.repoProvider = new RepositoryContextProvider(); this.relationshipAnalyzer = new CrossFileRelationshipAnalyzer(); } @@ -105,9 +128,10 @@ export class ASTContextCollector { targetFilePath: string, content: string, workspaceRoot: string, - options?: { collectWorkingSet?: boolean }, + options?: EnhancedContextOptions, ): Promise { const collectWorkingSet = options?.collectWorkingSet ?? true; + const collectRepositoryContext = options?.collectRepositoryContext ?? true; const startTime = Date.now(); const startMemory = process.memoryUsage().heapUsed; @@ -129,51 +153,58 @@ export class ASTContextCollector { // Phase 2: Working Set Context (Git-based). Suppressed only for callers // (ast_edit preview) that opt out; ast_read_file keeps the working set. if (collectWorkingSet) { - const connectedFiles = await enrichWithWorkingSetContext( + const workingSet = await enrichWithWorkingSetContext( targetFilePath, workspaceRoot, this.repoProvider, this.astExtractor, + options?.signal, ); - enhancedContext.connectedFiles = connectedFiles; + enhancedContext.connectedFiles = workingSet.files; + enhancedContext.workingSetStatus = workingSet.status; } - // Phase 3: Repository context and Cross-file Relationships - const repoContext = - await this.repoProvider.collectRepositoryContext(workspaceRoot); - enhancedContext.repositoryContext = repoContext ?? undefined; - - // [CCR] Relation: Cross-file relationship analysis segment. - // Reason: Optimized to use on-demand findInFiles instead of eager indexing. - if (repoContext) { - if (ASTConfig.ENABLE_SYMBOL_INDEXING) { - const workspaceFiles = await getWorkspaceFiles(workspaceRoot); - await this.relationshipAnalyzer.buildSymbolIndex(workspaceFiles); - - const relatedFiles = - await this.relationshipAnalyzer.findRelatedFiles(targetFilePath); - enhancedContext.relatedFiles = relatedFiles; + // Phase 3: Repository context and Cross-file Relationships. Skipped + // entirely for callers (ast_read_file) that opt out: none of these + // results reach their model-facing or display output, and the + // related-symbol searches are native whole-workspace traversals. + if (collectRepositoryContext) { + const repoContext = + await this.repoProvider.collectRepositoryContext(workspaceRoot); + enhancedContext.repositoryContext = repoContext ?? undefined; + + // [CCR] Relation: Cross-file relationship analysis segment. + // Reason: Optimized to use on-demand findInFiles instead of eager indexing. + if (repoContext) { + if (ASTConfig.ENABLE_SYMBOL_INDEXING) { + const workspaceFiles = await getWorkspaceFiles(workspaceRoot); + await this.relationshipAnalyzer.buildSymbolIndex(workspaceFiles); + + const relatedFiles = + await this.relationshipAnalyzer.findRelatedFiles(targetFilePath); + enhancedContext.relatedFiles = relatedFiles; + } + + // Prioritize symbols for Lazy search + const topSymbols = prioritizeSymbolsFromDeclarations( + enhancedContext.declarations, + ); + + // Execute atomic queries with strict limits and Survivability (Promise.allSettled) + const relatedSymbolsTasks = topSymbols.map((symbol) => + this.relationshipAnalyzer.findRelatedSymbols(symbol, workspaceRoot), + ); + + const relatedSymbolsResults = + await Promise.allSettled(relatedSymbolsTasks); + enhancedContext.relatedSymbols = relatedSymbolsResults + .filter( + (r): r is PromiseFulfilledResult => + r.status === 'fulfilled', + ) + .map((r) => r.value) + .flat(); } - - // Prioritize symbols for Lazy search - const topSymbols = prioritizeSymbolsFromDeclarations( - enhancedContext.declarations, - ); - - // Execute atomic queries with strict limits and Survivability (Promise.allSettled) - const relatedSymbolsTasks = topSymbols.map((symbol) => - this.relationshipAnalyzer.findRelatedSymbols(symbol, workspaceRoot), - ); - - const relatedSymbolsResults = - await Promise.allSettled(relatedSymbolsTasks); - enhancedContext.relatedSymbols = relatedSymbolsResults - .filter( - (r): r is PromiseFulfilledResult => - r.status === 'fulfilled', - ) - .map((r) => r.value) - .flat(); } const duration = Date.now() - startTime; diff --git a/packages/tools/src/tools/ast-edit/repository-context-provider.ts b/packages/tools/src/tools/ast-edit/repository-context-provider.ts index 4eb006f8ef..2a45fce88c 100644 --- a/packages/tools/src/tools/ast-edit/repository-context-provider.ts +++ b/packages/tools/src/tools/ast-edit/repository-context-provider.ts @@ -4,13 +4,65 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { promises as fsPromises } from 'fs'; +import { existsSync } from 'fs'; import * as path from 'path'; -import { spawnSync } from 'child_process'; +import { spawn, spawnSync, type ChildProcess } from 'child_process'; +import { StringDecoder } from 'string_decoder'; import type { RepositoryContext } from './types.js'; const GIT_TIMEOUT_MS = 3000; const GIT_MAX_BUFFER = 1024 * 1024; +const GIT_ERROR_STDERR_LIMIT = 4096; +const RECENT_COMMIT_LIMIT = 5; + +/** Outcome of a bounded working-set Git discovery run. */ +export type WorkingSetDiscoveryOutcome = + /** Every Git phase ran to exhaustion below the candidate cap. */ + | 'complete' + /** Discovery stopped at the finite candidate cap: at least that many + * eligible files existed and more were never observed. */ + | 'truncated' + /** The AbortSignal fired; the exact in-flight Git child was terminated. */ + | 'aborted' + /** A Git phase exited nonzero (or failed to spawn/timed out). */ + | 'git-error' + /** The directory is not inside a Git work tree (or git is unavailable). */ + | 'no-working-set'; + +export interface WorkingSetDiscoveryOptions { + /** Hard cap on observed candidates (finite, at least 1). */ + readonly maxCandidates: number; + /** Cancellation signal; aborting terminates the exact in-flight child. */ + readonly signal?: AbortSignal; + /** Absolute path excluded from candidacy (the read target itself). */ + readonly excludePath?: string; +} + +export interface WorkingSetDiscoveryResult { + /** Deduplicated absolute candidate paths, bounded by maxCandidates. */ + readonly candidates: readonly string[]; + readonly outcome: WorkingSetDiscoveryOutcome; + /** Present iff outcome is git-error: the terminating Git failure. */ + readonly gitError?: string; +} + +/** Terminal result of one NUL-delimited Git listing phase. */ +interface GitPhaseResult { + readonly status: + | 'ok' + | 'truncated' + | 'aborted' + | 'git-error' + | 'output-overflow'; + /** Present iff status is git-error or output-overflow: the failure. */ + readonly error?: string; +} + +interface GitCaptureResult { + readonly kind: 'ok' | 'error' | 'aborted'; + readonly stdout?: string; + readonly error?: string; +} /** * RepositoryContextProvider handles git operations to collect repository context. @@ -41,62 +93,227 @@ export class RepositoryContextProvider { } /** - * Get the "Working Set" of files: - * 1. Unstaged changes (git diff --name-only) - * 2. Staged changes (git diff --name-only --cached) - * 3. Recent commits (git log -n --name-only) + * Discover working-set candidates under a finite bound. + * + * Git phases run asynchronously and incrementally: NUL-delimited names are + * decoded UTF-8-safely and observed one at a time, and the run stops at + * {@link WorkingSetDiscoveryOptions.maxCandidates} candidates (the finite + * count plus one-over sentinel semantics live with the caller's policy). + * The provided AbortSignal terminates exactly the in-flight child process, + * never anything broader. Git failures, truncation, and abort are surfaced + * as outcomes instead of collapsing into an empty "complete" set. Once the + * candidate cap is reached the observer is idempotent: stdout that was + * already buffered when the child was killed can never add another + * candidate past the cap. */ - async getWorkingSetFiles( + async discoverWorkingSetFiles( workspaceRoot: string, - limit: number = 5, - ): Promise { - const files = new Set(); + options: WorkingSetDiscoveryOptions, + ): Promise { + const { maxCandidates, signal, excludePath } = options; + if (!Number.isFinite(maxCandidates) || maxCandidates < 1) { + throw new Error( + `maxCandidates must be a positive finite number, got: ${String( + maxCandidates, + )}`, + ); + } + if (signal?.aborted ?? false) { + return { candidates: [], outcome: 'aborted' }; + } - try { - const execGit = (args: string[]) => { - const result = spawnSync('git', ['-C', workspaceRoot, ...args], { - encoding: 'utf-8', - stdio: 'pipe', - timeout: GIT_TIMEOUT_MS, - maxBuffer: GIT_MAX_BUFFER, - }); - return result.status === 0 ? result.stdout.trim() : ''; - }; + const inside = await this.runGitCapture( + workspaceRoot, + ['rev-parse', '--is-inside-work-tree'], + signal, + ); + if (inside.kind === 'aborted') { + return { candidates: [], outcome: 'aborted' }; + } + if (inside.kind !== 'ok' || (inside.stdout ?? '').trim() !== 'true') { + // A directory outside any repository has no working set. A repository + // whose metadata is broken (e.g. an unparseable HEAD) fails the same + // probe, so the on-disk .git presence decides between "no working set" + // and a surfaced Git failure. + if (inside.kind === 'error' && hasGitDirectory(workspaceRoot)) { + return { candidates: [], outcome: 'git-error', gitError: inside.error }; + } + return { candidates: [], outcome: 'no-working-set' }; + } - // 1. Unstaged changes - execGit(['diff', '--name-only', '-z']) - .split('\0') - .forEach((f) => f && files.add(f)); - - // 2. Staged changes - execGit(['diff', '--name-only', '--cached', '-z']) - .split('\0') - .forEach((f) => f && files.add(f)); - - // 3. Recent commits - // Note: -z works with --name-only in log but we need to ensure format doesn't break it. - // Safest is to rely on diffs for working set, but strictly following plan: - execGit(['log', `-n${limit}`, '--name-only', '--format=', '-z']) - .split('\0') - .forEach((f) => f && files.add(f)); - } catch { - // Git command failed; return what we have. + const addName = createAddNameFn(workspaceRoot, excludePath, maxCandidates); + const phases: string[][] = [ + ['diff', '--name-only', '-z'], + ['diff', '--name-only', '--cached', '-z'], + ]; + // A fresh repository without commits has no HEAD: the recent-commit + // phase is skipped rather than reported as a Git error. + const head = await this.runGitCapture( + workspaceRoot, + ['rev-parse', '--verify', '--quiet', 'HEAD'], + signal, + ); + if (head.kind === 'aborted') { + return { candidates: [], outcome: 'aborted' }; + } + if (head.kind === 'ok') { + phases.push([ + 'log', + `-n${RECENT_COMMIT_LIMIT}`, + '--name-only', + '--format=', + '-z', + ]); } - // Filter existing files and convert to absolute paths - const validFiles: string[] = []; - for (const file of files) { - if (!file.trim()) continue; - const absPath = path.resolve(workspaceRoot, file); - try { - await fsPromises.access(absPath); - validFiles.push(absPath); - } catch { - // File might be deleted + return this.collectPhaseCandidates(workspaceRoot, phases, signal, addName); + } + + /** Run each NUL-delimited Git phase and collect candidates until one stops. */ + private async collectPhaseCandidates( + workspaceRoot: string, + phases: readonly string[][], + signal: AbortSignal | undefined, + addName: AddNameFn, + ): Promise { + for (const args of phases) { + // A signal that fired between phases is already past every listener: + // an aborted signal never re-fires, so the flag is checked directly + // before another child is spawned. + if (signal?.aborted ?? false) { + return { candidates: addName.candidates, outcome: 'aborted' }; + } + const result = await this.runGitNulPhase( + workspaceRoot, + args, + signal, + addName, + ); + if (result.status === 'aborted') { + return { candidates: addName.candidates, outcome: 'aborted' }; + } + if (result.status === 'truncated') { + return { candidates: addName.candidates, outcome: 'truncated' }; + } + if (result.status === 'output-overflow') { + // The listing itself exceeded its bounded output allowance. This is + // a Git/discovery failure, not candidate-cap truncation: reporting + // "truncated" would claim at least N eligible files were observed, + // which was never established. + return { + candidates: addName.candidates, + outcome: 'git-error', + gitError: + result.error ?? 'git working-set listing exceeded its output limit', + }; + } + if (result.status === 'git-error') { + return { + candidates: addName.candidates, + outcome: 'git-error', + gitError: result.error ?? 'git working-set listing failed', + }; } } + return { candidates: addName.candidates, outcome: 'complete' }; + } + + /** Run one small Git command and capture stdout. Fails fast on spawn errors + * and nonzero exit; aborts and timeouts terminate only the exact child. + */ + private async runGitCapture( + workspaceRoot: string, + args: string[], + signal: AbortSignal | undefined, + ): Promise { + return new Promise((resolve) => { + let child: ChildProcess; + try { + child = spawn('git', ['-C', workspaceRoot, ...args], { + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch (error) { + resolve({ kind: 'error', error: describeSpawnError(error) }); + return; + } + let aborted = false; + let timedOut = false; + let stdout = ''; + let stderr = ''; + const onAbort = createAbortHandler(child, () => { + aborted = true; + }); + const timer = setTimeout(() => { + timedOut = true; + child.kill(); + }, GIT_TIMEOUT_MS); + const finish = createFinish(resolve, timer, signal, onAbort); + signal?.addEventListener('abort', onAbort, { once: true }); + // An already-aborted signal never fires a newly attached listener, so + // the flag is checked directly after wiring: the child is killed and + // the run settles as aborted instead of running to completion. + if (signal?.aborted ?? false) { + onAbort(); + } + child.stdout?.on('data', (chunk: Buffer) => { + stdout += chunk.toString('utf8'); + if (stdout.length > GIT_MAX_BUFFER) { + child.kill(); + } + }); + child.stderr?.on('data', (chunk: Buffer) => { + if (stderr.length < GIT_ERROR_STDERR_LIMIT) { + stderr += chunk.toString('utf8'); + } + }); + child.on('error', (error: Error) => { + finish({ kind: 'error', error: describeSpawnError(error) }); + }); + child.on('close', (code: number | null) => { + if (aborted) { + finish({ kind: 'aborted' }); + } else if (timedOut) { + finish({ + kind: 'error', + error: `git timed out after ${GIT_TIMEOUT_MS}ms`, + }); + } else if (stdout.length > GIT_MAX_BUFFER) { + // The overflow kill leaves a signaled exit; report the actual + // cause instead of a generic "exited with status null" line. + finish({ + kind: 'error', + error: 'git output exceeded the capture limit', + }); + } else if (code === 0) { + finish({ kind: 'ok', stdout }); + } else { + finish({ + kind: 'error', + error: stderr.trim() || `git exited with status ${String(code)}`, + }); + } + }); + }); + } - return validFiles; + /** + * Run one NUL-delimited Git listing phase, feeding each decoded name to + * the observer incrementally. Stops (terminating the exact child) when the + * observer reports its cap or the bounded output allowance is exceeded. + */ + private async runGitNulPhase( + workspaceRoot: string, + args: string[], + signal: AbortSignal | undefined, + onName: (name: string) => 'continue' | 'stop', + ): Promise { + const child = spawnGitChild(workspaceRoot, args); + if (child === null) { + return { status: 'git-error' }; + } + return new Promise((resolve) => { + wireGitNulPhase(child, signal, onName, resolve); + }); } private async getGitRemoteUrl(repoPath: string): Promise { @@ -149,3 +366,271 @@ export class RepositoryContextProvider { } } } + +/** Render a Git spawn failure as a bounded single-line description. */ +function describeSpawnError(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +/** Create an abort handler that marks the flag and kills the exact child. */ +function createAbortHandler( + child: ChildProcess, + onAbort: () => void, +): () => void { + return () => { + onAbort(); + child.kill(); + }; +} + +/** Create a settle-once finish function that cleans up timer and signal. */ +function createFinish( + resolve: (result: T) => void, + timer: NodeJS.Timeout, + signal: AbortSignal | undefined, + onAbort: () => void, +): (result: T) => void { + let settled = false; + return (result: T) => { + if (settled) { + return; + } + settled = true; + clearTimeout(timer); + signal?.removeEventListener('abort', onAbort); + resolve(result); + }; +} + +/** + * True when a .git entry exists at or above the directory. Used to separate + * "outside any repository" from a repository whose metadata is broken: both + * fail the same rev-parse probe, but only the latter is a Git failure. + */ +function hasGitDirectory(startDir: string): boolean { + let current = path.resolve(startDir); + for (;;) { + if (existsSync(path.join(current, '.git'))) { + return true; + } + const parent = path.dirname(current); + if (parent === current) { + return false; + } + current = parent; + } +} + +/** + * Create the incremental name observer used by discovery. Deduplicates names, + * resolves them to absolute paths, honors the exclude path, and stops when + * the candidate count reaches the cap. The observer is idempotent once the + * cap is reached: late names from stdout that was already buffered when the + * child was killed are refused instead of appended, so the candidate array + * can never exceed maxCandidates. The candidate array is captured in the + * closure and returned by the outer {@link RepositoryContextProvider} phase + * loop. + */ +function createAddNameFn( + workspaceRoot: string, + excludePath: string | undefined, + maxCandidates: number, +): AddNameFn { + const names = new Set(); + const candidates: string[] = []; + let capped = false; + const fn = (name: string): 'continue' | 'stop' => { + if (capped) { + return 'stop'; + } + if (!name.trim() || names.has(name)) { + return 'continue'; + } + names.add(name); + const absolute = path.resolve(workspaceRoot, name); + if (excludePath !== undefined && isSamePath(absolute, excludePath)) { + return 'continue'; + } + candidates.push(absolute); + if (candidates.length >= maxCandidates) { + capped = true; + return 'stop'; + } + return 'continue'; + }; + return Object.assign(fn, { candidates }); +} + +/** Platforms whose filesystems resolve paths case-insensitively. */ +const CASE_INSENSITIVE_PATH_PLATFORMS: ReadonlySet = new Set([ + 'win32', + 'darwin', +]); + +/** + * True when two absolute paths denote the same file. Git reports tracked + * names with their literal on-disk casing while a caller may hold an + * equivalently-spelled path in different case; on Windows and macOS those + * are the same file and must compare equal for exclusion. + */ +function isSamePath(left: string, right: string): boolean { + if (left === right) { + return true; + } + return ( + CASE_INSENSITIVE_PATH_PLATFORMS.has(process.platform) && + left.toLowerCase() === right.toLowerCase() + ); +} + +/** Add-name callback with a captured candidates array. */ +interface AddNameFn { + (name: string): 'continue' | 'stop'; + readonly candidates: string[]; +} + +/** Spawn a Git child process for NUL-delimited listing, or null on failure. */ +function spawnGitChild( + workspaceRoot: string, + args: readonly string[], +): ChildProcess | null { + try { + return spawn('git', ['-C', workspaceRoot, ...args], { + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch { + return null; + } +} + +/** Why a NUL-delimited listing phase stopped consuming its stream. */ +type PhaseStop = 'none' | 'capped' | 'overflow'; + +/** Wire all event handlers for one NUL-delimited Git listing phase. */ +function wireGitNulPhase( + child: ChildProcess, + signal: AbortSignal | undefined, + onName: (name: string) => 'continue' | 'stop', + resolve: (result: GitPhaseResult) => void, +): void { + let aborted = false; + let timedOut = false; + let stop: PhaseStop = 'none'; + const decoder = new StringDecoder('utf8'); + let pending = ''; + let observedBytes = 0; + let stderr = ''; + const onAbort = createAbortHandler(child, () => { + aborted = true; + }); + const timer = setTimeout(() => { + timedOut = true; + child.kill(); + }, GIT_TIMEOUT_MS); + const finish = createFinish(resolve, timer, signal, onAbort); + signal?.addEventListener('abort', onAbort, { once: true }); + // An already-aborted signal never fires a newly attached listener, so the + // flag is checked directly after wiring: the child is killed and the phase + // settles as aborted instead of streaming names for a cancelled run. + if (signal?.aborted ?? false) { + onAbort(); + } + + const consume = (text: string): boolean => { + pending += text; + let separator = pending.indexOf('\0'); + while (separator !== -1) { + const name = pending.slice(0, separator); + pending = pending.slice(separator + 1); + if (name.length > 0 && onName(name) === 'stop') { + return true; + } + separator = pending.indexOf('\0'); + } + return false; + }; + + child.stdout?.on('data', (chunk: Buffer) => { + // Idempotent stop: stdout that was already buffered when the child was + // killed keeps arriving, but no name is observed afterwards. Without + // this guard those late chunks push candidates past the cap. + if (stop !== 'none') { + return; + } + observedBytes += chunk.length; + if (consume(decoder.write(chunk))) { + stop = 'capped'; + child.kill(); + return; + } + if (observedBytes > GIT_MAX_BUFFER) { + stop = 'overflow'; + child.kill(); + } + }); + child.stderr?.on('data', (chunk: Buffer) => { + if (stderr.length < GIT_ERROR_STDERR_LIMIT) { + stderr += chunk.toString('utf8'); + } + }); + child.on('error', () => { + finish({ status: 'git-error' }); + }); + child.on('close', (code: number | null) => { + finish( + closeResult( + code, + aborted, + stop, + timedOut, + pending, + decoder, + onName, + stderr, + ), + ); + }); +} + +/** Derive the terminal result from a close event. */ +function closeResult( + code: number | null, + aborted: boolean, + stop: PhaseStop, + timedOut: boolean, + pending: string, + decoder: StringDecoder, + onName: (name: string) => 'continue' | 'stop', + stderr: string, +): GitPhaseResult { + if (aborted) { + return { status: 'aborted' }; + } + if (stop === 'overflow') { + return { + status: 'output-overflow', + error: `git working-set listing exceeded its ${GIT_MAX_BUFFER}-byte output limit`, + }; + } + if (stop === 'capped') { + return { status: 'truncated' }; + } + if (timedOut) { + return { + status: 'git-error', + error: `git timed out after ${GIT_TIMEOUT_MS}ms`, + }; + } + if (code !== 0) { + return { + status: 'git-error', + error: stderr.trim() || `git exited with status ${String(code)}`, + }; + } + // Flush any trailing name emitted without a final NUL separator. + const tail = pending + decoder.end(); + if (tail.length > 0 && onName(tail) === 'stop') { + return { status: 'truncated' }; + } + return { status: 'ok' }; +} diff --git a/packages/tools/src/tools/ast-edit/types.ts b/packages/tools/src/tools/ast-edit/types.ts index 3c4d1742dc..b8d41f9c24 100644 --- a/packages/tools/src/tools/ast-edit/types.ts +++ b/packages/tools/src/tools/ast-edit/types.ts @@ -120,8 +120,84 @@ export interface CrossFileContext { } export interface ConnectedFile { - filePath: string; - declarations: EnhancedDeclaration[]; + readonly filePath: string; + readonly declarations: readonly EnhancedDeclaration[]; +} + +// ===== Working-Set Acquisition Interfaces ===== +export type WorkingSetPartialReason = + | 'file-count' + | 'source-bytes' + | 'declarations' + | 'cancelled' + | 'git-error' + | 'discovery-limit' + | 'skipped-files'; + +/** + * Bounded accounting for one working-set acquisition. Counts only — retained + * declarations live in {@link ConnectedFile} entries and are never duplicated + * here, so a partial result is described without a second copy of its data. + * + * Discriminated on {@link WorkingSetAcquisitionStatusBase.complete}: a + * complete acquisition may not carry a partial reason, and an incomplete one + * must carry exactly one. + */ +export interface WorkingSetAcquisitionStatusBase { + /** + * True only when the context is complete: every eligible working-set file + * was observed, retained, and none was omitted. Any skip, early policy + * stop, cancellation, Git failure, or discovery truncation makes this + * false even when {@link traversalComplete} is true. + */ + readonly complete: boolean; + /** + * True when the traversal itself ran to exhaustion: discovery completed + * and no policy stop ended acquisition early. Distinct from context + * completeness — a fully-traversed run with skips is traversal-complete + * but partial. + */ + readonly traversalComplete: boolean; + /** + * True when Git discovery stopped at its finite candidate cap: at least + * {@link eligibleFiles} eligible files existed and more were never counted. + */ + readonly discoveryTruncated: boolean; + readonly retainedFiles: number; + readonly retainedDeclarations: number; + readonly retainedSourceBytes: number; + /** Eligible working-set candidates observed (bounded by the discovery cap). */ + readonly eligibleFiles: number; + /** Files skipped because reading failed (missing permissions, not a file). */ + readonly skippedFiles: number; + /** Files skipped because one alone exceeds the aggregate byte budget. */ + readonly oversizedFiles: number; + /** Files that Git reported but that no longer exist when acquisition ran. */ + readonly missingFiles: number; +} + +/** A complete acquisition: nothing was omitted, so no partial reason exists. */ +export interface CompleteWorkingSetAcquisitionStatus + extends WorkingSetAcquisitionStatusBase { + readonly complete: true; + readonly partialReason?: undefined; +} + +/** An incomplete acquisition: exactly one terminating reason is required. */ +export interface PartialWorkingSetAcquisitionStatus + extends WorkingSetAcquisitionStatusBase { + readonly complete: false; + readonly partialReason: WorkingSetPartialReason; +} + +export type WorkingSetAcquisitionStatus = + | CompleteWorkingSetAcquisitionStatus + | PartialWorkingSetAcquisitionStatus; + +/** Bounded working-set acquisition result: retained files plus accounting. */ +export interface WorkingSetAcquisition { + readonly files: readonly ConnectedFile[]; + readonly status: WorkingSetAcquisitionStatus; } export interface EnhancedDeclaration extends Declaration { @@ -140,7 +216,8 @@ export interface EnhancedASTContext extends ASTContext { relatedFiles?: string[]; relatedSymbols?: SymbolReference[]; crossFileContext?: CrossFileContext; - connectedFiles?: ConnectedFile[]; + connectedFiles?: readonly ConnectedFile[]; + workingSetStatus?: WorkingSetAcquisitionStatus; } // ===== Simplified Parameter Interfaces ===== diff --git a/packages/tools/src/tools/ast-edit/workspace-context-provider.ts b/packages/tools/src/tools/ast-edit/workspace-context-provider.ts index 9ff63b383b..d81bd01a31 100644 --- a/packages/tools/src/tools/ast-edit/workspace-context-provider.ts +++ b/packages/tools/src/tools/ast-edit/workspace-context-provider.ts @@ -7,52 +7,612 @@ */ import { promises as fsPromises } from 'fs'; -import type { ConnectedFile } from './types.js'; +import { isNodeError } from '../../utils/errors.js'; +import type { + ConnectedFile, + EnhancedDeclaration, + WorkingSetAcquisition, + WorkingSetAcquisitionStatus, + WorkingSetPartialReason, +} from './types.js'; import type { ASTQueryExtractor } from './ast-query-extractor.js'; -import type { RepositoryContextProvider } from './repository-context-provider.js'; +import type { + RepositoryContextProvider, + WorkingSetDiscoveryOutcome, +} from './repository-context-provider.js'; +import { createDefaultByteBudget } from '../../acquisition/byteBudget.js'; + +/** Finite policy: maximum working-set files retained per acquisition. */ +export const MAX_WORKING_SET_FILES = 50; + +/** Finite policy: maximum declarations retained across the working set. */ +export const MAX_WORKING_SET_DECLARATIONS = 500; + +/** Finite policy: working-set files acquired concurrently per chunk. */ +export const WORKING_SET_ACQUISITION_CONCURRENCY = 4; /** - * Enrich context with declarations from working-set files. - * Gets the current working set from git (unstaged/staged/recent commits), - * reads each file, and extracts declarations for a skeleton view. + * Bounded-read growth sentinel: a working-set file may grow between its + * planned stat and its read, so reads materialize at most the admitted + * allowance plus this window. Filling the window means the content can never + * be confirmed complete within the allowance, and the file is skipped. + */ +const READ_SENTINEL_BYTES = 4096; + +/** One planned working-set candidate: admissible, or a skip outcome. */ +type PlannedCandidate = + | { readonly kind: 'admit'; readonly path: string; readonly size: number } + | { readonly kind: 'missing'; readonly path: string } + | { readonly kind: 'unreadable'; readonly path: string } + | { readonly kind: 'oversized'; readonly path: string }; + +/** One settled acquisition item: a parsed file, or a skip outcome. */ +type AcquiredItem = + | { + readonly kind: 'file'; + readonly sourceBytes: number; + readonly declarations: readonly EnhancedDeclaration[]; + } + | { readonly kind: 'skipped'; readonly reason: 'unreadable' | 'oversized' }; + +/** One admitted chunk item with its pre-assigned byte allowance. */ +interface AdmittedCandidate { + readonly path: string; + readonly allowance: number; +} + +/** + * Immutable accumulator for one bounded acquisition run. Every transition + * returns a new frozen state, so the retained arrays and the counters used + * to derive the final status can never diverge. + */ +interface WorkingSetRunState { + readonly retained: readonly ConnectedFile[]; + readonly retainedFiles: number; + readonly retainedDeclarations: number; + readonly retainedSourceBytes: number; + readonly skippedFiles: number; + readonly oversizedFiles: number; + readonly missingFiles: number; +} + +const EMPTY_RUN_STATE: WorkingSetRunState = Object.freeze({ + retained: Object.freeze([]), + retainedFiles: 0, + retainedDeclarations: 0, + retainedSourceBytes: 0, + skippedFiles: 0, + oversizedFiles: 0, + missingFiles: 0, +}); + +function retainFile( + state: WorkingSetRunState, + file: ConnectedFile, + sourceBytes: number, +): WorkingSetRunState { + return Object.freeze({ + ...state, + retained: Object.freeze([...state.retained, file]), + retainedFiles: state.retainedFiles + 1, + retainedDeclarations: state.retainedDeclarations + file.declarations.length, + retainedSourceBytes: state.retainedSourceBytes + sourceBytes, + }); +} + +function countSkip( + state: WorkingSetRunState, + kind: 'missing' | 'unreadable' | 'oversized', +): WorkingSetRunState { + if (kind === 'missing') { + return Object.freeze({ ...state, missingFiles: state.missingFiles + 1 }); + } + if (kind === 'unreadable') { + return Object.freeze({ ...state, skippedFiles: state.skippedFiles + 1 }); + } + return Object.freeze({ ...state, oversizedFiles: state.oversizedFiles + 1 }); +} + +/** True when the optional signal is present and already aborted. */ +function signalAborted(signal: AbortSignal | undefined): boolean { + return signal?.aborted ?? false; +} + +/** + * Plan one discovered candidate with a bounded stat. A Git-reported path + * that vanished is missing; one that is no longer a regular file is + * unreadable; one whose size alone exceeds the aggregate budget is oversized. + * Never rejects: planning problems become skip outcomes so one bad candidate + * cannot fail the whole collection. + */ +async function planCandidate( + candidate: string, + budgetBytes: number, +): Promise { + let stats; + try { + stats = await fsPromises.stat(candidate); + } catch (error: unknown) { + if (isNodeError(error) && error.code === 'ENOENT') { + return { kind: 'missing', path: candidate }; + } + return { kind: 'unreadable', path: candidate }; + } + if (!stats.isFile()) { + return { kind: 'unreadable', path: candidate }; + } + if (stats.size > budgetBytes) { + return { kind: 'oversized', path: candidate }; + } + return { kind: 'admit', path: candidate, size: stats.size }; +} + +/** + * Read the bounded window into `buffer`, returning the exact raw byte count. + * Throws on a real read failure (for example the path became a directory + * after its stat ran) so the caller can turn it into a single-file skip. + */ +async function readBoundedTotal( + handle: fsPromises.FileHandle, + buffer: Buffer, +): Promise { + let total = 0; + while (total < buffer.length) { + const { bytesRead } = await handle.read( + buffer, + total, + buffer.length - total, + total, + ); + if (bytesRead === 0) { + break; + } + total += bytesRead; + } + return total; +} + +/** + * Acquire one admitted candidate with a bounded read. * - * @param targetFilePath - The file currently being edited + * Materializes at most the admitted allowance plus the growth sentinel: if + * the read fills the buffer the content cannot be confirmed complete within + * the allowance and the file is skipped as oversized. The authoritative byte + * charge is the exact raw byte count the read returned — never a re-encoded + * UTF-8 length, which would mis-account invalid bytes. Never rejects: a + * candidate that fails at the open, read, or parse boundary is counted as a + * skip, and the handle is always closed. A close that fails is not + * suppressed either: the candidate is counted unreadable instead of + * retained, so the partial accounting surfaces the fault. + */ +async function acquireAdmittedItem( + candidate: AdmittedCandidate, + budgetBytes: number, + declarationLimit: number, + astExtractor: ASTQueryExtractor, +): Promise { + let handle; + try { + handle = await fsPromises.open(candidate.path, 'r'); + } catch { + return { kind: 'skipped', reason: 'unreadable' }; + } + let settled: AcquiredItem; + let closeFailed = false; + try { + settled = await readAdmittedCandidate( + handle, + candidate, + budgetBytes, + declarationLimit, + astExtractor, + ); + } finally { + try { + await handle.close(); + } catch { + closeFailed = true; + } + } + return closeFailed ? { kind: 'skipped', reason: 'unreadable' } : settled; +} + +/** + * Read and parse one admitted candidate through an open handle. Never + * rejects: read- and parse-boundary failures of one candidate become skip + * outcomes so they cannot fail the whole collection. + */ +async function readAdmittedCandidate( + handle: fsPromises.FileHandle, + candidate: AdmittedCandidate, + budgetBytes: number, + declarationLimit: number, + astExtractor: ASTQueryExtractor, +): Promise { + const buffer = Buffer.alloc(candidate.allowance + READ_SENTINEL_BYTES); + let total: number; + try { + total = await readBoundedTotal(handle, buffer); + } catch { + // One bad candidate must never reject the whole collection. + return { kind: 'skipped', reason: 'unreadable' }; + } + if (total === buffer.length) { + return { kind: 'skipped', reason: 'oversized' }; + } + if (total > budgetBytes) { + return { kind: 'skipped', reason: 'oversized' }; + } + const content = buffer.toString('utf8', 0, total); + let declarations; + try { + declarations = await astExtractor.extractDeclarationsBounded( + candidate.path, + content, + declarationLimit, + ); + } catch { + // The bounded parser is an external boundary for arbitrary file + // contents: a parse fault skips the file instead of failing the run. + return { kind: 'skipped', reason: 'unreadable' }; + } + return { + kind: 'file', + sourceBytes: Math.max(candidate.allowance, total), + declarations, + }; +} + +/** Derive the partial reason with deterministic precedence. */ +function derivePartialReason( + stop: WorkingSetPartialReason | null, + discovery: WorkingSetDiscoveryOutcome, + skippedTotal: number, +): WorkingSetPartialReason | undefined { + if (stop !== null) { + return stop; + } + if (discovery === 'git-error') { + return 'git-error'; + } + if (discovery === 'truncated') { + return 'discovery-limit'; + } + return skippedTotal > 0 ? 'skipped-files' : undefined; +} + +/** Derive the final bounded status with deterministic reason precedence. */ +function buildStatus( + state: WorkingSetRunState, + stop: WorkingSetPartialReason | null, + discovery: WorkingSetDiscoveryOutcome, + eligibleFiles: number, +): WorkingSetAcquisitionStatus { + const skippedTotal = + state.skippedFiles + state.oversizedFiles + state.missingFiles; + const partialReason = derivePartialReason(stop, discovery, skippedTotal); + const traversalComplete = discovery === 'complete' && stop === null; + const base = { + traversalComplete, + discoveryTruncated: discovery === 'truncated', + retainedFiles: state.retainedFiles, + retainedDeclarations: state.retainedDeclarations, + retainedSourceBytes: state.retainedSourceBytes, + eligibleFiles, + skippedFiles: state.skippedFiles, + oversizedFiles: state.oversizedFiles, + missingFiles: state.missingFiles, + }; + if (partialReason === undefined) { + return Object.freeze({ ...base, complete: true }); + } + return Object.freeze({ ...base, complete: false, partialReason }); +} + +/** + * Enrich context with declarations from working-set files (Git unstaged, + * staged, and recent commits) under finite acquisition policies. + * + * Discovery observes at most MAX_WORKING_SET_FILES + 1 candidates (the finite + * count plus a one-over sentinel) through an abortable, exact-child-terminated + * Git run. Acquisition proceeds in bounded-concurrency chunks: candidates are + * planned with a stat, admitted sequentially against the remaining aggregate + * byte budget (so concurrent in-flight materialization is bounded by the + * budget, not the worker count), read with bounded buffers, and extracted + * with a bounded declaration acquisition. Policies are enforced before + * over-budget reads/parses start, in a fixed precedence: file-count, then + * source-bytes, then declarations; cancellation dominates when the signal is + * the binding cause. Any skipped, truncated, or failed candidate makes the + * context partial with explicit accounting; in-flight chunk items always + * settle before the returned promise resolves, so no invocation-owned work is + * left running. + * + * @param targetFilePath - The file currently being read (excluded from the set) * @param workspaceRoot - The workspace root directory * @param repoProvider - Repository context provider for git operations * @param astExtractor - AST query extractor for declaration extraction - * @returns Array of connected files with their declarations + * @param signal - Optional cancellation signal for owned acquisition + * @returns Bounded working-set acquisition: retained files and accounting */ export async function enrichWithWorkingSetContext( targetFilePath: string, workspaceRoot: string, repoProvider: RepositoryContextProvider, astExtractor: ASTQueryExtractor, -): Promise { - const connectedFiles: ConnectedFile[] = []; - - // Phase 2: Working Set Context (Git-based) - // Replace BM25 search with working set file declarations - const workingSetFiles = await repoProvider.getWorkingSetFiles(workspaceRoot); - - // Filter out current file - const otherFiles = workingSetFiles.filter((f) => f !== targetFilePath); - - const settled = await Promise.allSettled( - otherFiles.map(async (filePath) => { - const fileContent = await fsPromises.readFile(filePath, 'utf-8'); - const declarations = await astExtractor.extractDeclarations( - filePath, - fileContent, + signal?: AbortSignal, +): Promise { + if (signalAborted(signal)) { + return { + files: EMPTY_RUN_STATE.retained, + status: buildStatus(EMPTY_RUN_STATE, 'cancelled', 'complete', 0), + }; + } + + const discovery = await repoProvider.discoverWorkingSetFiles(workspaceRoot, { + maxCandidates: MAX_WORKING_SET_FILES + 1, + signal, + excludePath: targetFilePath, + }); + if (discovery.outcome === 'aborted') { + return { + files: EMPTY_RUN_STATE.retained, + status: buildStatus( + EMPTY_RUN_STATE, + 'cancelled', + 'aborted', + discovery.candidates.length, + ), + }; + } + // Git failure keeps whatever candidates earlier phases observed; the + // acquisition below stays bounded and the status stays partial. + const discoveryOutcome: WorkingSetDiscoveryOutcome = + discovery.outcome === 'no-working-set' ? 'complete' : discovery.outcome; + + const budgetBytes = createDefaultByteBudget().bytes; + const planned = await planCandidates( + [...discovery.candidates].sort(), + budgetBytes, + ); + const eligibleFiles = planned.length; + const { state, stop } = await acquirePlanned( + planned, + astExtractor, + budgetBytes, + signal, + ); + + return { + files: state.retained, + status: buildStatus(state, stop, discoveryOutcome, eligibleFiles), + }; +} + +/** + * Drive the bounded acquisition loop over planned candidates. Enforces the + * file-count, aggregate-source-byte, and retained-declaration policies with + * fixed precedence (file-count → source-bytes → declarations) before + * over-budget reads/parses start. Cancellation dominates when the signal is + * the binding cause. In-flight chunk items always settle before the returned + * promise resolves, so no invocation-owned work is left running. + */ +async function acquirePlanned( + planned: readonly PlannedCandidate[], + astExtractor: ASTQueryExtractor, + budgetBytes: number, + signal: AbortSignal | undefined, +): Promise<{ + state: WorkingSetRunState; + stop: WorkingSetPartialReason | null; +}> { + let state = EMPTY_RUN_STATE; + let stop: WorkingSetPartialReason | null = null; + let index = 0; + let shouldContinue = true; + + while (shouldContinue && index < planned.length && !signalAborted(signal)) { + if (state.retainedFiles >= MAX_WORKING_SET_FILES) { + stop = 'file-count'; + shouldContinue = false; + } else { + const chunkSize = boundedChunkSize( + state.retainedFiles, + planned.length, + index, ); - return { filePath, declarations }; - }), + const chunk = planned.slice(index, index + chunkSize); + state = processPlannedSkips(state, chunk); + const admission = admitChunk(state, chunk, budgetBytes); + state = admission.state; + const retention = await settleAndRetain( + admission, + state, + astExtractor, + budgetBytes, + signal, + ); + state = retention.state; + index += chunk.length; + if (admission.stopAfter !== null) { + stop = admission.stopAfter; + shouldContinue = false; + } else if (retention.stop !== null) { + stop = retention.stop; + shouldContinue = false; + } + } + } + if (stop === null && signalAborted(signal)) { + stop = 'cancelled'; + } + return { state, stop }; +} + +/** Compute a chunk size that never exceeds concurrency, remaining budget, or supply. */ +function boundedChunkSize( + retainedFiles: number, + totalPlanned: number, + index: number, +): number { + return Math.min( + WORKING_SET_ACQUISITION_CONCURRENCY, + MAX_WORKING_SET_FILES - retainedFiles, + totalPlanned - index, + ); +} + +/** Acquire, retain, and return the settled state plus any retention stop. */ +async function settleAndRetain( + admission: AdmissionResult, + state: WorkingSetRunState, + astExtractor: ASTQueryExtractor, + budgetBytes: number, + signal: AbortSignal | undefined, +): Promise<{ + state: WorkingSetRunState; + stop: WorkingSetPartialReason | null; +}> { + if (admission.admitted.length === 0 || signalAborted(signal)) { + return { state, stop: null }; + } + const declarationLimit = + MAX_WORKING_SET_DECLARATIONS - state.retainedDeclarations + 1; + const items = await Promise.all( + admission.admitted.map((candidate) => + acquireAdmittedItem( + candidate, + budgetBytes, + declarationLimit, + astExtractor, + ), + ), ); + const retention = retainItems(state, admission.admitted, items, budgetBytes); + return { state: retention.state, stop: retention.stop }; +} + +/** + * Stat discovered candidates in policy-sized chunks, preserving the input + * (sorted) order. Planning obeys the same finite concurrency policy as + * acquisition instead of starting one stat promise per candidate. + */ +async function planCandidates( + candidates: readonly string[], + budgetBytes: number, +): Promise { + const planned: PlannedCandidate[] = []; + for ( + let index = 0; + index < candidates.length; + index += WORKING_SET_ACQUISITION_CONCURRENCY + ) { + const chunk = candidates.slice( + index, + index + WORKING_SET_ACQUISITION_CONCURRENCY, + ); + planned.push( + ...(await Promise.all( + chunk.map((candidate) => planCandidate(candidate, budgetBytes)), + )), + ); + } + return planned; +} + +/** Count every skip already known from planning (independent of admission). */ +function processPlannedSkips( + state: WorkingSetRunState, + chunk: readonly PlannedCandidate[], +): WorkingSetRunState { + let next = state; + for (const candidate of chunk) { + if (candidate.kind !== 'admit') { + next = countSkip(next, candidate.kind); + } + } + return next; +} - for (const item of settled) { - if (item.status === 'fulfilled') { - connectedFiles.push(item.value); +interface AdmissionResult { + readonly state: WorkingSetRunState; + readonly admitted: readonly AdmittedCandidate[]; + /** + * Present when a candidate could not be admitted against the remaining + * aggregate byte budget: acquisition stops after the already-admitted + * candidates settle, and the non-fitting candidate (and everything after + * it) is never read. + */ + readonly stopAfter: WorkingSetPartialReason | null; +} + +/** + * Sequentially admit chunk candidates against the remaining aggregate byte + * budget. Assigning allowances before any read starts bounds concurrent + * in-flight materialization to the remaining budget even with four workers. + */ +function admitChunk( + state: WorkingSetRunState, + chunk: readonly PlannedCandidate[], + budgetBytes: number, +): AdmissionResult { + const admitted: AdmittedCandidate[] = []; + let assigned = 0; + for (const candidate of chunk) { + if (candidate.kind !== 'admit') { + continue; } + if (state.retainedSourceBytes + assigned + candidate.size > budgetBytes) { + return { state, admitted, stopAfter: 'source-bytes' }; + } + admitted.push({ path: candidate.path, allowance: candidate.size }); + assigned += candidate.size; } + return { state, admitted, stopAfter: null }; +} + +interface RetentionResult { + readonly state: WorkingSetRunState; + readonly stop: WorkingSetPartialReason | null; +} - return connectedFiles; +/** + * Retain settled items sequentially in deterministic order. Byte charges use + * the authoritative actual byte length; a declaration one-over sentinel drops + * its file and stops acquisition. + */ +function retainItems( + state: WorkingSetRunState, + admitted: readonly AdmittedCandidate[], + items: readonly AcquiredItem[], + budgetBytes: number, +): RetentionResult { + let next = state; + for (let position = 0; position < items.length; position++) { + const item = items[position]; + if (item.kind === 'skipped') { + next = countSkip(next, item.reason); + continue; + } + const overBudget = + next.retainedSourceBytes + item.sourceBytes > budgetBytes; + if (overBudget) { + return { state: next, stop: 'source-bytes' }; + } + const declarations = Object.freeze([...item.declarations]); + const overDeclarations = + declarations.length > + MAX_WORKING_SET_DECLARATIONS - next.retainedDeclarations; + if (overDeclarations) { + return { state: next, stop: 'declarations' }; + } + next = retainFile( + next, + Object.freeze({ + filePath: admitted[position].path, + declarations, + }), + item.sourceBytes, + ); + } + return { state: next, stop: null }; } diff --git a/project-plans/issue3232/plan.md b/project-plans/issue3232/plan.md new file mode 100644 index 0000000000..e3c989cdb9 --- /dev/null +++ b/project-plans/issue3232/plan.md @@ -0,0 +1,179 @@ +# Plan: Bound `ast_read_file` Native Memory and Working-Set Acquisition (Issue #3232) + +Plan ID: PLAN-20260814-ISSUE3232 +Generated: 2026-08-14 +Issue: #3232 + +## Problem statement + +A real `ast_read_file` invocation on a small TypeScript target can start up to five concurrent whole-workspace `@ast-grep/napi.findInFiles` traversals. The JavaScript timeout does not cancel those native producers, and the callback adapter can let the tool return before native traversal completes. A bounded Darwin reproduction reached approximately 7.82 GB RSS from one invocation; overlapping calls can exhaust a 32 GB workstation. + +For `ast_read_file`, the resulting `repositoryContext`, `relatedFiles`, and `relatedSymbols` are absent from both `llmContent` and `returnDisplay`. The dominant native traversal is therefore dead work on this path. The useful Git working-set context is rendered to the LLM, but its current all-at-once file acquisition is also unbounded. + +## Preflight findings + +1. `ASTReadFileToolInvocation.execute()` passes the complete target to `ASTContextCollector.collectEnhancedContext()` and does not pass its `AbortSignal`. +2. `ASTEditToolInvocation.executePreview()` uses the same collector and renders repository and related-symbol data; that behavior is outside the removal and must remain intact. +3. `collectEnhancedContext()` already has caller-specific options for working-set collection, so caller-specific repository collection belongs at the same boundary rather than in output formatting. +4. `enrichWithWorkingSetContext()` currently reads every working-set file concurrently with `Promise.allSettled`, then retains every successful file and every extracted declaration. +5. The shared acquisition package provides a validated 4 MiB default byte budget and a finite 64 MiB hard maximum. The working-set implementation should reuse this established primitive instead of inventing another byte-budget type. +6. Existing tests use real temporary files, real Git repositories, and the real `ASTReadFileTool`; these patterns satisfy the behavioral-test requirement without mocking the component under test. +7. The dedicated Windows memory workflow currently runs only the memory-diagnostics test directory and is not triggered by AST tool changes. + +## Requirements and behavior + +### REQ-3232-1: Eliminate unobservable repository analysis from `ast_read_file` + +**Full text:** `ast_read_file` must not collect repository metadata, build a workspace symbol index, enumerate a workspace for related symbols, or launch native related-symbol searches because none of those results are present in its model-facing or display output. `ast_edit` preview behavior that consumes repository and related-symbol context must remain compatible. + +- GIVEN a supported source target containing several declarations and a workspace containing related symbols +- WHEN `ast_read_file` executes +- THEN it returns the same selected display content, local declaration/snippet context, metadata, and bounded working-set context without starting repository relationship analysis +- AND no producer started by the read invocation continues after the invocation resolves +- GIVEN the same target is used for `ast_edit` preview +- WHEN preview context is built +- THEN its existing repository/related-symbol behavior remains available + +### REQ-3232-2: Bound working-set acquisition during collection + +**Full text:** Working-set context must enforce finite file-count, aggregate-source-byte, retained-declaration, and concurrency policies before unbounded data is materialized. Exact-limit input is complete; the first over-limit condition produces bounded partial context and accurate reason/accounting metadata. Per-file size is checked before reading, aggregate source bytes are charged across the entire collection, and retained results never exceed any policy. + +- GIVEN a normal Git working set within every policy +- WHEN `ast_read_file` executes +- THEN the current `WORKING SET CONTEXT` content remains compatible and is reported complete +- GIVEN exact file, byte, or declaration limits +- WHEN collection completes without observing additional eligible data +- THEN it is complete rather than falsely truncated +- GIVEN one-over or far-over input +- WHEN collection reaches a policy +- THEN it retains only bounded context, marks it partial, and identifies the limiting policy to the LLM +- AND it does not read or parse files after the authoritative stop +- GIVEN unreadable, missing, unsupported, or oversized files +- WHEN they are encountered +- THEN the collection remains bounded and reports accurate skipped/partial accounting without failing the target-file read + +### REQ-3232-3: Honor cancellation throughout LLxprt-owned acquisition + +**Full text:** The invocation signal must be threaded through enhanced-context and working-set acquisition. A pre-aborted signal performs no working-set reads or AST extraction. A signal raised during collection stops scheduling new reads and returns control without fire-and-forget work. The tool must not treat a cancelled partial collection as complete. + +- GIVEN an already-aborted signal +- WHEN `ast_read_file` starts +- THEN no working-set file is read or parsed +- GIVEN cancellation during bounded collection +- WHEN an in-flight item finishes +- THEN no additional item is scheduled and the collection is identified as partial due to cancellation +- AND all promises owned by the invocation are settled before it resolves + +### REQ-3232-4: Bound model-facing working-set rendering + +**Full text:** Model-facing connected-file rendering must consume the bounded retained collection and must not create an unbounded declaration aggregate. A partial marker and concise accounting must fit within the same finite retained-item/output policy. Normal bounded output wording remains compatible. + +- GIVEN complete context +- WHEN LLM content is built +- THEN existing `WORKING SET CONTEXT` file/declaration information remains present +- GIVEN partial context +- WHEN LLM content is built +- THEN the model sees an explicit partial marker and reason +- AND no omitted file/declaration is accidentally rendered or retained in result metadata + +### REQ-3232-5: Cross-platform memory regression evidence + +**Full text:** A safe child-process behavioral test must invoke the real `ASTReadFileTool` against a generated fixture that would trigger the former multi-symbol workspace fan-out, record peak RSS/private memory, and prove bounded completion without callbacks or acquisition continuing after the tool result. The fixture must be large enough to distinguish the old behavior while remaining safe for developer and CI machines. The same test must run under Bun on Windows and at least one non-Windows CI path. + +- GIVEN a generated workspace and a target with several prioritizable declarations +- WHEN the real tool executes with a small line limit +- THEN peak process memory stays below a conservative cross-platform ceiling +- AND the child exits normally after proving work has drained +- GIVEN repeated or parallel safe invocations +- WHEN they complete +- THEN memory remains bounded and no invocation leaves native work running behind it + +## Design constraints + +1. Add an explicit enhanced-context collection option for repository relationship context. Preserve the existing default for callers that consume it, and have `ast_read_file` opt out deliberately. +2. Do not change public tool parameters or remove useful read output. +3. Reuse the shared acquisition byte-budget primitive. Keep count and concurrency policies internal to the AST working-set module. +4. Discover and charge work incrementally. Do not enumerate/read everything and slice afterward. +5. Use bounded workers or an equivalent immutable scheduling design; do not start one promise per working-set file. +6. Preserve deterministic working-set order so bounded output is stable. +7. Make partiality/accounting explicit in the enhanced context through bounded summary metadata, not a second copy of retained declarations. +8. Do not add dependencies, suppressions, lint exceptions, ignore rules, threshold increases, or public settings. +9. Do not redesign `ast_edit` cross-file semantics in this issue. The unsafe `findInFiles` implementation remains reachable only from behavior that currently consumes its output; broader redesign requires separate intent. +10. Prefer fail-fast validation and authoritative producer cancellation over timeout wrappers that merely abandon waiting. + +## Test-first implementation sequence + +### Phase 1: Read-path repository opt-out + +1. Add a failing Bun behavioral test using the real collector/tool and a generated related-symbol fixture. Prove the read result retains local/display/working-set behavior and that the child process does not continue workspace activity after completion. +2. Add a companion regression proving `ast_edit` preview still receives its repository/related-symbol context. +3. Implement the minimal caller-specific repository-context option and pass the read invocation signal through the collector. +4. Run the focused AST-read/edit suites and preserve the RED-to-GREEN evidence. + +### Phase 2: Bounded working-set acquisition + +1. Add failing real-Git fixture tests for below-limit, exact-limit, one-over, far-over, per-file-size, aggregate-byte, declaration, deterministic ordering, and bounded concurrency behavior. +2. Add failing pre-abort and mid-collection cancellation tests that assert public returned context/metadata and observable filesystem effects rather than mock call counts. +3. Implement validated internal policies, stat-before-read, incremental aggregate charging, bounded scheduling, retained declaration limits, and partial accounting. +4. Add the partial marker to `ast_read_file` LLM rendering and verify normal complete wording remains compatible. +5. Run the focused tests after each RED/GREEN cycle. + +### Phase 3: Cross-platform memory regression + +1. Add a safe child-process Bun fixture that invokes the real tool and samples RSS/private memory. +2. Verify one, repeated, and parallel bounded invocations without generating a destructive workload. +3. Wire the focused memory regression into Windows CI and an existing non-Windows test path using plain `bun install` where installation is needed. +4. Keep workflow path filters narrow to the AST read implementation, its memory fixture, and the workflow itself. + +### Phase 4: Full verification and review remediation + +1. Run focused AST/tool tests, then the full repository verification commands. +2. Run DeepThinker review and Open Code Review, classify every finding against issue intent, and remediate grounded findings test-first. +3. Repeat no more than one additional review/remediation cycle if significant changes result. +4. Re-run full verification, smoke testing, Git diff review, and PR checks after remediation. + +## Behavioral test matrix + +| Area | Below limit | Exact limit | One over | Far over | Abort | Repeated/parallel | +| --- | --- | --- | --- | --- | --- | --- | +| Repository phase on `ast_read_file` | absent | N/A | N/A | N/A | no background producer | bounded | +| Working-set files | complete | complete | partial | partial | partial/cancelled | deterministic | +| Aggregate source bytes | complete | complete | partial | partial | partial/cancelled | bounded | +| Retained declarations | complete | complete | partial | partial | partial/cancelled | bounded | +| Model-facing rendering | compatible | compatible | explicit partial marker | explicit partial marker | explicit cancellation partiality | no duplicate growth | +| Process RSS/private memory | bounded | bounded | bounded | bounded | drains | bounded ceiling | + +## Scope boundaries + +- No removal of target-file AST declarations, relevant snippets, selected display content, display metadata, or useful bounded working-set context. +- No public schema or CLI setting changes. +- No change to the generic 20 MiB target-file gate. +- No line-range parser redesign in this issue; the dominant dead workspace traversal and used working-set acquisition are the accepted remediation. +- No general rewrite of `CrossFileRelationshipAnalyzer` or `ast_edit` repository relationships unless required to prevent `ast_read_file` from invoking them. +- No unrelated memory-history/provider/UI refactor. + +## Verification commands + +```bash +bun test packages/tools/src/tools/ast-edit/ +bun test packages/tools/src/tools/file-size-gate.bun.test.ts +npm run test +npm run lint +npm run typecheck +npm run format +npm run build +bun scripts/start.ts --profile-load stepfun-37 "write me a haiku and nothing else" +git diff --check +``` + +Before pushing, run detached Open Code Review with the required timeout floor and verify test files are included. After creating the PR, run `gh pr checks NUM --watch --interval 300`, evaluate every CodeRabbit thread against the source, remediate grounded findings, resolve addressed threads with an explanatory comment, and repeat verification until CI is green. + +## Acceptance criteria + +- The former repository-wide native fan-out is unreachable from `ast_read_file`. +- Current observable read functionality remains for ordinary bounded inputs. +- Working-set acquisition and rendering are finite by construction and explicitly partial when bounded. +- Cancellation schedules no new work and leaves no invocation-owned background work. +- Safe real-tool memory tests pass on Windows and non-Windows environments. +- All Bun tests and full project verification gates pass. +- No suppression, ignore, lint downgrade, complexity-threshold increase, package dependency, lockfile change, or `.llxprt` modification is introduced.