Files
gbrain/test/sources.test.ts
2934c53c1d fix(sources): validate --path is a git repo with committed content at registration (#2707) (#2975)
* fix(sources): validate --path is a git repo at registration time (#2707)

`sources add --path <dir>` accepted any existing non-git directory with
zero validation, deferring the failure to the first `gbrain sync` ("Not
inside a git repository: ..."). By the time that surfaces, the source
has been silently stale for however long nobody read the sync logs.

Add a registration-time check (git-remote.ts:isInsideGitRepo, mirroring
sync.ts's discoverGitRoot walk-up so subdir-of-git-repo sources still
pass) that rejects an existing-but-non-git --path directory with an
actionable error pointing at `git init && git add -A && git commit`.
Non-existent paths are unaffected (out of scope — different, pre-existing
failure mode) and `--force` opts out for callers who want to register
before git-init exists.

This is registration-time validation ONLY — it never auto-`git init`s
the directory, preserving the consent boundary #2967 established for
sync-time self-heal (a --path source is the user's own external
directory; gbrain must not mutate it without explicit ask).

Also documents the git requirement (docs/guides/multi-source-brains.md),
including the "files must be committed, not just present" gotcha and
that a stale/unreachable sync anchor already self-heals on plain
`gbrain sync` (verified manually against HEAD — no reset-anchor command
needed).

* fix(sources): require a committed HEAD + shell-quote remediation cmd (codex round 1)

Codex review round 1 on #2707 found two real gaps:

1. isInsideGitRepo alone accepts a `git init`ed-but-never-committed
   directory (rev-parse --show-toplevel succeeds with no HEAD), so
   registration would still pass a source that fails sync's own
   "No commits in repo ... Make at least one commit before syncing."
   Add hasGitCommits (git rev-parse HEAD) as a second required check.

2. The remediation command in the error message interpolated the raw
   path unquoted — spaces, $(), backticks, etc. would break or, worse,
   execute unintended shell syntax if pasted. POSIX single-quote it
   (mirrors src/commands/connect.ts:shellQuote; duplicated locally
   rather than imported, since commands/ depends on core/ not the
   reverse).

* fix(sources): require tracked content in HEAD, not just a resolvable HEAD (codex round 2)

Codex review round 2 P1: hasGitCommits (rev-parse HEAD) accepted a repo
with an empty commit (git commit --allow-empty) followed by untracked
files — HEAD resolves fine (to git's well-known empty-tree object), so
registration passed, but the first sync would "succeed" importing
nothing and then silently never notice the untracked files change. The
exact same gap applied to an untracked subdirectory of an otherwise-
real git repo (monorepo case).

Replace hasGitCommits with hasTrackedContent (`git ls-tree HEAD -- .`,
non-recursive — one entry is enough, no need to walk the whole
subtree). `-C path` + pathspec `.` scopes correctly to both a repo
toplevel and a subdirectory-of-a-repo source, and an empty tree lists
zero entries where a bare `rev-parse HEAD` would still succeed. Also
subsumes the "no commits at all" case hasGitCommits covered (ls-tree
on an unborn repo fails the same way), so this is one check instead
of two.

Updated the error copy and docs/guides/multi-source-brains.md to match
what's actually verified now.

* fix(sources): O(1)-output tree-emptiness probe, avoid maxBuffer overflow (codex round 3)

Codex review round 3 found the round-2 `git ls-tree HEAD -- .` listing
buffers the whole (non-recursive) tree — a real repo with ~17-20K
directly-tracked entries exceeds execFileSync's default 1 MiB
maxBuffer, throws ENOBUFS, and the catch-all incorrectly rejects a
perfectly valid registration.

Replace the listing with `git rev-parse --verify HEAD:./` (resolves
the tree object for `path` specifically, correct for both toplevel and
subdirectory sources same as before) compared against git's canonical
empty-tree SHA-1 (4b825dc6...) — a fixed ~40-byte read regardless of
how many entries the tree has, structurally immune to this class of
bug rather than just raising the threshold. Added a 300-file
regression test locking this in.

Declined a second round-3 finding (P1: reject a tree if ANY untracked
file exists anywhere under the path, not just when the tree is
entirely empty) — untracked files never being synced is standard,
existing git-source behavior throughout this codebase (identical for
--url managed clones), not a bug specific to this validation. Enforcing
zero-untracked-files at registration would reject ordinary repos with
gitignored build output, .DS_Store, editor swapfiles, etc. Out of
scope relative to what #2707 actually asks for (a directory with real,
committed content that will sync) and how every other git source in
this system already behaves.

* fix(sources): derive empty-tree OID per repo instead of hardcoding SHA-1 (codex round 4)

Codex review round 4 P2, confirmed by directly testing against a
`git init --object-format=sha256` repo: the hardcoded SHA-1 empty-tree
constant only matches SHA-1 repositories. An empty SHA-256 repo's real
empty-tree OID is a different (64-char) hash, so the SHA-1 comparison
silently mismatched and let an empty/untracked SHA-256 source through
— exactly the case this validation exists to catch.

Replace the constant with `emptyTreeOid()`: `git hash-object -t tree
--stdin < /dev/null` computed in the target repo's own context, so it
returns the correct empty-tree OID for whichever object format that
repo actually uses, without gbrain needing to know or care which one.
Added gated regression tests (git 2.29+ / --object-format=sha256,
test.skipIf on older git) for both the empty-repo-rejected and
real-content-registers-fine cases.

Converging here (4 review rounds; this is the last outstanding
finding from round 4, and round 4 raised only this one issue).

* fix(test): --force the incidental non-git second source in #1434 routing test (#2707)

CI caught a real regression from this PR's registration-time git
validation: test/sync-sole-non-default-routing.test.ts's "2+ non-default
sources" case registers a bare mkdtempSync temp dir (no git init) as a
second source purely to have 2 sources present — the directory's content
was never meant to be exercised, only its existence as a distinct
local_path. #2707's new validation correctly rejects that dir at
registration time, since nothing else in the test suite told it
otherwise.

--force is the right fix, not adding unnecessary git-init/commit
boilerplate to secondRepo: it documents that this specific registration
intentionally doesn't care about git-validity, matching what a real
caller opting into the legacy lenient behavior would do.

Verified: the specific test (3/3 pass), plus every other test file in
the repo using `sources add --path` (sources.test.ts, sources-ops.test.ts
already covered by the PR's own commits; repos-alias.test.ts,
sync-cost-gate.serial.test.ts — 11/11 pass, no similar fixture gap).
typecheck clean, verify 31/31.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2026-07-20 16:28:53 -07:00

311 lines
12 KiB
TypeScript

/**
* v0.18.0 Step 6 — sources CLI subcommand tests.
*
* Pure unit tests that exercise the subcommand dispatcher via a
* stub BrainEngine. No DB required — we just confirm the SQL
* shape, validation, and flag parsing.
*/
import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
import { mkdtempSync, mkdirSync, rmSync } from 'fs';
import { tmpdir } from 'os';
import { join } from 'path';
import { runSources } from '../src/commands/sources.ts';
import type { BrainEngine } from '../src/core/engine.ts';
// ── Stub engine that records queries ───────────────────────
interface RecordedCall {
sql: string;
params: unknown[];
}
function makeStub(rowsByPattern: Record<string, unknown[]> = {}): {
engine: BrainEngine;
calls: RecordedCall[];
configSet: Array<{ key: string; value: string }>;
} {
const calls: RecordedCall[] = [];
const configSet: Array<{ key: string; value: string }> = [];
const executeRaw = async (sql: string, params?: unknown[]) => {
calls.push({ sql, params: params ?? [] });
// Match by substring so tests are robust against whitespace.
for (const [pattern, rows] of Object.entries(rowsByPattern)) {
if (sql.includes(pattern)) return rows as never;
}
return [] as never;
};
const setConfig = async (key: string, value: string) => {
configSet.push({ key, value });
};
// Minimal BrainEngine stub — only the methods sources.ts touches.
const engine = {
kind: 'pglite' as const,
executeRaw,
setConfig,
// Unused methods throw if called accidentally during these tests.
getConfig: async () => null,
} as unknown as BrainEngine;
return { engine, calls, configSet };
}
// ── add ─────────────────────────────────────────────────────
// Intercept process.exit so unit tests under bun:test don't actually
// exit. Each test that might trigger process.exit() wraps its call in
// `withExitCapture`. We only return when the function under test returns
// or throws; process.exit() is turned into a recoverable throw.
async function withExitCapture(fn: () => Promise<void>): Promise<number | null> {
const origExit = process.exit;
let captured: number | null = null;
process.exit = ((code?: number) => {
captured = code ?? 0;
throw new Error('__process_exit__');
}) as never;
try {
await fn();
} catch (e) {
if (!(e instanceof Error) || !e.message.includes('__process_exit__')) throw e;
} finally {
process.exit = origExit;
}
return captured;
}
describe('sources add', () => {
test('rejects invalid ids', async () => {
const { engine } = makeStub();
const code = await withExitCapture(() => runSources(engine, ['add']));
expect(code).toBe(2);
});
test('rejects uppercase / invalid chars in id', async () => {
const { engine } = makeStub();
await expect(runSources(engine, ['add', 'BadId', '--path', '/tmp/x'])).rejects.toThrow(/Invalid source id/);
});
test('rejects id longer than 32 chars', async () => {
const { engine } = makeStub();
const long = 'a'.repeat(33);
await expect(runSources(engine, ['add', long, '--path', '/tmp/x'])).rejects.toThrow(/Invalid source id/);
});
test('inserts a valid source with defaults (federated unset → isolated)', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [{
id: 'gstack',
name: 'gstack',
local_path: '/tmp/gstack',
last_commit: null,
last_sync_at: null,
config: '{}',
created_at: new Date(),
}],
});
await runSources(engine, ['add', 'gstack', '--path', '/tmp/gstack']);
const insert = calls.find(c => c.sql.includes('INSERT INTO sources'));
expect(insert).toBeDefined();
expect(insert!.params[0]).toBe('gstack');
expect(insert!.params[1]).toBe('gstack'); // name defaults to id
expect(insert!.params[2]).toBe('/tmp/gstack');
expect(insert!.params[3]).toBe('{}'); // federated unset → empty config
});
test('--federated sets config.federated = true', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [{
id: 'wiki',
name: 'wiki',
local_path: '/tmp/wiki',
last_commit: null,
last_sync_at: null,
config: '{"federated":true}',
created_at: new Date(),
}],
});
await runSources(engine, ['add', 'wiki', '--path', '/tmp/wiki', '--federated']);
const insert = calls.find(c => c.sql.includes('INSERT INTO sources'));
expect(insert!.params[3]).toBe('{"federated":true}');
});
test('--no-federated sets config.federated = false (isolation opt-in)', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [{
id: 'yc-media',
name: 'yc-media',
local_path: '/tmp/yc',
last_commit: null,
last_sync_at: null,
config: '{"federated":false}',
created_at: new Date(),
}],
});
await runSources(engine, ['add', 'yc-media', '--path', '/tmp/yc', '--no-federated']);
const insert = calls.find(c => c.sql.includes('INSERT INTO sources'));
expect(insert!.params[3]).toBe('{"federated":false}');
});
test('rejects overlapping paths (per eng review finding 4.1)', async () => {
const { engine } = makeStub({
'SELECT id, local_path FROM sources WHERE local_path': [
{ id: 'gstack', local_path: '/tmp/gstack' },
],
});
// New source at /tmp/gstack/plans is inside existing gstack at /tmp/gstack.
await expect(runSources(engine, ['add', 'plans', '--path', '/tmp/gstack/plans']))
.rejects.toThrow(/overlaps with existing source "gstack"/);
});
});
// ── add — #2707 git-repo validation (CLI wiring) ───────────────
//
// Uses a REAL on-disk temp dir (unlike the fake-path tests above) so the
// core addSource git check actually runs; the stub engine still fakes the
// DB round-trip. Confirms --force parses through to opsAddSource.
describe('sources add — #2707 --force flag wiring', () => {
let plainDir: string;
beforeEach(() => {
plainDir = mkdtempSync(join(tmpdir(), 'gbrain-sources-cli-2707-'));
});
afterEach(() => {
rmSync(plainDir, { recursive: true, force: true });
});
test('rejects a real non-git --path directory by default', async () => {
const { engine } = makeStub();
await expect(runSources(engine, ['add', 'cli-plain', '--path', plainDir]))
.rejects.toThrow(/not a git repository/);
});
test('--force registers the same directory anyway', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [{
id: 'cli-forced',
name: 'cli-forced',
local_path: plainDir,
last_commit: null,
last_sync_at: null,
config: '{}',
created_at: new Date(),
}],
});
await runSources(engine, ['add', 'cli-forced', '--path', plainDir, '--force']);
const insert = calls.find(c => c.sql.includes('INSERT INTO sources'));
expect(insert).toBeDefined();
expect(insert!.params[2]).toBe(plainDir);
});
});
// ── list ────────────────────────────────────────────────────
describe('sources list', () => {
test('orders default source first, then alphabetical', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'default', name: 'default', local_path: null, last_commit: null, last_sync_at: null, config: '{"federated":true}', created_at: new Date() },
],
'COUNT(*)::int AS n FROM pages': [{ n: 0 }],
});
await runSources(engine, ['list']);
const select = calls.find(c => c.sql.includes('ORDER BY (id = \'default\') DESC'));
expect(select).toBeDefined();
});
test('counts only visible pages', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'default', name: 'default', local_path: null, last_commit: null, last_sync_at: null, config: '{"federated":true}', created_at: new Date() },
],
'COUNT(*)::int AS n FROM pages': [{ n: 1 }],
});
await runSources(engine, ['list']);
const count = calls.find(c => c.sql.includes('COUNT(*)::int AS n FROM pages'));
expect(count?.sql).toContain('deleted_at IS NULL');
});
});
// ── remove ──────────────────────────────────────────────────
describe('sources remove', () => {
test("refuses to remove the 'default' source", async () => {
const { engine } = makeStub();
const code = await withExitCapture(() => runSources(engine, ['remove', 'default', '--yes']));
expect(code).toBe(3);
});
test('refuses without --yes', async () => {
const { engine } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'gstack', name: 'gstack', local_path: '/tmp/g', last_commit: null, last_sync_at: null, config: '{}', created_at: new Date() },
],
'COUNT(*)::int AS n FROM pages': [{ n: 10 }],
});
const code = await withExitCapture(() => runSources(engine, ['remove', 'gstack']));
expect(code).toBe(5);
});
test('--dry-run reports but does not DELETE', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'gstack', name: 'gstack', local_path: '/tmp/g', last_commit: null, last_sync_at: null, config: '{}', created_at: new Date() },
],
'COUNT(*)::int AS n FROM pages': [{ n: 10 }],
});
await runSources(engine, ['remove', 'gstack', '--dry-run']);
const del = calls.find(c => c.sql.startsWith('DELETE FROM sources'));
expect(del).toBeUndefined();
});
});
// ── default ─────────────────────────────────────────────────
describe('sources default', () => {
test("stores id in config key 'sources.default'", async () => {
const { engine, configSet } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'gstack', name: 'gstack', local_path: null, last_commit: null, last_sync_at: null, config: '{}', created_at: new Date() },
],
});
await runSources(engine, ['default', 'gstack']);
expect(configSet).toEqual([{ key: 'sources.default', value: 'gstack' }]);
});
});
// ── federate / unfederate ──────────────────────────────────
describe('sources federate / unfederate', () => {
test('federate sets config.federated = true', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'gstack', name: 'gstack', local_path: null, last_commit: null, last_sync_at: null, config: '{}', created_at: new Date() },
],
});
await runSources(engine, ['federate', 'gstack']);
const upd = calls.find(c => c.sql.includes('UPDATE sources SET config'));
expect(upd).toBeDefined();
expect(JSON.parse(upd!.params[0] as string)).toEqual({ federated: true });
});
test('unfederate preserves other config keys', async () => {
const { engine, calls } = makeStub({
'SELECT id, name, local_path, last_commit, last_sync_at, config, created_at': [
{ id: 'gstack', name: 'gstack', local_path: null, last_commit: null, last_sync_at: null, config: '{"ttl_days":90,"federated":true}', created_at: new Date() },
],
});
await runSources(engine, ['unfederate', 'gstack']);
const upd = calls.find(c => c.sql.includes('UPDATE sources SET config'));
const parsed = JSON.parse(upd!.params[0] as string);
// Must preserve ttl_days while flipping federated.
expect(parsed.ttl_days).toBe(90);
expect(parsed.federated).toBe(false);
});
});