diff --git a/src/commands/embed.ts b/src/commands/embed.ts index 08685dc69..3afefab03 100644 --- a/src/commands/embed.ts +++ b/src/commands/embed.ts @@ -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.`); + } + } } /** diff --git a/src/commands/frontmatter-install-hook.ts b/src/commands/frontmatter-install-hook.ts index 1477d28c1..6224a982f 100644 --- a/src/commands/frontmatter-install-hook.ts +++ b/src/commands/frontmatter-install-hook.ts @@ -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 diff --git a/src/commands/lint.ts b/src/commands/lint.ts index 21b3cac55..c9bfdba8d 100644 --- a/src/commands/lint.ts +++ b/src/commands/lint.ts @@ -45,6 +45,7 @@ const FRONTMATTER_RULE_NAMES: Record = { 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', }; diff --git a/src/core/content-sanity.ts b/src/core/content-sanity.ts index 1d516075a..18c33fd69 100644 --- a/src/core/content-sanity.ts +++ b/src/core/content-sanity.ts @@ -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; diff --git a/src/core/markdown.ts b/src/core/markdown.ts index 19cf0474f..271f40393 100644 --- a/src/core/markdown.ts +++ b/src/core/markdown.ts @@ -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)}").`, + }); + } + } } /** diff --git a/src/core/skill-catalog.ts b/src/core/skill-catalog.ts index 237283521..31d6fe9e0 100644 --- a/src/core/skill-catalog.ts +++ b/src/core/skill-catalog.ts @@ -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. */ diff --git a/test/frontmatter-install-hook.test.ts b/test/frontmatter-install-hook.test.ts index 4f11177a0..baf928fe8 100644 --- a/test/frontmatter-install-hook.test.ts +++ b/test/frontmatter-install-hook.test.ts @@ -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 }); diff --git a/test/frontmatter-non-string-fields.test.ts b/test/frontmatter-non-string-fields.test.ts new file mode 100644 index 000000000..273b3f473 --- /dev/null +++ b/test/frontmatter-non-string-fields.test.ts @@ -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(); + }); +}); diff --git a/test/skill-catalog-block-scalar.test.ts b/test/skill-catalog-block-scalar.test.ts new file mode 100644 index 000000000..4f53ebc85 --- /dev/null +++ b/test/skill-catalog-block-scalar.test.ts @@ -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.'); + }); +});