mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-29 17:20:29 +00:00
fix(ingest): non-string frontmatter no longer aborts lint/sync; embed/hook/catalog papercuts
Parser coerces a non-string title to a string and falls back to inference for slug/type (never fabricating a "123" slug), with a lint NON_STRING_FIELD finding surfacing the malformed frontmatter; a defensive guard in content-sanity stops a non-string title from crashing the whole lint/sync run brain-wide. Plus: embed --catch-up no longer arms the overflowed 32-bit budget timer (and surfaces unembeddable chunks); the frontmatter pre-commit hook ships a correct .md/.mdx regex; and the skill catalog parses YAML block-scalar descriptions. Closes #1883, #1658, #1556, #1948, #1946, #1840, #1711.
This commit is contained in:
+31
-8
@@ -629,15 +629,20 @@ async function embedAllStale(
|
||||
const CONCURRENCY = parseInt(process.env.GBRAIN_EMBED_CONCURRENCY || '20', 10);
|
||||
|
||||
// D3 + D3a + D8: wall-clock budget. 30 min default; env override.
|
||||
// v0.41.18.0 (A13): --catch-up removes the wall-clock cap entirely so the
|
||||
// handler runs until countStaleChunks() returns 0. Use Number.MAX_SAFE_INTEGER
|
||||
// (effectively unbounded) instead of the 30-min default. The AbortController
|
||||
// still wraps for SIGINT propagation; just the timer never fires.
|
||||
const BUDGET_MS = staleOpts?.catchUp
|
||||
? Number.MAX_SAFE_INTEGER
|
||||
// #1946: --catch-up removes the wall-clock cap. The prior code set BUDGET_MS =
|
||||
// Number.MAX_SAFE_INTEGER and passed it to setTimeout — but setTimeout's delay
|
||||
// is a 32-bit signed int, so MAX_SAFE_INTEGER (9e15) overflows and the timer
|
||||
// fires almost immediately, aborting catch-up after a single batch. The fix is
|
||||
// to NOT arm the timer in catch-up at all: the keyset pass below terminates on
|
||||
// its own (the (page_id, chunk_index) cursor advances monotonically), and
|
||||
// SIGINT / worker-abort still propagate via externalSignal.
|
||||
const BUDGET_MS: number | null = staleOpts?.catchUp
|
||||
? null
|
||||
: parseInt(process.env.GBRAIN_EMBED_TIME_BUDGET_MS || `${30 * 60 * 1000}`, 10);
|
||||
const budgetController = new AbortController();
|
||||
const budgetTimer = setTimeout(() => budgetController.abort(), BUDGET_MS);
|
||||
const budgetTimer = BUDGET_MS != null
|
||||
? setTimeout(() => budgetController.abort(), BUDGET_MS)
|
||||
: undefined;
|
||||
const budgetSignal = budgetController.signal;
|
||||
// #1737: the effective signal fires when EITHER the internal wall-clock
|
||||
// budget OR the caller's abort (worker timeout / lock loss / SIGTERM) fires.
|
||||
@@ -660,6 +665,10 @@ async function embedAllStale(
|
||||
let afterUpdatedAt: string | null = null;
|
||||
let totalChunksLoaded = 0;
|
||||
let budgetExitNotified = false;
|
||||
// #1946 (OV2a): track chunks that errored out so a catch-up pass that finishes
|
||||
// with stale chunks still remaining (un-embeddable for a non-transient reason)
|
||||
// surfaces that loudly instead of looking like a clean run.
|
||||
let embedFailures = 0;
|
||||
|
||||
try {
|
||||
// eslint-disable-next-line no-constant-condition
|
||||
@@ -746,6 +755,7 @@ async function embedAllStale(
|
||||
// Budget/abort-fired cancellations are expected on the way out; don't
|
||||
// spam per-page "Error embedding" lines when we're shutting down.
|
||||
if (effectiveSignal.aborted) return;
|
||||
embedFailures++;
|
||||
serr(`\n Error embedding ${slug}: ${e instanceof Error ? e.message : e}`);
|
||||
}
|
||||
totalProcessedPages++;
|
||||
@@ -772,10 +782,23 @@ async function embedAllStale(
|
||||
if (batch.length < PAGE_SIZE) break;
|
||||
}
|
||||
} finally {
|
||||
clearTimeout(budgetTimer);
|
||||
if (budgetTimer) clearTimeout(budgetTimer);
|
||||
}
|
||||
|
||||
slog(`Embedded ${result.embedded} chunks across ${totalProcessedPages} pages`);
|
||||
|
||||
// #1946 (OV2a): a catch-up pass that completed without being aborted but left
|
||||
// chunks unembedded means those chunks are stuck (a non-transient embed
|
||||
// failure), not that we ran out of time. Surface it loudly so it doesn't read
|
||||
// as a clean run — re-running won't help until the underlying failure is fixed.
|
||||
if (staleOpts?.catchUp && !effectiveSignal.aborted && embedFailures > 0) {
|
||||
const remaining = await engine.countStaleChunks(
|
||||
signature ? { signature, ...(sourceId ? { sourceId } : {}) } : (sourceId ? { sourceId } : undefined),
|
||||
);
|
||||
if (remaining > 0) {
|
||||
serr(`\n [embed] catch-up finished but ${remaining} chunk(s) remain stale after ${embedFailures} embed failure(s). These are not embeddable as-is; re-running won't clear them until the underlying error is resolved.`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -39,7 +39,7 @@ if ! command -v gbrain >/dev/null 2>&1; then
|
||||
exit 0
|
||||
fi
|
||||
|
||||
staged=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\\\.mdx?$' || true)
|
||||
staged=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\.mdx?$' || true)
|
||||
[ -z "$staged" ] && exit 0
|
||||
|
||||
failed=0
|
||||
|
||||
@@ -45,6 +45,7 @@ const FRONTMATTER_RULE_NAMES: Record<ParseValidationCode, string> = {
|
||||
SLUG_MISMATCH: 'frontmatter-slug-mismatch',
|
||||
NULL_BYTES: 'frontmatter-null-bytes',
|
||||
NESTED_QUOTES: 'frontmatter-nested-quotes',
|
||||
NON_STRING_FIELD: 'frontmatter-non-string-field',
|
||||
EMPTY_FRONTMATTER: 'frontmatter-empty',
|
||||
};
|
||||
|
||||
|
||||
@@ -376,14 +376,18 @@ export function assessContentSanity(opts: {
|
||||
// doesn't repeat the lowercase per literal.
|
||||
const bodyHead = body.slice(0, SCAN_HEAD_BYTES);
|
||||
const bodyHeadLower = bodyHead.toLowerCase();
|
||||
const titleLower = opts.title.toLowerCase();
|
||||
// Belt-and-suspenders (#1883/#1658): the parser already coerces a non-string
|
||||
// frontmatter title, but any caller passing an unsanitized non-string title
|
||||
// would otherwise crash `.toLowerCase()` and abort the whole lint/sync run.
|
||||
const titleStr = typeof opts.title === 'string' ? opts.title : String(opts.title ?? '');
|
||||
const titleLower = titleStr.toLowerCase();
|
||||
|
||||
const junk_pattern_matches: string[] = [];
|
||||
for (const p of BUILT_IN_JUNK_PATTERNS) {
|
||||
const scope = p.applies_to ?? 'both';
|
||||
let matched = false;
|
||||
if (scope === 'title' || scope === 'both') {
|
||||
if (p.pattern.test(opts.title)) matched = true;
|
||||
if (p.pattern.test(titleStr)) matched = true;
|
||||
}
|
||||
if (!matched && (scope === 'body' || scope === 'both')) {
|
||||
if (p.pattern.test(bodyHead)) matched = true;
|
||||
|
||||
+36
-4
@@ -10,6 +10,7 @@ export type ParseValidationCode =
|
||||
| 'SLUG_MISMATCH'
|
||||
| 'NULL_BYTES'
|
||||
| 'NESTED_QUOTES'
|
||||
| 'NON_STRING_FIELD'
|
||||
| 'EMPTY_FRONTMATTER';
|
||||
|
||||
export interface ParseValidationError {
|
||||
@@ -105,12 +106,28 @@ export function parseMarkdown(
|
||||
|
||||
const { compiled_truth, timeline } = splitBody(body);
|
||||
|
||||
const type = (frontmatter.type as string) || (
|
||||
// #1883/#1658/#1556/#1948: frontmatter values can be non-strings (YAML coerces
|
||||
// `title: 123` → number, `published: false` → boolean). Pre-fix the `as string`
|
||||
// cast lied: a truthy non-string flowed downstream typed as string and crashed
|
||||
// the first `.toLowerCase()` (content-sanity), aborting the whole lint/sync run.
|
||||
// - title: a scalar (number/bool) is coerced to String (preserve intent;
|
||||
// `title: 123` → "123"); anything else falls back to the inferred title.
|
||||
// - type/slug: NEVER fabricated from a non-string (a coerced "123" slug
|
||||
// collides + diverges from the slug validator). Fall back to inference.
|
||||
const rawType = frontmatter.type;
|
||||
const type = (typeof rawType === 'string' && rawType.length > 0 ? rawType : (
|
||||
opts?.activePack ? inferTypeFromPack(filePath, opts.activePack) : inferType(filePath)
|
||||
);
|
||||
const title = (frontmatter.title as string) || inferTitle(filePath);
|
||||
)) as PageType;
|
||||
const rawTitle = frontmatter.title;
|
||||
const title =
|
||||
typeof rawTitle === 'string' && rawTitle.length > 0
|
||||
? rawTitle
|
||||
: (typeof rawTitle === 'number' || typeof rawTitle === 'boolean')
|
||||
? String(rawTitle)
|
||||
: inferTitle(filePath);
|
||||
const tags = extractTags(frontmatter);
|
||||
const slug = (frontmatter.slug as string) || inferSlug(filePath);
|
||||
const rawSlug = frontmatter.slug;
|
||||
const slug = typeof rawSlug === 'string' && rawSlug.length > 0 ? rawSlug : inferSlug(filePath);
|
||||
|
||||
const cleanFrontmatter = { ...frontmatter };
|
||||
delete cleanFrontmatter.type;
|
||||
@@ -289,6 +306,21 @@ function collectValidationErrors(
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// 8. NON_STRING_FIELD (#1948) — title/type/slug declared as a non-string YAML
|
||||
// scalar (e.g. `title: 123`, `slug: 2024`). The parser coerces title to a
|
||||
// string and falls back to inference for type/slug, but lint surfaces the
|
||||
// malformed frontmatter so it gets fixed rather than silently rewritten.
|
||||
// Pre-fix the slug validator above `typeof`-skipped these, hiding them.
|
||||
for (const field of ['title', 'type', 'slug'] as const) {
|
||||
const v = ctx.parsedFrontmatter[field];
|
||||
if (v != null && typeof v !== 'string') {
|
||||
errors.push({
|
||||
code: 'NON_STRING_FIELD',
|
||||
message: `Frontmatter "${field}" should be a string but is ${typeof v} (${JSON.stringify(v)}); quote the value (e.g. ${field}: "${String(v)}").`,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -326,12 +326,48 @@ function availableBrainTools(ctx: OperationContext): string[] {
|
||||
// Frontmatter projection + description
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Parse a single-line `description:` from raw frontmatter (best-effort). */
|
||||
/**
|
||||
* Parse a `description:` from raw frontmatter (best-effort).
|
||||
*
|
||||
* #1711: handles YAML block scalars. `description: |` (literal) and
|
||||
* `description: >` (folded), with optional chomping/indent indicators
|
||||
* (`|-`, `>+`, `|2`), are parsed by reading the following indented lines and
|
||||
* folding them into a single line for the one-line catalog. Pre-fix the regex
|
||||
* captured the bare `|`/`>` indicator as the description, so block-scalar skills
|
||||
* showed a literal "|" in the catalog instead of their text.
|
||||
*/
|
||||
function parseDescriptionField(raw: string): string | undefined {
|
||||
const m = raw.match(/^description:\s*["']?(.+?)["']?\s*$/m);
|
||||
if (!m) return undefined;
|
||||
const v = m[1].trim();
|
||||
return v.length > 0 ? v : undefined;
|
||||
const lines = raw.split('\n');
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const m = lines[i].match(/^description:[ \t]*(.*)$/);
|
||||
if (!m) continue;
|
||||
const inline = m[1].trim();
|
||||
|
||||
// Block scalar indicator (`|`, `>`, with optional chomp `+`/`-` and indent digit).
|
||||
if (/^[|>][+-]?\d*$/.test(inline)) {
|
||||
const block: string[] = [];
|
||||
let baseIndent: number | null = null;
|
||||
for (let j = i + 1; j < lines.length; j++) {
|
||||
const line = lines[j];
|
||||
if (line.trim().length === 0) { block.push(''); continue; }
|
||||
const indent = line.length - line.trimStart().length;
|
||||
if (baseIndent === null) {
|
||||
if (indent === 0) break; // no indented continuation
|
||||
baseIndent = indent;
|
||||
}
|
||||
if (indent < baseIndent) break; // dedent ends the block
|
||||
block.push(line.slice(baseIndent));
|
||||
}
|
||||
// Catalog descriptions are one line; collapse literal/folded whitespace.
|
||||
const folded = block.join(' ').replace(/\s+/g, ' ').trim();
|
||||
return folded.length > 0 ? folded : undefined;
|
||||
}
|
||||
|
||||
// Inline scalar: strip a matching pair of surrounding quotes.
|
||||
const unq = inline.replace(/^(['"])([\s\S]*)\1$/, '$2').trim();
|
||||
return unq.length > 0 ? unq : undefined;
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/** Strip the leading `---\n...\n---` fence; return the prose body. */
|
||||
|
||||
@@ -53,6 +53,25 @@ describe('frontmatter install-hook (B13)', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('#1840 — generated hook matches .md/.mdx (single-backslash regex, not over-escaped)', () => {
|
||||
installHook(tmp, false);
|
||||
const content = readFileSync(join(tmp, '.githooks', 'pre-commit'), 'utf8');
|
||||
// The shell must see `grep -E '\.mdx?$'`. Pre-fix it emitted `'\\.mdx?$'`
|
||||
// (literal backslash), so the hook matched nothing and silently no-opped.
|
||||
expect(content).toContain("grep -E '\\.mdx?$'");
|
||||
expect(content).not.toContain("grep -E '\\\\.mdx?$'");
|
||||
|
||||
// Prove the emitted pattern actually selects markdown files. Extract the
|
||||
// exact pattern between the single quotes and run it through ripgrep-free
|
||||
// JS regex parity (POSIX ERE `\.mdx?$` == JS `/\.mdx?$/`).
|
||||
const m = content.match(/grep -E '([^']+)'/);
|
||||
expect(m).not.toBeNull();
|
||||
const re = new RegExp(m![1]);
|
||||
expect(re.test('notes/thing.md')).toBe(true);
|
||||
expect(re.test('notes/thing.mdx')).toBe(true);
|
||||
expect(re.test('notes/thing.txt')).toBe(false);
|
||||
});
|
||||
|
||||
test('installHook refuses to clobber existing hook without --force', () => {
|
||||
const hooksDir = join(tmp, '.githooks');
|
||||
mkdirSync(hooksDir, { recursive: true });
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
/**
|
||||
* Non-string frontmatter title/type/slug (#1883, #1658, #1556, #1948).
|
||||
*
|
||||
* A YAML scalar like `title: 123` parses to a NUMBER. Pre-fix the parser cast it
|
||||
* `as string`, so a non-string flowed downstream typed as string and crashed the
|
||||
* first `.toLowerCase()` in content-sanity — aborting the whole lint/sync run
|
||||
* brain-wide (root trigger behind the never-converging-sync reports #1794/#1939).
|
||||
*
|
||||
* Fix: coerce title to a string at the parser; for slug/type fall back to
|
||||
* inference (never fabricate a "123" slug); guard content-sanity defensively;
|
||||
* and surface the malformed frontmatter via a lint NON_STRING_FIELD finding.
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { parseMarkdown } from '../src/core/markdown.ts';
|
||||
import { assessContentSanity } from '../src/core/content-sanity.ts';
|
||||
|
||||
const fm = (body: string) => `---\n${body}\n---\nbody text here\n`;
|
||||
|
||||
describe('parseMarkdown coerces/guards non-string frontmatter', () => {
|
||||
test('numeric title is coerced to a string (intent preserved)', () => {
|
||||
const p = parseMarkdown(fm('title: 123'), 'notes/thing.md');
|
||||
expect(typeof p.title).toBe('string');
|
||||
expect(p.title).toBe('123');
|
||||
});
|
||||
|
||||
test('boolean title is coerced to a string', () => {
|
||||
const p = parseMarkdown(fm('title: false'), 'notes/thing.md');
|
||||
expect(p.title).toBe('false');
|
||||
});
|
||||
|
||||
test('missing title falls back to inferred title (string)', () => {
|
||||
const p = parseMarkdown(fm('type: note'), 'notes/My Thing.md');
|
||||
expect(typeof p.title).toBe('string');
|
||||
expect(p.title.length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
test('numeric slug is NOT fabricated — falls back to inferred slug', () => {
|
||||
const p = parseMarkdown(fm('slug: 2024'), 'notes/real-slug.md');
|
||||
expect(typeof p.slug).toBe('string');
|
||||
expect(p.slug).not.toBe('2024');
|
||||
});
|
||||
|
||||
test('numeric type falls back to inferred type, not the number', () => {
|
||||
const p = parseMarkdown(fm('type: 5\ntitle: ok'), 'notes/thing.md');
|
||||
expect(typeof p.type).toBe('string');
|
||||
expect(p.type).not.toBe('5');
|
||||
});
|
||||
|
||||
test('valid string fields pass through unchanged', () => {
|
||||
const p = parseMarkdown(fm('title: Real Title\ntype: concept\nslug: my-slug'), 'x.md');
|
||||
expect(p.title).toBe('Real Title');
|
||||
expect(p.type).toBe('concept');
|
||||
expect(p.slug).toBe('my-slug');
|
||||
});
|
||||
});
|
||||
|
||||
describe('lint surfaces non-string frontmatter (NON_STRING_FIELD)', () => {
|
||||
test('numeric title produces a NON_STRING_FIELD validation error', () => {
|
||||
const p = parseMarkdown(fm('title: 123'), 'notes/thing.md', { validate: true });
|
||||
const codes = (p.errors ?? []).map(e => e.code);
|
||||
expect(codes).toContain('NON_STRING_FIELD');
|
||||
});
|
||||
|
||||
test('all-string frontmatter produces no NON_STRING_FIELD error', () => {
|
||||
const p = parseMarkdown(fm('title: Fine\ntype: note'), 'notes/thing.md', { validate: true });
|
||||
const codes = (p.errors ?? []).map(e => e.code);
|
||||
expect(codes).not.toContain('NON_STRING_FIELD');
|
||||
});
|
||||
});
|
||||
|
||||
describe('assessContentSanity never throws on a non-string title (belt-and-suspenders)', () => {
|
||||
test('numeric title does not crash the sanity pass', () => {
|
||||
expect(() =>
|
||||
assessContentSanity({ compiled_truth: 'hello world', timeline: '', title: 123 as unknown as string }),
|
||||
).not.toThrow();
|
||||
});
|
||||
|
||||
test('undefined title does not crash the sanity pass', () => {
|
||||
expect(() =>
|
||||
assessContentSanity({ compiled_truth: 'hello world', timeline: '', title: undefined as unknown as string }),
|
||||
).not.toThrow();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,49 @@
|
||||
/**
|
||||
* #1711 — skill catalog parses YAML block-scalar `description:` fields.
|
||||
*
|
||||
* Pre-fix `parseDescriptionField` matched `description: |` with a greedy regex
|
||||
* and captured the bare block indicator (`|` / `>`) as the description, so a
|
||||
* skill written with `description: |` showed a literal "|" in the catalog
|
||||
* instead of its text.
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { oneLineDescription } from '../src/core/skill-catalog.ts';
|
||||
|
||||
describe('oneLineDescription — block scalars', () => {
|
||||
test('literal block scalar (|) folds indented lines into the description', () => {
|
||||
const raw = ['name: demo', 'description: |', ' First line of the description.', ' Second line continues it.'].join('\n');
|
||||
const out = oneLineDescription(raw, 'body fallback');
|
||||
expect(out).toBe('First line of the description. Second line continues it.');
|
||||
expect(out).not.toContain('|');
|
||||
});
|
||||
|
||||
test('folded block scalar (>) is parsed too', () => {
|
||||
const raw = ['description: >', ' Folded description', ' across two lines.'].join('\n');
|
||||
expect(oneLineDescription(raw, 'fallback')).toBe('Folded description across two lines.');
|
||||
});
|
||||
|
||||
test('chomping/indent indicators (|-, >+, |2) are recognized', () => {
|
||||
const raw = ['description: |-', ' Trimmed block scalar.'].join('\n');
|
||||
expect(oneLineDescription(raw, 'fallback')).toBe('Trimmed block scalar.');
|
||||
});
|
||||
|
||||
test('block scalar with no indented continuation falls back to body prose', () => {
|
||||
const raw = ['description: |', 'name: next-key'].join('\n');
|
||||
expect(oneLineDescription(raw, 'Body prose line')).toBe('Body prose line');
|
||||
});
|
||||
});
|
||||
|
||||
describe('oneLineDescription — inline scalars still work', () => {
|
||||
test('plain inline description', () => {
|
||||
expect(oneLineDescription('description: A plain one-liner', 'fallback')).toBe('A plain one-liner');
|
||||
});
|
||||
|
||||
test('quoted inline description strips surrounding quotes', () => {
|
||||
expect(oneLineDescription('description: "Quoted desc"', 'fallback')).toBe('Quoted desc');
|
||||
expect(oneLineDescription("description: 'Single quoted'", 'fallback')).toBe('Single quoted');
|
||||
});
|
||||
|
||||
test('absent description falls back to first prose line', () => {
|
||||
expect(oneLineDescription('name: x', 'The first prose line.')).toBe('The first prose line.');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user