Compare commits

..
Author SHA1 Message Date
Garry TanandClaude Fable 5 00460441b7 fix(markdown): MULTI_FRONTMATTER stops at the first non-frontmatter line
Review catch on #3163: the fence-pairing scan walked arbitrarily far past
body prose to find a second ---, and one colon-prefixed prose line
(`Note: …`, `TODO: …`) was enough to reject legitimate content at
import. Issue #2743's spec is leading-only detection that stops at the
first non-frontmatter character — the walk now breaks on the first line
that isn't YAML-shaped (key:, list item, comment, indented continuation,
or blank), so an hrule followed by prose is body content, never a
stacked block. Real stacked blocks (adjacent, blank-separated,
list-valued keys) still reject.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-22 10:59:15 -07:00
Garry TanandClaude Fable 5 fb5e7476f7 fix(put): reject empty content and stacked frontmatter instead of writing corrupt pages
Two verified backlog items on the put_page ingest path:

- #2822: `gbrain put` with empty non-TTY stdin silently wrote an empty
  page (0 chunks — invisible to search AND to embed --stale). parseOpArgs
  now leaves the stdin-fed param unset when stdin is empty/whitespace so
  the required-param check rejects with a message naming the empty pipe;
  a flag that overwrites a positional value (and vice versa) now warns to
  stderr instead of silently discarding it; and importFromContent throws
  on empty/whitespace content as the shared backstop, covering MCP
  put_page, capture, and every other caller.

- #2743: put_page silently accepted stacked frontmatter (the double-put
  corruption class: already-serialized markdown re-wrapped in fresh
  frontmatter). parseMarkdown(validate) grows a MULTI_FRONTMATTER finding
  (second ---…--- block with YAML-shaped lines right after the close
  fence; lone horizontal rules and the timeline sentinel stay untouched),
  lint maps it to frontmatter-multi, and importFromContent rejects it
  before any DB write.

Fixes #2822
Fixes #2743

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-21 14:58:03 -07:00
9 changed files with 242 additions and 94 deletions
+22 -1
View File
@@ -365,6 +365,12 @@ async function main() {
if (def.required && params[key] === undefined) {
if (queryHasAlt && key === 'query') continue;
const cliName = op.cliHints?.name || op.name;
// #2822: when the missing param is the op's stdin-fed one, the usage
// line alone is misleading (the positionals may all be present — the
// pipe was just empty). Name the real problem.
if (op.cliHints?.stdin === key) {
console.error(`Error: required "${key}" is missing — stdin was empty or not piped. Pipe content on stdin or pass --${key.replace(/_/g, '-')}.`);
}
const positional = op.cliHints?.positional || [];
const usage = positional.map(p => `<${p}>`).join(' ');
console.error(`Usage: gbrain ${cliName} ${usage}`);
@@ -761,6 +767,10 @@ export function parseOpArgs(op: Operation, args: string[]): Record<string, unkno
const params: Record<string, unknown> = {};
const positional = op.cliHints?.positional || [];
let posIdx = 0;
// #2822: track which params came from positionals so a later flag that
// silently discards one (`gbrain put CONTENT --slug foo` — CONTENT was
// parsed as the slug) gets a stderr warning instead of vanishing.
const positionallySet = new Set<string>();
for (let i = 0; i < args.length; i++) {
const arg = args[i];
@@ -778,13 +788,20 @@ export function parseOpArgs(op: Operation, args: string[]): Record<string, unkno
if (paramDef?.type === 'boolean') {
params[key] = true;
} else if (i + 1 < args.length) {
if (positionallySet.has(key) && params[key] !== args[i + 1]) {
console.error(`Warning: ${arg} overrides the positional <${key}> value ${JSON.stringify(params[key])}.`);
}
params[key] = args[++i];
if (paramDef?.type === 'number') params[key] = Number(params[key]);
}
} else if (posIdx < positional.length) {
const key = positional[posIdx++];
const paramDef = op.params[key];
if (params[key] !== undefined && params[key] !== (paramDef?.type === 'number' ? Number(arg) : arg)) {
console.error(`Warning: positional <${key}> overrides the earlier --${key.replace(/_/g, '-')} value ${JSON.stringify(params[key])}.`);
}
params[key] = paramDef?.type === 'number' ? Number(arg) : arg;
positionallySet.add(key);
}
}
@@ -796,7 +813,11 @@ export function parseOpArgs(op: Operation, args: string[]): Record<string, unkno
console.error(`Error: stdin content exceeds ${MAX_STDIN} bytes. Split into smaller inputs.`);
process.exit(1);
}
params[op.cliHints.stdin] = stdinContent;
// #2822: empty/whitespace-only stdin (cron with no input, broken pipe)
// stays UNSET so the required-param check rejects the call instead of
// silently writing an empty page (0 chunks, invisible to search and
// embed --stale).
if (stdinContent.trim().length > 0) params[op.cliHints.stdin] = stdinContent;
}
return params;
+1
View File
@@ -48,6 +48,7 @@ const FRONTMATTER_RULE_NAMES: Record<ParseValidationCode, string> = {
NESTED_QUOTES: 'frontmatter-nested-quotes',
NON_STRING_FIELD: 'frontmatter-non-string-field',
EMPTY_FRONTMATTER: 'frontmatter-empty',
MULTI_FRONTMATTER: 'frontmatter-multi',
};
/** Codes whose lint findings are fixable by `gbrain frontmatter validate --fix`. */
+2 -50
View File
@@ -3,11 +3,7 @@
*
* Usage:
* gbrain migrate --to supabase [--url <connection_string>]
* (--url is persisted to config.json, mode 0600, so the migrated brain
* works without env — #1271)
* gbrain migrate --to pglite [--path <db_path>]
* (an explicit --path destination is bootstrapped with its own
* <path>/.gbrain/config.json so GBRAIN_HOME=<path> just works — #1271)
* gbrain migrate --to <engine> --force (overwrite non-empty target)
*/
@@ -15,9 +11,9 @@ import { createEngine } from '../core/engine-factory.ts';
import { loadConfig, saveConfig, toEngineConfig, gbrainPath, effectiveEnvDatabaseUrl, type GBrainConfig } from '../core/config.ts';
import type { BrainEngine } from '../core/engine.ts';
import type { EngineConfig } from '../core/types.ts';
import { writeFileSync, readFileSync, existsSync, unlinkSync, mkdirSync, chmodSync } from 'fs';
import { writeFileSync, readFileSync, existsSync, unlinkSync } from 'fs';
import { createHash } from 'crypto';
import { resolve, join } from 'path';
import { resolve } from 'path';
import { createProgress } from '../core/progress.ts';
import { getCliOptions, cliOptsToProgressOptions } from '../core/cli-options.ts';
@@ -63,31 +59,6 @@ export interface MigrateManifest {
started_at: string;
}
/**
* #1271 Finding 1: make an explicit `--to pglite --path P` destination usable
* as a standalone brain. Writes `P/.gbrain/config.json` (mode 0600, plus a
* `*` .gitignore) so `GBRAIN_HOME=P` resolves without a manual `gbrain init`.
* Never clobbers an existing config at the destination. Returns the written
* config path, or null when skipped.
*/
export function bootstrapDestinationConfig(dbPath: string): string | null {
const abs = resolve(dbPath);
const dir = join(abs, '.gbrain');
const file = join(dir, 'config.json');
if (existsSync(file)) return null;
mkdirSync(dir, { recursive: true });
const cfg: GBrainConfig = { engine: 'pglite', database_path: abs };
writeFileSync(file, JSON.stringify(cfg, null, 2) + '\n', { mode: 0o600 });
try { chmodSync(file, 0o600); } catch { /* platform-specific */ }
// Same worktree-safety pattern as saveConfig()'s ensureGitignore, scoped
// to the destination home. Don't clobber a user-customized .gitignore.
const gitignore = join(dir, '.gitignore');
if (!existsSync(gitignore)) {
writeFileSync(gitignore, '*\n', { mode: 0o600 });
}
return file;
}
export function migrationTargetId(config: EngineConfig): string {
const locator = config.engine === 'postgres'
? config.database_url ?? ''
@@ -381,25 +352,6 @@ export async function runMigrateEngine(sourceEngine: BrainEngine, args: string[]
};
saveConfig(newConfig);
// #1271 Finding 2 (by design, but say it out loud): the connection string
// is persisted so the migrated brain works without env. Mode 0600.
if (opts.targetEngine === 'postgres' && opts.targetUrl) {
console.error('Note: the --url connection string (including credentials) is persisted to config.json (mode 0600).');
}
// #1271 Finding 1: an explicit --path destination doubles as a standalone
// GBRAIN_HOME. Best-effort — never fail a completed migration over it.
if (opts.targetEngine === 'pglite' && opts.targetPath) {
try {
const written = bootstrapDestinationConfig(opts.targetPath);
if (written) {
console.log(`Destination bootstrapped: ${written} (usable via GBRAIN_HOME=${resolve(opts.targetPath)})`);
}
} catch (e) {
console.warn(` WARN could not bootstrap destination config: ${e instanceof Error ? e.message : String(e)}`);
}
}
// Clean up
clearManifest();
+22 -1
View File
@@ -301,6 +301,17 @@ export async function importFromContent(
// silently fabricated a duplicate at (default, slug) — causing later
// bare-slug subqueries (getTags, deleteChunks, etc.) to crash with 21000.
const sourceId = opts.sourceId;
// #2822: reject empty/whitespace-only content before any work happens. An
// empty page writes 0 chunks — invisible to search AND to `embed --stale`
// (nothing to embed), so the mistake never surfaces. Empty content is
// always a caller bug (empty piped stdin, bad shell substitution). Thrown
// (not returned) so every wrapper site surfaces the message, matching the
// ContentSanityBlockError flow.
if (content.trim().length === 0) {
throw new Error(
`Content for "${slug}" is empty; refusing to write an empty page (0 chunks would be invisible to search and embed --stale).`,
);
}
// Reject oversized payloads before any parsing, chunking, or embedding happens.
// Uses Buffer.byteLength to count UTF-8 bytes the same way disk size would,
// so the network path behaves identically to the file path.
@@ -314,7 +325,17 @@ export async function importFromContent(
};
}
const parsed = parseMarkdown(content, slug + '.md', { activePack: opts.activePack });
const parsed = parseMarkdown(content, slug + '.md', { activePack: opts.activePack, validate: true });
// #2743: reject stacked frontmatter (the double-put corruption class —
// already-serialized markdown re-wrapped in fresh frontmatter). gray-matter
// parses only the first block; the second would land verbatim in the body
// and poison every subsequent round-trip. Only MULTI_FRONTMATTER rejects
// here — the other validation codes keep their lint-only semantics.
const multiFm = parsed.errors?.find(e => e.code === 'MULTI_FRONTMATTER');
if (multiFm) {
throw new Error(`MULTI_FRONTMATTER: ${multiFm.message} (slug "${slug}", line ${multiFm.line})`);
}
// v0.42 (#1699 trust boundary): strip gate-owned markers from UNTRUSTED
// input. parseMarkdown preserves every frontmatter key except type/title/
+46 -1
View File
@@ -11,7 +11,8 @@ export type ParseValidationCode =
| 'NULL_BYTES'
| 'NESTED_QUOTES'
| 'NON_STRING_FIELD'
| 'EMPTY_FRONTMATTER';
| 'EMPTY_FRONTMATTER'
| 'MULTI_FRONTMATTER';
export interface ParseValidationError {
code: ParseValidationCode;
@@ -331,6 +332,50 @@ function collectValidationErrors(
});
}
}
// 9. MULTI_FRONTMATTER (#2743) — a second ---…--- block right after the
// closing fence is stacked frontmatter (the double-put corruption class:
// already-serialized markdown re-wrapped in fresh frontmatter).
// gray-matter parses only the first block and silently leaves the second
// in the body. Heuristic: first non-empty line after the close is `---`,
// a later `---` closes it, EVERY line between is frontmatter-shaped
// (YAML `key:`, `- ` list item, `#` comment, indented continuation, or
// blank — the issue's "stop at the first non-frontmatter character"
// spec), and at least one is a `key:` line. A lone `---` stays a
// markdown horizontal rule, and an hrule followed by prose — even
// colon-prefixed prose like `Note: …` mixed with plain lines — is body
// content, not a stacked block.
let afterClose = closeLine + 1;
while (afterClose < lines.length && lines[afterClose].trim().length === 0) afterClose++;
if (afterClose < lines.length && lines[afterClose].trim() === '---') {
let secondClose = -1;
for (let i = afterClose + 1; i < lines.length; i++) {
const trimmed = lines[i].trim();
if (trimmed === '---') {
secondClose = i;
break;
}
const yamlShaped =
trimmed.length === 0 ||
/^[A-Za-z_][\w-]*\s*:/.test(trimmed) ||
trimmed.startsWith('- ') ||
trimmed === '-' ||
trimmed.startsWith('#') ||
/^\s/.test(lines[i]);
if (!yamlShaped) break; // first non-frontmatter line → body prose, not a stacked block
}
if (
secondClose > afterClose + 1 &&
lines.slice(afterClose + 1, secondClose).some(l => /^\s*[A-Za-z_][\w-]*\s*:/.test(l))
) {
errors.push({
code: 'MULTI_FRONTMATTER',
message:
'Stacked frontmatter: a second ---…--- block follows the frontmatter (double-put corruption); merge into a single frontmatter block',
line: afterClose + 1,
});
}
}
}
/**
+70 -1
View File
@@ -1,4 +1,8 @@
import { describe, expect, test } from 'bun:test';
import { afterEach, describe, expect, test } from 'bun:test';
import { spawnSync } from 'child_process';
import { join, resolve } from 'path';
import { mkdtempSync, rmSync } from 'fs';
import { tmpdir } from 'os';
import { parseOpArgs } from '../src/cli.ts';
import { operationsByName } from '../src/core/operations.ts';
@@ -20,5 +24,70 @@ describe('parseOpArgs', () => {
source_id: 'gstack-code-repo-0e4763c9',
});
});
describe('positional/flag overwrite warning (#2822)', () => {
const errors: string[] = [];
const origError = console.error;
const captureErrors = () => {
console.error = (...args: unknown[]) => errors.push(args.join(' '));
};
afterEach(() => {
console.error = origError;
errors.length = 0;
});
test('a flag that overwrites a positional value warns to stderr', () => {
captureErrors();
const params = parseOpArgs(operationsByName.query, ['positional text', '--query', 'flag text']);
expect(params.query).toBe('flag text');
expect(errors.some(e => e.includes('Warning') && e.includes('--query'))).toBe(true);
});
test('a positional that overwrites an earlier flag value warns to stderr', () => {
captureErrors();
const params = parseOpArgs(operationsByName.query, ['--query', 'flag text', 'positional text']);
expect(params.query).toBe('positional text');
expect(errors.some(e => e.includes('Warning') && e.includes('<query>'))).toBe(true);
});
test('no warning when flag and positional agree', () => {
captureErrors();
parseOpArgs(operationsByName.query, ['same', '--query', 'same']);
expect(errors).toEqual([]);
});
});
});
describe('gbrain put — empty non-TTY stdin rejects (#2822)', () => {
const REPO = resolve(import.meta.dir, '..');
const CLI = join(REPO, 'src', 'cli.ts');
const runPut = (input: string) => {
// Isolated HOME so a regression can never write into a real brain.
const home = mkdtempSync(join(tmpdir(), 'gbrain-put-empty-'));
try {
return spawnSync('bun', [CLI, 'put', 'inbox/empty-stdin-test'], {
stdio: ['pipe', 'pipe', 'pipe'],
input,
encoding: 'utf-8',
timeout: 60_000,
env: { ...process.env, HOME: home, GBRAIN_SKIP_STARTUP_HOOKS: '1' },
});
} finally {
rmSync(home, { recursive: true, force: true });
}
};
test('empty stdin exits 1 and names the missing content param', () => {
const res = runPut('');
expect(res.status).toBe(1);
expect(res.stderr).toContain('content');
expect(res.stderr).toContain('stdin');
}, 90_000);
test('whitespace-only stdin also exits 1', () => {
const res = runPut(' \n\t\n');
expect(res.status).toBe(1);
expect(res.stderr).toContain('stdin');
}, 90_000);
});
+29
View File
@@ -708,3 +708,32 @@ body unchanged
expect(shortCircuited).toBe(true);
});
});
describe('importFromContent — empty content guard (#2822)', () => {
test('empty string throws instead of writing an invisible 0-chunk page', async () => {
const engine = mockEngine();
await expect(importFromContent(engine, 'inbox/empty', '', { noEmbed: true })).rejects.toThrow(/empty/i);
expect((engine as any)._calls.find((c: any) => c.method === 'putPage')).toBeUndefined();
});
test('whitespace-only content throws', async () => {
const engine = mockEngine();
await expect(importFromContent(engine, 'inbox/ws', ' \n\t \n', { noEmbed: true })).rejects.toThrow(/empty/i);
});
});
describe('importFromContent — stacked frontmatter rejection (#2743)', () => {
test('double-put shaped content (two ---…--- blocks) throws MULTI_FRONTMATTER', async () => {
const engine = mockEngine();
const md = '---\ntitle: outer\n---\n\n---\ntitle: inner\ntype: concept\n---\n\nreal body';
await expect(importFromContent(engine, 'inbox/double', md, { noEmbed: true })).rejects.toThrow(/MULTI_FRONTMATTER/);
expect((engine as any)._calls.find((c: any) => c.method === 'putPage')).toBeUndefined();
});
test('normal content with horizontal rules in the body still imports', async () => {
const engine = mockEngine();
const md = '---\ntitle: ok\ntype: concept\n---\n\nprose before\n\n---\n\nprose after the rule';
const result = await importFromContent(engine, 'inbox/hrule', md, { noEmbed: true });
expect(result.status).toBe('imported');
});
});
+50
View File
@@ -256,6 +256,56 @@ body`;
});
});
describe('MULTI_FRONTMATTER (#2743)', () => {
test('stacked frontmatter immediately after the close fence', () => {
const md = `${fence}\ntitle: outer\n${fence}\n${fence}\ntitle: inner\ntype: concept\n${fence}\n\nbody`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).toContain('MULTI_FRONTMATTER');
});
test('stacked frontmatter with a blank line between blocks (serializeMarkdown shape)', () => {
const md = `${fence}\ntitle: outer\n${fence}\n\n${fence}\ntitle: inner\n${fence}\n\nbody`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).toContain('MULTI_FRONTMATTER');
});
test('horizontal rules in the body are NOT flagged', () => {
const md = `${fence}\ntitle: ok\n${fence}\n\nsome prose\n\n${fence}\n\nmore prose\n\n${fence}\n\nend`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).not.toContain('MULTI_FRONTMATTER');
});
test('hrule pair at body start without YAML-shaped lines is NOT flagged', () => {
const md = `${fence}\ntitle: ok\n${fence}\n\n${fence}\n\nplain prose between rules\n\n${fence}\n\nend`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).not.toContain('MULTI_FRONTMATTER');
});
test('timeline sentinel form is NOT flagged', () => {
const md = `${fence}\ntitle: ok\n${fence}\n\nbody text\n\n${fence}\n\n## Timeline\n- 2024-01-01: thing`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).not.toContain('MULTI_FRONTMATTER');
});
test('body hrule + colon-prefixed prose (`Note: …`) mixed with plain lines is NOT flagged', () => {
const md = `${fence}\ntitle: ok\ntype: concept\n${fence}\n\n${fence}\n\nNote: remember to follow up\n\nlots of plain prose here\n\n${fence}\n\nmore prose`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).not.toContain('MULTI_FRONTMATTER');
});
test('fence pairing stops at the first non-frontmatter line (no far-fence pairing across prose)', () => {
const md = `${fence}\ntitle: ok\n${fence}\n\n${fence}\n\n${'plain prose line\n'.repeat(40)}TODO: fix the widget\n${'more prose\n'.repeat(40)}${fence}\nend`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).not.toContain('MULTI_FRONTMATTER');
});
test('stacked block with list-valued keys is still flagged', () => {
const md = `${fence}\ntitle: outer\n${fence}\n\n${fence}\ntitle: inner\ntags:\n - a\n - b\n${fence}\n\nbody`;
const parsed = parseMarkdown(md, undefined, { validate: true });
expect(parsed.errors!.map(e => e.code)).toContain('MULTI_FRONTMATTER');
});
});
test('error.line is set for line-bearing errors', () => {
const md = `${fence}\ntype: concept\n${fence}\n# Heading inline\n\nbody\x00drop`;
const parsed = parseMarkdown(md, undefined, { validate: true });
@@ -1,40 +0,0 @@
import { describe, expect, test } from 'bun:test';
import { mkdtempSync, mkdirSync, readFileSync, statSync, writeFileSync } from 'fs';
import { tmpdir } from 'os';
import { join, resolve } from 'path';
import { bootstrapDestinationConfig } from '../src/commands/migrate-engine.ts';
import { loadConfigFileOnly } from '../src/core/config.ts';
import { withEnv } from './helpers/with-env.ts';
describe('migrate --to pglite destination bootstrap (#1271)', () => {
test('writes <path>/.gbrain/config.json so GBRAIN_HOME=<path> resolves a brain', async () => {
const dest = mkdtempSync(join(tmpdir(), 'gbrain-dest-'));
const written = bootstrapDestinationConfig(dest);
const file = join(dest, '.gbrain', 'config.json');
expect(written).toBe(file);
const cfg = JSON.parse(readFileSync(file, 'utf-8'));
expect(cfg.engine).toBe('pglite');
expect(cfg.database_path).toBe(resolve(dest));
expect(statSync(file).mode & 0o777).toBe(0o600);
// worktree safety: destination home is git-ignored like saveConfig()'s home
expect(readFileSync(join(dest, '.gbrain', '.gitignore'), 'utf-8')).toBe('*\n');
// The exact failure mode from #1271: config resolution under
// GBRAIN_HOME=<path> used to find nothing ("No brain configured").
await withEnv({ GBRAIN_HOME: dest }, () => {
const loaded = loadConfigFileOnly();
expect(loaded?.engine).toBe('pglite');
expect(loaded?.database_path).toBe(resolve(dest));
});
});
test('never clobbers an existing destination config', () => {
const dest = mkdtempSync(join(tmpdir(), 'gbrain-dest-'));
mkdirSync(join(dest, '.gbrain'), { recursive: true });
writeFileSync(join(dest, '.gbrain', 'config.json'), '{"engine":"postgres"}\n');
expect(bootstrapDestinationConfig(dest)).toBe(null);
expect(JSON.parse(readFileSync(join(dest, '.gbrain', 'config.json'), 'utf-8')).engine).toBe('postgres');
});
});