Bound directory scanning and document Git selection policy

Author: mchwang

README.md

@@ -44,6 +44,7 @@ Inputs such as `planText` and the ledger must come from the caller. The future `
 ## Current limits and safety
 
 - History must be linear and descend from the requested base. Merge histories are rejected with a rebase instruction; repositories using object alternates or symlinked object storage are rejected (storage inspection is limited to 100,000 entries). Reads are bounded to 500 commits, 32 MiB per Git response, 64 MiB of unique blob bytes across the history (callers may lower `maxBlobBytes`), and a 2-second budget per line diff; oversized work fails explicitly.
+- The caller selects and trusts the repository and its Git administrative directory. Normal Git discovery, linked-worktree gitfiles, and symlinked gitdirs are supported; object-storage links and alternates inside that selected gitdir are rejected. This adapter is not a filesystem-containment boundary for untrusted repository roots.
 - Object storage must remain unchanged during a read; these library checks do not isolate a concurrently hostile filesystem.
 - Ownership uses line diffs, not semantic inference. Within one replacement block, new lines inherit all affected owners conservatively. Function context comes from Git hunk headers, not an AST.
 - The importer requires an accurate base-file list. It rejects path traversal, Git metadata paths, and traversal through a listed file/symlink/submodule. Runtime symlink and write-scope enforcement belong to the future container/runner; plan validation alone is not a sandbox.

docs/implementation/build-step-1.md

@@ -55,6 +55,14 @@ Fixed unbounded accumulation of unique blobs across a history. The adapter check
 
 Fixed symlinked object storage bypassing the alternates check. Root, loose-directory, and pack-directory symlink regressions all reproduced the problem. The adapter now inspects the object store without following symlinks, rejects links at any depth, and bounds inspection to 100,000 entries before resolving commits. The caller must keep storage stable during reads; concurrent filesystem isolation belongs to the runner.
 
+## Review round 7
+
+Fixed eager directory listing before the inspection limit: use incremental directory reads with a one-entry buffer and close handles on every exit. Callers may lower the entry budget; the regression first failed and now rejects explicitly.
+
+Declined the YAML finding: the pinned yaml 2.9.1 implementation and types explicitly define `maxAliasCount: 0` as rejecting all aliases (`-1` disables limits). An otherwise valid plan with an alias fails with “Alias resolution is disabled”.
+
+Clarified the gitdir policy rather than rejecting normal linked worktrees. The caller selects and trusts the repository and its administrative directory; gitfiles and symlinked gitdirs are supported, while object-storage symlinks and alternates within that gitdir are rejected. Real-Git fixtures exercise both administrative layouts. Filesystem containment of an untrusted repository root belongs to the runner, not this read-only library.
+
 ## Remaining gates
 
 This is a working foundation, not a completed application or a claim that all implementation tasks are done. T18's pure validation/edit core is present; its agent adapters, import UI, and persistence are pending. Ledger storage, rebase mappings, and the read-only review screen remain next. The already-fixed GitHub check belongs to the later GitHub/runner integration.

git/history.ts

@@ -1,12 +1,14 @@
 import { execFileSync } from 'node:child_process';
-import { lstatSync, readdirSync } from 'node:fs';
+import { lstatSync, opendirSync } from 'node:fs';
 import { resolve as resolvePath, join } from 'node:path';
 import type { FileDelta, FileVersion, History } from '../core/linking.ts';
 
 /** Read-only Git adapter. Never follows working-tree symlinks or runs diff helpers. */
-export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', limits: { maxBlobBytes?: number } = {}): History {
+export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', limits: { maxBlobBytes?: number; maxObjectEntries?: number } = {}): History {
   const maxBlobBytes = limits.maxBlobBytes ?? 64 * 1024 * 1024;
   if (!Number.isSafeInteger(maxBlobBytes) || maxBlobBytes < 1 || maxBlobBytes > 64 * 1024 * 1024) throw new Error('Blob byte budget must be a positive integer no larger than 64 MiB.');
+  const maxObjectEntries = limits.maxObjectEntries ?? 100_000;
+  if (!Number.isSafeInteger(maxObjectEntries) || maxObjectEntries < 1 || maxObjectEntries > 100_000) throw new Error('Object entry budget must be a positive integer no larger than 100000.');
   let blobBytes = 0;
   // Inherited Git variables can redirect repository, index, config, and object lookup.
   const environment = Object.fromEntries(Object.entries(process.env).filter(([key]) => !/^GIT_/i.test(key)));
@@ -22,12 +24,17 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
   let inspected = 0;
   while (pending.length) {
     const path = pending.pop()!;
-    if (++inspected > 100_000) throw new Error('Object storage inspection exceeds 100000 entries.');
+    if (++inspected > maxObjectEntries) throw new Error('Object storage inspection exceeds its entry budget.');
     const stat = lstatSync(path);
     if (stat.isSymbolicLink()) throw new Error('Review repositories must not use symlinked object storage.');
-    if (stat.isDirectory()) for (const entry of readdirSync(path)) {
-      if (pending.length + inspected >= 100_000) throw new Error('Object storage inspection exceeds 100000 entries.');
-      pending.push(join(path, entry));
+    if (stat.isDirectory()) {
+      const directory = opendirSync(path, { bufferSize: 1 });
+      try {
+        for (let entry = directory.readSync(); entry; entry = directory.readSync()) {
+          if (pending.length + inspected >= maxObjectEntries) throw new Error('Object storage inspection exceeds its entry budget.');
+          pending.push(join(path, entry.name));
+        }
+      } finally { directory.closeSync(); }
     }
   }
   const alternates = join(objects, 'info/alternates');

test/history.test.ts

@@ -208,3 +208,20 @@ it.each(['root', 'loose', 'pack'])('rejects symlinked %s object storage', kind =
   renameSync(storage, borrowed); symlinkSync(borrowed, storage);
   expect(() => readHistory(f.dir, f.base)).toThrow(/symlinked object storage/i);
 });
+
+it('enforces a caller-lowered object inspection entry limit', () => {
+  const f = fixture();
+  expect(() => readHistory(f.dir, f.base, 'HEAD', { maxObjectEntries: 1 })).toThrow(/inspection exceeds/i);
+});
+it('supports a caller-selected linked worktree gitdir', () => {
+  const f = fixture(); f.write('a.txt', 'changed\n'); f.commit('P1');
+  const worktree = mkdtempSync(join(tmpdir(), 'codeboost-linked-')); dirs.push(worktree);
+  f.git('worktree', 'add', '--detach', worktree, 'HEAD');
+  expect(readHistory(worktree, f.base).final[0]!.after!.text).toBe('changed\n');
+});
+it('supports a caller-selected symlinked gitdir with regular object storage', () => {
+  const f = fixture(); f.write('a.txt', 'changed\n'); f.commit('P1');
+  const metadata = mkdtempSync(join(tmpdir(), 'codeboost-gitdir-')); dirs.push(metadata);
+  rmSync(metadata, { recursive: true }); renameSync(join(f.dir, '.git'), metadata); symlinkSync(metadata, join(f.dir, '.git'));
+  expect(readHistory(f.dir, f.base).final[0]!.after!.text).toBe('changed\n');
+});

test/plan.test.ts

@@ -1,3 +1,4 @@
+import { stringify } from 'yaml';
 import { readFileSync } from 'node:fs';
 import { describe, expect, it } from 'vitest';
 import { applySuggestion, assertEditReply, commandArgv, commandAllowed, importPlan, isRepoPath, validatePlan, type Plan, type PlanContext } from '../core/plan.ts';
@@ -106,3 +107,11 @@ it('rejects file/parent collisions declared within the same item', () => {
   const plan = basePlan(); plan.items[0]!.files = ['new', 'new/child'].map(path => ({ path, kind: 'add', renamed_from: null, change: 'Create' }));
   expect(validatePlan(plan, context).errors.some(e => e.code === 'path-parent')).toBe(true);
 });
+
+it('rejects an alias in an otherwise valid plan before schema validation', () => {
+  const source = stringify(basePlan());
+  expect(() => importPlan(source, 'yaml', context, 1)).not.toThrow();
+  const aliased = source.replace('summary: Change behavior.', 'summary: &summary Change behavior.').replace('title: Change', 'title: *summary');
+  expect(aliased).toContain('*summary');
+  expect(() => importPlan(aliased, 'yaml', context, 1)).toThrow(/Alias resolution is disabled/);
+});