feat: final test coverage gaps for #54

[dean0x]

Summary

Closes #54 — adds tests for the 4 remaining non-trivial untested source files:

• validation.test.ts (13 tests): Security-critical path traversal prevention (symlink blocking, ../ detection), buffer size limits (1KB–1GB), timeout validation (1s–24h), NaN rejection
• output-repository.test.ts (8 tests): SQLite persistence with real DB, file fallback for large output (above threshold), append to existing/new, delete with file cleanup, ENOENT tolerance
• process-connector.test.ts (7 tests): stdout/stderr stream wiring, exit code 0 preservation (nullish coalescing), error capture with onExit(1), double-exit guard, missing streams safety
• git-state.test.ts (5 tests): Branch/SHA capture, non-git-repo → ok(null), empty repo handling, dirty file parsing from porcelain output, status failure resilience

Updates test:repositories and test:services scripts in package.json.

Coverage summary for #54

Tier	Files	Status
Tier 1	task-manager, event-driven-worker-pool, worktree-manager (removed)	✅ Complete
Tier 2	autoscaling-manager, resource-monitor, recovery-manager, container	✅ Complete
Tier 3	validation, output-repository, process-connector, git-state	✅ This PR
Skip	bootstrap.ts (DI wiring, integration-tested), process-spawner-adapter.ts (temporary shim), CLI commands (thin wrappers)	N/A

Test plan

• npm run test:repositories — 117 passed (includes new output-repository)
• npm run test:services — 141 passed (includes new process-connector)
• vitest run tests/unit/utils — 90 passed (includes new validation + git-state)
• npm run build — clean
• npx biome check src/ tests/ — no errors

[greptile-apps]

Confidence Score: 5/5

• Safe to merge — the only source change is a well-scoped, regression-tested bug fix; all other changes are new test files.
• The production code change is a single-line bug fix with a dedicated regression test. All new test files are well-structured, match established patterns in the repo, and include proper setup/teardown. The three style-level comments are non-blocking. No logic or syntax issues were found.
• No files require special attention.

Important Files Changed

Filename	Overview
src/utils/git-state.ts	Bug fix: removes .trim() before .split('\n') and replaces it with a .filter((line) => line.length > 0) guard, correctly preserving leading-space status prefixes (e.g., M src/foo.ts) that git status --porcelain emits for modified-but-unstaged files. The fix is minimal and correct.
tests/unit/implementations/output-repository.test.ts	New test file covering SQLite persistence, file-backed fallback for large output, append, delete, and ENOENT tolerance. Uses a real in-memory DB with proper FK setup and afterEach cleanup of the output/ directory. Minor gap: the "delete file-backed output" test doesn't assert the physical file was removed from disk.
tests/unit/services/process-connector.test.ts	New test file covering stdout/stderr wiring, exit-code preservation for 0 via nullish coalescing, error-to-stderr capture, double-exit guard, and null stream safety. Minor: the getOutput mock return value uses an unbranded 'test' string where TaskId is expected.
tests/unit/utils/git-state.test.ts	New test file covering branch/SHA capture, non-git-repo ok(null), empty repo, dirty file porcelain parsing (including the leading-space edge case from the accompanying bug fix), and status failure resilience. The mockExecFileSequence helper has no bounds guard, which could produce cryptic errors if a test triggers more execFile calls than expected responses.
tests/unit/utils/validation.test.ts	New test file with solid coverage of path traversal prevention (symlink and ../ vectors), buffer size and timeout boundary validation, and NaN rejection. Uses a real temp directory with proper beforeEach/afterEach cleanup including macOS symlink resolution via realpathSync.
package.json	Adds process-connector.test.ts to test:services, adds output-repository.test.ts to test:repositories, and — importantly — adds both checkpoint-repository.test.ts and output-repository.test.ts to the --exclude list in test:implementations, eliminating the double-run issues flagged in previous review threads.

Sequence Diagram

sequenceDiagram
    participant T as Test
    participant PC as ProcessConnector
    participant CP as ChildProcess (mock)
    participant OC as OutputCapture (mock)

    T->>PC: connect(proc, taskId, onExit)
    PC->>CP: proc.stdout.on('data', ...)
    PC->>CP: proc.stderr.on('data', ...)
    PC->>CP: proc.on('exit', ...)
    PC->>CP: proc.on('error', ...)

    Note over CP,OC: stdout data flow
    CP-->>PC: emit('data', buffer)
    PC->>OC: capture(taskId, 'stdout', text)

    Note over CP,OC: stderr data flow
    CP-->>PC: emit('data', buffer)
    PC->>OC: capture(taskId, 'stderr', text)

    Note over CP,PC: exit with double-guard
    CP-->>PC: emit('exit', code)
    PC->>PC: exitHandled = true
    PC-->>T: onExit(code ?? null)
    CP-->>PC: emit('exit', code) [2nd — ignored]
    PC->>PC: exitHandled already true — return

    Note over CP,OC: error path
    CP-->>PC: emit('error', err)
    PC->>OC: capture(taskId, 'stderr', 'Process error: ...')
    PC-->>T: onExit(1)

Loading

_{Last reviewed commit: d3118f6}

[greptile-apps]

Unbranded string used for TaskId in mock return value

taskId: 'test' is a plain string, but TaskId is a branded type (string & { readonly __brand: 'TaskId' }). This is technically a type error in the mock's return value object.

While this doesn't cause a test failure (since getOutput is never actually invoked in any of these tests, and vi.fn() returns a loosely-typed mock where mockReturnValue doesn't enforce the generic bound), it is inconsistent with how TaskId is used elsewhere in the same file — e.g., const taskId = 'task-1' as TaskId. For consistency and correctness, use the TaskId() constructor:

Suggested change

	it('should capture stdout data', () => {
	const capture = createMockOutputCapture();
	const logger = createTestLogger();
	const connector = new ProcessConnector(capture, logger);
	getOutput: vi.fn().mockReturnValue(ok({ taskId: TaskId('test'), stdout: [], stderr: [], totalSize: 0 })),

Note that TaskId would need to be imported at the top of the file alongside the other imports.

[greptile-apps]

File-backed delete test doesn't assert physical file removal

The "should delete file-backed output and clean up file" test verifies the DB entry is gone (via repo.get returning null), but the test name advertises "clean up file". The delete method has a specific code path to unlink the file from disk — that branch is untested here.

Consider asserting the file no longer exists on the filesystem after delete:

// After deleteResult assertion
const outputDir = path.join(process.cwd(), 'output');
const filePath = path.join(outputDir, `${taskId}.json`);
expect(fs.existsSync(filePath)).toBe(false);

This would require importing path at the top of the file (it's already imported as part of the cleanup in afterEach).

[greptile-apps]

mockExecFileSequence has no bounds guard on responses array

If a test accidentally triggers more execFile calls than there are entries in responses, responses[callIndex++] returns undefined. The subsequent 'error' in response check will then throw TypeError: Cannot use 'in' operator to search for 'error' in undefined, producing a cryptic test failure that obscures the root cause.

Adding a guard would make debugging much easier:

mock.mockImplementation((_cmd: unknown, _args: unknown, _opts: unknown, callback?: unknown) => {
  const cb = (callback ?? _opts) as ExecFileCallback;
  const response = responses[callIndex++];
  if (!response) {
    throw new Error(`mockExecFileSequence: unexpected call #${callIndex} — no response configured`);
  }
  // ...rest of implementation