mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-30 11:22:34 +00:00
* 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>
211 lines
8.6 KiB
TypeScript
211 lines
8.6 KiB
TypeScript
/**
|
|
* v0.41.13 (#1434) — performSync without --source auto-routes to the only
|
|
* registered non-default source AND prints the nudge.
|
|
*
|
|
* Codex review of the original plan caught the load-bearing gap: adding the
|
|
* `sole_non_default` tier to source-resolver.ts is dead code unless
|
|
* `runSync` actually calls the resolver in the no-explicit-source case.
|
|
* Pre-fix at commands/sync.ts:1500-1505, the resolver was skipped when
|
|
* neither --source nor GBRAIN_SOURCE was set, leaving sourceId undefined.
|
|
*
|
|
* This test proves the wiring works end-to-end on PGLite: register one
|
|
* non-default source, call performSync with no source arg, assert pages
|
|
* land in that source (NOT 'default') AND the nudge appears on stderr.
|
|
*/
|
|
|
|
import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test';
|
|
import { mkdtempSync, writeFileSync, rmSync, mkdirSync } from 'fs';
|
|
import { execSync } from 'child_process';
|
|
import { tmpdir } from 'os';
|
|
import { join } from 'path';
|
|
import { PGLiteEngine } from '../src/core/pglite-engine.ts';
|
|
import { runSources } from '../src/commands/sources.ts';
|
|
import { resetPgliteState } from './helpers/reset-pglite.ts';
|
|
|
|
let engine: PGLiteEngine;
|
|
let repoPath: string;
|
|
|
|
async function pageCountBySource(): Promise<Record<string, number>> {
|
|
const rows = await engine.executeRaw<{ source_id: string; n: number }>(
|
|
`SELECT source_id, COUNT(*)::int AS n FROM pages GROUP BY source_id`,
|
|
);
|
|
const out: Record<string, number> = {};
|
|
for (const r of rows) out[r.source_id] = r.n;
|
|
return out;
|
|
}
|
|
|
|
describe('#1434 — runSync auto-routes to sole_non_default source', () => {
|
|
beforeAll(async () => {
|
|
engine = new PGLiteEngine();
|
|
await engine.connect({});
|
|
await engine.initSchema();
|
|
}, 60_000);
|
|
|
|
afterAll(async () => {
|
|
if (engine) await engine.disconnect();
|
|
}, 60_000);
|
|
|
|
beforeEach(async () => {
|
|
await resetPgliteState(engine);
|
|
repoPath = mkdtempSync(join(tmpdir(), 'gbrain-snd-routing-'));
|
|
execSync('git init', { cwd: repoPath, stdio: 'pipe' });
|
|
execSync('git config user.email "t@t.com"', { cwd: repoPath, stdio: 'pipe' });
|
|
execSync('git config user.name "T"', { cwd: repoPath, stdio: 'pipe' });
|
|
mkdirSync(join(repoPath, 'topics'), { recursive: true });
|
|
writeFileSync(join(repoPath, 'topics/foo.md'), [
|
|
'---',
|
|
'type: concept',
|
|
'title: Foo',
|
|
'---',
|
|
'',
|
|
'baseline.',
|
|
].join('\n'));
|
|
execSync('git add -A && git commit -m initial', { cwd: repoPath, stdio: 'pipe' });
|
|
});
|
|
|
|
afterEach(() => {
|
|
if (repoPath) rmSync(repoPath, { recursive: true, force: true });
|
|
});
|
|
|
|
test('sole non-default source: performSync without --source routes there', async () => {
|
|
// local_path is required for tier 5.5 to fire — point at the synthetic
|
|
// git repo so resolveSourceWithTier sees one non-default source with
|
|
// a local_path AND falls through brain_default (unset).
|
|
await runSources(engine, ['add', 'studiovault', '--path', repoPath, '--no-federated']);
|
|
const { runSync } = await import('../src/commands/sync.ts');
|
|
|
|
// Capture stderr to verify the nudge fires.
|
|
const origWrite = process.stderr.write.bind(process.stderr);
|
|
const captured: string[] = [];
|
|
(process.stderr as unknown as { write: typeof origWrite }).write = (
|
|
chunk: unknown,
|
|
...rest: unknown[]
|
|
): boolean => {
|
|
const s = typeof chunk === 'string' ? chunk : chunk instanceof Buffer ? chunk.toString() : String(chunk);
|
|
captured.push(s);
|
|
return origWrite(chunk as Parameters<typeof origWrite>[0], ...(rest as []));
|
|
};
|
|
|
|
try {
|
|
// runSync takes (engine, args). With no --source it relies on the
|
|
// resolver to pick sole_non_default. --full to bypass git-diff
|
|
// bookmarking. --no-embed since we have no embedding provider.
|
|
// --repo points at the synthetic vault.
|
|
// Note: runSync calls process.exit on some paths — guard accordingly.
|
|
const origExit = process.exit;
|
|
let exitCode: number | undefined;
|
|
process.exit = ((code?: number) => {
|
|
exitCode = code;
|
|
throw new Error('__exit__');
|
|
}) as typeof process.exit;
|
|
|
|
try {
|
|
await runSync(engine, ['--full', '--no-embed', '--repo', repoPath]);
|
|
} catch (e) {
|
|
if ((e as Error).message !== '__exit__') throw e;
|
|
} finally {
|
|
process.exit = origExit;
|
|
}
|
|
// Exit code 0 (success) or undefined (no exit called) both fine
|
|
expect(exitCode === undefined || exitCode === 0).toBe(true);
|
|
} finally {
|
|
(process.stderr as unknown as { write: typeof origWrite }).write = origWrite;
|
|
}
|
|
|
|
// IRON RULE: pages landed in studiovault, NOT in default.
|
|
const counts = await pageCountBySource();
|
|
expect(counts['studiovault']).toBeGreaterThan(0);
|
|
expect(counts['default'] ?? 0).toBe(0);
|
|
|
|
// The nudge appears on stderr.
|
|
const stderrText = captured.join('');
|
|
expect(stderrText).toContain("routing to source 'studiovault'");
|
|
expect(stderrText).toContain('sole non-default source registered');
|
|
}, 60_000);
|
|
|
|
test('explicit --source overrides auto-routing (no nudge)', async () => {
|
|
await runSources(engine, ['add', 'studiovault', '--path', repoPath, '--no-federated']);
|
|
const { runSync } = await import('../src/commands/sync.ts');
|
|
|
|
const origWrite = process.stderr.write.bind(process.stderr);
|
|
const captured: string[] = [];
|
|
(process.stderr as unknown as { write: typeof origWrite }).write = (
|
|
chunk: unknown,
|
|
...rest: unknown[]
|
|
): boolean => {
|
|
const s = typeof chunk === 'string' ? chunk : chunk instanceof Buffer ? chunk.toString() : String(chunk);
|
|
captured.push(s);
|
|
return origWrite(chunk as Parameters<typeof origWrite>[0], ...(rest as []));
|
|
};
|
|
|
|
try {
|
|
const origExit = process.exit;
|
|
process.exit = ((_code?: number) => { throw new Error('__exit__'); }) as typeof process.exit;
|
|
try {
|
|
await runSync(engine, ['--full', '--no-embed', '--repo', repoPath, '--source', 'default']);
|
|
} catch (e) {
|
|
if ((e as Error).message !== '__exit__') throw e;
|
|
} finally {
|
|
process.exit = origExit;
|
|
}
|
|
} finally {
|
|
(process.stderr as unknown as { write: typeof origWrite }).write = origWrite;
|
|
}
|
|
|
|
// No nudge — user picked explicitly.
|
|
const stderrText = captured.join('');
|
|
expect(stderrText).not.toContain('sole non-default source registered');
|
|
|
|
// Pages went to 'default' as requested.
|
|
const counts = await pageCountBySource();
|
|
expect(counts['default']).toBeGreaterThan(0);
|
|
}, 60_000);
|
|
|
|
test('2+ non-default sources: no auto-route, no nudge, falls through to default', async () => {
|
|
// Both need local_path to be counted by the sole_non_default helper.
|
|
// Pre-existing helper filters local_path IS NOT NULL.
|
|
// secondRepo is a bare temp dir (no git init) — its content is
|
|
// irrelevant to what this test verifies (that 2+ non-default sources
|
|
// disable auto-routing); --force skips #2707's registration-time git
|
|
// validation, which is orthogonal to this test's assertion.
|
|
const secondRepo = mkdtempSync(join(tmpdir(), 'gbrain-snd-routing-second-'));
|
|
await runSources(engine, ['add', 'studiovault', '--path', repoPath, '--no-federated']);
|
|
await runSources(engine, ['add', 'second-vault', '--path', secondRepo, '--no-federated', '--force']);
|
|
const { runSync } = await import('../src/commands/sync.ts');
|
|
|
|
const origWrite = process.stderr.write.bind(process.stderr);
|
|
const captured: string[] = [];
|
|
(process.stderr as unknown as { write: typeof origWrite }).write = (
|
|
chunk: unknown,
|
|
...rest: unknown[]
|
|
): boolean => {
|
|
const s = typeof chunk === 'string' ? chunk : chunk instanceof Buffer ? chunk.toString() : String(chunk);
|
|
captured.push(s);
|
|
return origWrite(chunk as Parameters<typeof origWrite>[0], ...(rest as []));
|
|
};
|
|
|
|
try {
|
|
const origExit = process.exit;
|
|
process.exit = ((_code?: number) => { throw new Error('__exit__'); }) as typeof process.exit;
|
|
try {
|
|
await runSync(engine, ['--full', '--no-embed', '--repo', repoPath]);
|
|
} catch (e) {
|
|
if ((e as Error).message !== '__exit__') throw e;
|
|
} finally {
|
|
process.exit = origExit;
|
|
}
|
|
} finally {
|
|
(process.stderr as unknown as { write: typeof origWrite }).write = origWrite;
|
|
}
|
|
|
|
const stderrText = captured.join('');
|
|
expect(stderrText).not.toContain('sole non-default source registered');
|
|
|
|
// Multi-source brains fall through to seed_default — same as pre-fix.
|
|
const counts = await pageCountBySource();
|
|
expect(counts['default'] ?? 0).toBeGreaterThan(0);
|
|
expect(counts['studiovault'] ?? 0).toBe(0);
|
|
expect(counts['second-vault'] ?? 0).toBe(0);
|
|
}, 60_000);
|
|
});
|