diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 828505160..06d60504f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -21,11 +21,22 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} test: + # ubuntu-latest is free 2-core/7GB. Larger runners (16-cores, etc.) require + # a provisioned runner pool in repo settings. Falling back to default keeps + # the matrix shard speedup (~5-6x via parallelism) at zero cost. runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3, 4] steps: - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: bun-version: latest - run: bun install - - run: bun run test + - name: Pre-test gates (shard 1 only — they're not test files) + if: matrix.shard == 1 + run: scripts/check-jsonb-pattern.sh && scripts/check-progress-to-stdout.sh && scripts/check-wasm-embedded.sh && bun run typecheck + - name: Run test shard ${{ matrix.shard }}/4 + run: scripts/test-shard.sh ${{ matrix.shard }} 4 diff --git a/.gitignore b/.gitignore index fc91e5fa3..0a684271d 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,4 @@ eval/data/world-v1/world.html # BrainBench amara-life-v1 Opus cache (regenerate via eval:generate-amara-life) eval/data/amara-life-v1/_cache/ +.claude/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 84a61fabe..894216583 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,74 @@ All notable changes to GBrain will be documented in this file. +## [0.22.9] - 2026-04-29 + +**Sync failures now tell you why, not just how many.** +**`gbrain sync --skip-failed` and `gbrain doctor` group failures by error code, so 2,685 silent SLUG_MISMATCH files don't hide behind a single count.** + +Before this release, when sync hit per-file parse errors the only signal was a number: + +``` +Sync blocked: 2688 file(s) failed to parse. Fix the YAML frontmatter... +``` + +That count is useless when you're staring at 2,688 files and don't know what's wrong. On a real 81K-page brain, 2,685 of those turned out to be `SLUG_MISMATCH` from a posterous import — a single root cause hiding behind a giant number. It took manual `cat ~/.gbrain/sync-failures.jsonl | jq` to figure that out. + +After: + +``` +Sync blocked: 2688 file(s) failed to parse: + SLUG_MISMATCH: 2685 + YAML_DUPLICATE_KEY: 3 + +Fix the YAML frontmatter in the files above and re-run, or use 'gbrain sync --skip-failed' to acknowledge and move on. + +# gbrain sync --skip-failed +Acknowledged 2688 failure(s) and advancing past them: + SLUG_MISMATCH: 2685 + YAML_DUPLICATE_KEY: 3 +``` + +`gbrain doctor` shows the same breakdown for unacknowledged AND historical entries: + +``` +[WARN] sync_failures: 2688 unacknowledged sync failure(s) [SLUG_MISMATCH=2685, YAML_DUPLICATE_KEY=3]. +[OK] sync_failures: 500544 historical sync failure(s), all acknowledged [SLUG_MISMATCH=2685, ...]. +``` + +The classifier knows the canonical messages from `collectValidationErrors()` in `src/core/markdown.ts` (8 frontmatter codes), Postgres unique-constraint violations (`DB_DUPLICATE_KEY`), statement-timeout errors (`STATEMENT_TIMEOUT`), invalid UTF-8, and YAML duplicates. DB-layer errors check before YAML-layer ones — so a Postgres `duplicate key value violates unique constraint` no longer mislabels as a YAML duplicate. Unrecognized errors fall through to `UNKNOWN`. + +### What this means for you + +If `gbrain sync` blocks with parse failures, the breakdown tells you what to fix first. SLUG_MISMATCH is one fix-pattern (frontmatter says one slug, path says another); YAML_PARSE is a different one (malformed YAML); STATEMENT_TIMEOUT means a DB timeout, not a parse problem. You stop staring at counts and start fixing root causes. + +### For contributors + +`acknowledgeSyncFailures()` in `src/core/sync.ts` now returns `{count, summary}` instead of `number`. If you import this directly from `gbrain/sync`, replace `n` with `result.count` and use `result.summary` (an `Array<{code, count}>`) for the new code-grouped breakdown. The function is reachable via the package exports map; this is a deliberate, non-shimmed breaking change. There is a new `formatCodeBreakdown()` helper in the same module that accepts either raw failures or pre-summarized input — use it instead of building breakdown strings inline. + +### Itemized changes + +#### Added + +- `classifyErrorCode(errorMsg)` in `src/core/sync.ts` — best-effort error-code extraction from sync failure messages. Codes: `SLUG_MISMATCH`, `YAML_PARSE`, `YAML_DUPLICATE_KEY`, `MISSING_OPEN`, `MISSING_CLOSE`, `EMPTY_FRONTMATTER`, `NULL_BYTES`, `NESTED_QUOTES`, `DB_DUPLICATE_KEY`, `STATEMENT_TIMEOUT`, `INVALID_UTF8`, `UNKNOWN`. +- `summarizeFailuresByCode(failures)` — groups failures by code and returns a sorted `Array<{code, count}>`. +- `formatCodeBreakdown(input)` — renders a multi-line `code: count` string from either raw failures or a pre-computed summary. Single helper, two input shapes. +- `code?: string` field on the `SyncFailure` JSONL row in `~/.gbrain/sync-failures.jsonl`. Populated at write-time so the classifier runs once per failure, not on every load. +- `AcknowledgeResult` interface as the new return shape of `acknowledgeSyncFailures()`. +- 15 new test cases in `test/sync-failures.test.ts`: DB-vs-YAML duplicate-key disambiguation, canonical-message coverage for all 7 frontmatter codes, `acknowledgeSyncFailures()` legacy-entry backfill branch, `formatCodeBreakdown()` dual-input shape. + +#### Changed + +- `gbrain sync` blocked-message: now lists code breakdown above the fix instructions (both incremental and full-sync paths). +- `gbrain sync --skip-failed` ack message: now lists what was skipped, grouped by code. +- `gbrain doctor` `sync_failures` check: warn-and-ok messages both include `[code=count, ...]` breakdown. +- `recordSyncFailures()` now stores `code` alongside `error` so downstream readers don't re-classify. +- `acknowledgeSyncFailures()` backfills `code` on legacy rows that predate the field — upgrade-safe for users with existing `~/.gbrain/sync-failures.jsonl`. +- DB-layer error patterns (`DB_DUPLICATE_KEY`, `STATEMENT_TIMEOUT`) check BEFORE YAML patterns in the classifier, so Postgres errors don't get YAML-labeled. +- Frontmatter regex patterns rewritten to match canonical messages from `collectValidationErrors()` (`File is empty...`, `No closing --- delimiter found`, `Frontmatter block is empty`) instead of aspirational code-token strings (`missing.*open`) that never appeared in practice. + +Closes #500. Eng-review plan: `~/.claude/plans/then-codex-synchronous-toucan.md` (codex outside-voice agreed on all 7 findings). + ## [0.22.8] - 2026-04-28 ## **Doctor stops timing out on Supabase. Integrity scan finishes in ~6s, multi-source brains get correct counts.** diff --git a/TODOS.md b/TODOS.md index 4ed2ce48f..80609105c 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,93 @@ # TODOS +## sync error-code classification (PR #501 follow-ups) + +### Plumb structured `ParseValidationCode` through `ImportResult` +**Priority:** P2 + +**What:** Replace the regex-on-error-message path in `src/core/sync.ts:classifyErrorCode` +with a structured `code` field threaded through `ImportResult` from the parse layer. + +Three changes: +1. `src/core/import-file.ts:362` — call `parseMarkdown(content, relativePath, { validate: true, expectedSlug })` + so `parsed.errors[0].code` is populated. +2. `src/core/import-file.ts` — add `code?: string` to `ImportResult`. Promote the + structured code (or `'SLUG_MISMATCH'` when the existing expectedSlug check trips) + into the result envelope alongside `error`. +3. `src/commands/sync.ts:488` — extend `failedFiles` shape with `code?: string`. + `recordSyncFailures` already accepts the field; the only thing missing is the + capture site populating it. +4. `src/core/sync.ts:classifyErrorCode` — keep as a fallback for un-coded errors + (DB exceptions, generic catches). Primary path reads the structured code. + +**Why:** The repo already has `ParseValidationCode` + `ParseValidationError` in +`src/core/markdown.ts:5-18`, and three other consumers (`src/commands/lint.ts:72`, +`src/commands/frontmatter.ts:148`, `src/core/brain-writer.ts:314`) read structured +errors directly. Sync is the outlier — it calls `parseMarkdown` without validation +and reverse-engineers codes via regex. PR #501 shipped that regex out of pragmatism; +this TODO removes ~50% of `classifyErrorCode` and eliminates a class of false-positives. + +**Pros:** +- One source of truth for parse codes (the enum in `markdown.ts`). +- Eliminates regex fragility — adding a new validation code in `markdown.ts` + automatically flows to sync without a new regex. +- Closes the case where canonical messages (`File is empty...`, `No closing ---...`) + don't match aspirational regex patterns. + +**Cons:** Touches `ImportResult` interface, which ripples through `src/commands/import.ts:105`, +`src/commands/sync.ts:498-510`, `src/core/cycle.ts`, brain-writer reconciler. + +**Context:** PR #501 documented this as P3 in the eng review at +`~/.claude/plans/then-codex-synchronous-toucan.md`. Codex's outside-voice review +agreed independently. The fix is small — ~50 lines including tests + downstream +call sites — and it's the correct architectural endpoint. + +**Effort:** M (human: ~2 hr / CC: ~20 min). + +**Depends on / blocked by:** Nothing. + +### CHANGELOG migration note for `acknowledgeSyncFailures()` shape change +**Priority:** P0 — required at /ship time + +**What:** When PR #501 ships, the release CHANGELOG entry MUST include this +`### For contributors` block: + +```markdown +### For contributors + +`acknowledgeSyncFailures()` now returns `{count, summary}` instead of `number`. +If you import this directly from `gbrain/sync`, replace `n` with `result.count` +and use `result.summary` for the new code-grouped breakdown. +``` + +**Why:** The function is exported from `src/core/sync.ts:433` and reachable via +the package exports map. External TS consumers (gbrain-evals, host agent forks) +that imported it got `number` and now get an object — silent type break. + +**Effort:** XS (human: ~1 min). Just don't forget. + +**Depends on / blocked by:** PR #501 ship. + +### Concurrent-safe ack of `~/.gbrain/sync-failures.jsonl` +**Priority:** P3 + +**What:** Two concurrent `gbrain sync` runs hitting `acknowledgeSyncFailures()` +can clobber each other. The function does a whole-file `writeFileSync` rewrite +(`src/core/sync.ts:433-455`); `recordSyncFailures()` does independent +`appendFileSync` (`src/core/sync.ts:395-416`). Concurrent ack + append can lose rows. + +**Why:** Pre-existing — predates PR #501. Real risk only on autopilot setups where +multiple sync invocations might overlap (rare today, more likely as multi-source +sync matures). + +**Fix sketch:** Atomic rename pattern (write to `sync-failures.jsonl.tmp`, then +`renameSync`) plus a file lock for the read-modify-write cycle. Or move the +acknowledged-set to the DB. + +**Effort:** S (human: ~1 hr / CC: ~10 min). + +**Depends on / blocked by:** Nothing. + ## test-infra ### Parallel-load timeout flake on v0.21 PGLite-heavy tests diff --git a/VERSION b/VERSION index eca6a4a43..6b9f278d0 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.22.8 +0.22.9 diff --git a/package.json b/package.json index 61f20235c..71692b75d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.22.8", + "version": "0.22.9", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", diff --git a/scripts/test-shard.sh b/scripts/test-shard.sh new file mode 100755 index 000000000..5f836c268 --- /dev/null +++ b/scripts/test-shard.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# Partition unit test files into N shards by stable hash and run one shard. +# +# Usage: scripts/test-shard.sh +# shard-index: 1-based (1..N) +# total-shards: positive integer +# +# E2E tests under test/e2e/ are excluded — they need DATABASE_URL and run via +# bun run test:e2e separately. +# +# Stable partitioning: a file's shard is `(hash(path) % N) + 1`. Same file +# lands in the same shard on every run, regardless of how many other files +# exist, so retries are reproducible. Hash is FNV-1a — pure shell, no jq. +set -euo pipefail + +if [ "$#" -ne 2 ]; then + echo "usage: scripts/test-shard.sh " >&2 + exit 1 +fi + +SHARD_INDEX="$1" +TOTAL_SHARDS="$2" + +if ! [[ "$SHARD_INDEX" =~ ^[0-9]+$ ]] || ! [[ "$TOTAL_SHARDS" =~ ^[0-9]+$ ]]; then + echo "error: shard index and total must be positive integers" >&2 + exit 1 +fi +if [ "$SHARD_INDEX" -lt 1 ] || [ "$SHARD_INDEX" -gt "$TOTAL_SHARDS" ]; then + echo "error: shard index $SHARD_INDEX out of range 1..$TOTAL_SHARDS" >&2 + exit 1 +fi + +cd "$(dirname "$0")/.." + +# Find all unit test files, deterministic order. Excludes test/e2e/. +# Portable: avoid `mapfile` (bash 4+) so this runs on macOS bash 3.2 too. +FILES=() +while IFS= read -r line; do + FILES+=("$line") +done < <(find test -name '*.test.ts' -not -path 'test/e2e/*' | sort) + +if [ "${#FILES[@]}" -eq 0 ]; then + echo "no test files found under test/" >&2 + exit 1 +fi + +# FNV-1a 32-bit hash of a string — implemented in pure bash so we don't depend +# on python/openssl/etc on the runner. Output is decimal. +fnv1a() { + local str="$1" + local h=2166136261 # FNV offset basis + local i ord + for (( i=0; i<${#str}; i++ )); do + ord=$(printf '%d' "'${str:$i:1}") + h=$(( (h ^ ord) & 0xFFFFFFFF )) + h=$(( (h * 16777619) & 0xFFFFFFFF )) + done + echo "$h" +} + +SHARD_FILES=() +for f in "${FILES[@]}"; do + hash=$(fnv1a "$f") + bucket=$(( hash % TOTAL_SHARDS + 1 )) + if [ "$bucket" -eq "$SHARD_INDEX" ]; then + SHARD_FILES+=("$f") + fi +done + +echo "shard $SHARD_INDEX/$TOTAL_SHARDS: ${#SHARD_FILES[@]}/${#FILES[@]} files" +if [ "${#SHARD_FILES[@]}" -eq 0 ]; then + echo "warning: shard $SHARD_INDEX has no files (rehash or reduce shard count)" >&2 + exit 0 +fi + +exec bun test --timeout=60000 "${SHARD_FILES[@]}" diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 45eb88d36..d12277c2d 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -249,25 +249,29 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo // Without this doctor check, users see "sync blocked" and have no // surface showing which files to fix. try { - const { unacknowledgedSyncFailures, loadSyncFailures } = await import('../core/sync.ts'); + const { unacknowledgedSyncFailures, loadSyncFailures, summarizeFailuresByCode } = await import('../core/sync.ts'); const unacked = unacknowledgedSyncFailures(); const all = loadSyncFailures(); if (unacked.length > 0) { + const codeSummary = summarizeFailuresByCode(unacked); + const codeBreakdown = codeSummary.map(s => `${s.code}=${s.count}`).join(', '); const preview = unacked.slice(0, 3).map(f => `${f.path} (${f.error.slice(0, 60)})`).join('; '); checks.push({ name: 'sync_failures', status: 'warn', message: - `${unacked.length} unacknowledged sync failure(s). ${preview}` + + `${unacked.length} unacknowledged sync failure(s) [${codeBreakdown}]. ${preview}` + `${unacked.length > 3 ? `, and ${unacked.length - 3} more` : ''}. ` + `Fix the file(s) and re-run 'gbrain sync', or use 'gbrain sync --skip-failed' to acknowledge.`, }); } else if (all.length > 0) { - // Acknowledged-only: informational, not a warning. + // Acknowledged-only: show code breakdown for visibility. + const ackedSummary = summarizeFailuresByCode(all); + const ackedBreakdown = ackedSummary.map(s => `${s.code}=${s.count}`).join(', '); checks.push({ name: 'sync_failures', status: 'ok', - message: `${all.length} historical sync failure(s), all acknowledged.`, + message: `${all.length} historical sync failure(s), all acknowledged [${ackedBreakdown}].`, }); } } catch { diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 0e5e40fbc..2bb28cb4e 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -12,6 +12,7 @@ import { recordSyncFailures, unacknowledgedSyncFailures, acknowledgeSyncFailures, + formatCodeBreakdown, } from '../core/sync.ts'; import { estimateTokens, CHUNKER_VERSION } from '../core/chunkers/code.ts'; import { EMBEDDING_MODEL, estimateEmbeddingCostUsd } from '../core/embedding.ts'; @@ -522,9 +523,13 @@ export async function performSync(engine: BrainEngine, opts: SyncOpts): Promise< // current set, --retry-failed re-parses before running the normal sync. if (failedFiles.length > 0) { recordSyncFailures(failedFiles, headCommit); + // Emit structured summary grouped by error code so the operator + // can see *why* files failed, not just how many. + const codeBreakdown = formatCodeBreakdown(failedFiles); if (!opts.skipFailed) { console.error( - `\nSync blocked: ${failedFiles.length} file(s) failed to parse. ` + + `\nSync blocked: ${failedFiles.length} file(s) failed to parse:\n` + + `${codeBreakdown}\n\n` + `Fix the YAML frontmatter in the files above and re-run, or use ` + `'gbrain sync --skip-failed' to acknowledge and move on.`, ); @@ -547,8 +552,11 @@ export async function performSync(engine: BrainEngine, opts: SyncOpts): Promise< } // --skip-failed: acknowledge the now-recorded set and proceed. const acked = acknowledgeSyncFailures(); - if (acked > 0) { - console.error(` Acknowledged ${acked} failure(s) and advancing past them.`); + if (acked.count > 0) { + console.error( + ` Acknowledged ${acked.count} failure(s) and advancing past them:\n` + + `${formatCodeBreakdown(acked.summary)}`, + ); } } @@ -656,9 +664,11 @@ async function performFullSync( // the sync module owns the last_commit write. Respect the same gate. if (result.failures.length > 0) { recordSyncFailures(result.failures, headCommit); + const codeBreakdown = formatCodeBreakdown(result.failures); if (!opts.skipFailed) { console.error( - `\nFull sync blocked: ${result.failures.length} file(s) failed. ` + + `\nFull sync blocked: ${result.failures.length} file(s) failed:\n` + + `${codeBreakdown}\n\n` + `Fix the YAML in those files and re-run, or use '--skip-failed'.`, ); await engine.setConfig('sync.last_run', new Date().toISOString()); @@ -675,7 +685,12 @@ async function performFullSync( }; } const acked = acknowledgeSyncFailures(); - if (acked > 0) console.error(` Acknowledged ${acked} failure(s) and advancing past them.`); + if (acked.count > 0) { + console.error( + ` Acknowledged ${acked.count} failure(s) and advancing past them:\n` + + `${formatCodeBreakdown(acked.summary)}`, + ); + } } // Persist sync state so next sync is incremental (C1 fix: was missing). diff --git a/src/core/sync.ts b/src/core/sync.ts index 3876b9d6b..6a9c0e4c5 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -307,6 +307,8 @@ import { createHash as _createHash } from 'crypto'; export interface SyncFailure { path: string; error: string; + /** Structured error code extracted from the error message. */ + code?: string; commit: string; line?: number; ts: string; @@ -314,6 +316,86 @@ export interface SyncFailure { acknowledged_at?: string; } +/** + * Best-effort extraction of a structured error code from a sync failure + * message. Matches known ParseValidationCode patterns (SLUG_MISMATCH, + * YAML_PARSE, etc.) and common DB / timeout errors. Returns 'UNKNOWN' + * when no pattern matches. + * + * Order matters: DB-layer errors are checked BEFORE YAML-layer ones so + * Postgres `duplicate key value violates unique constraint` doesn't get + * mislabeled as a YAML duplicate-key. Frontmatter patterns key off the + * canonical messages emitted by `collectValidationErrors()` in markdown.ts. + */ +export function classifyErrorCode(errorMsg: string): string { + // SLUG_MISMATCH: thrown by importFromFile() at src/core/import-file.ts:374. + if (/slug.*does not match|SLUG_MISMATCH/i.test(errorMsg)) return 'SLUG_MISMATCH'; + + // DB-layer errors come BEFORE the YAML duplicate-key check. Postgres unique- + // constraint violations contain "duplicate key" but are not a YAML problem. + if (/duplicate key value violates unique constraint|DB_DUPLICATE_KEY/i.test(errorMsg)) { + return 'DB_DUPLICATE_KEY'; + } + if (/canceling statement due to statement timeout|STATEMENT_TIMEOUT/i.test(errorMsg)) { + return 'STATEMENT_TIMEOUT'; + } + + // YAML / frontmatter patterns. These match either the canonical message + // strings in src/core/markdown.ts (collectValidationErrors) or the literal + // ParseValidationCode token, so they fire whether the caller stores the + // message or just the code. + if (/YAML parse failed|YAML_PARSE/i.test(errorMsg)) return 'YAML_PARSE'; + if (/YAMLException|duplicated mapping key|YAML_DUPLICATE_KEY/i.test(errorMsg)) { + return 'YAML_DUPLICATE_KEY'; + } + if (/File is empty or whitespace-only|Frontmatter must start with ---|MISSING_OPEN/i.test(errorMsg)) { + return 'MISSING_OPEN'; + } + if (/No closing --- delimiter|Heading at line .* found inside frontmatter|MISSING_CLOSE/i.test(errorMsg)) { + return 'MISSING_CLOSE'; + } + if (/Frontmatter block is empty|EMPTY_FRONTMATTER/i.test(errorMsg)) return 'EMPTY_FRONTMATTER'; + if (/Content contains null bytes|NULL_BYTES|null byte/i.test(errorMsg)) return 'NULL_BYTES'; + if (/Nested double quotes|NESTED_QUOTES/i.test(errorMsg)) return 'NESTED_QUOTES'; + + // Generic fallbacks. + if (/invalid UTF-?8|INVALID_UTF8/i.test(errorMsg)) return 'INVALID_UTF8'; + return 'UNKNOWN'; +} + +/** Group failures by error code and return a sorted summary. */ +export function summarizeFailuresByCode( + failures: Array<{ error: string; code?: string }>, +): Array<{ code: string; count: number }> { + const counts: Record = {}; + for (const f of failures) { + const code = f.code ?? classifyErrorCode(f.error); + counts[code] = (counts[code] ?? 0) + 1; + } + return Object.entries(counts) + .sort(([, a], [, b]) => b - a) + .map(([code, count]) => ({ code, count })); +} + +/** + * Format a code-grouped summary as a human-readable multi-line string for + * stderr / doctor output. Accepts either raw failures (which are summarized + * internally) or an already-summarized `{code, count}[]` shape (the return + * value of `summarizeFailuresByCode` or `AcknowledgeResult.summary`). + * Returns an empty string when the input is empty. + */ +export function formatCodeBreakdown( + input: Array<{ error: string; code?: string }> | Array<{ code: string; count: number }>, +): string { + // Distinguish by shape: summary entries have a numeric `count`. Empty array + // returns '' from either branch — both paths produce a 0-length join. + const summary = + input.length > 0 && typeof (input[0] as { count?: unknown }).count === 'number' + ? (input as Array<{ code: string; count: number }>) + : summarizeFailuresByCode(input as Array<{ error: string; code?: string }>); + return summary.map(s => ` ${s.code}: ${s.count}`).join('\n'); +} + function _failuresDir(): string { return _joinPath(_homedir(), '.gbrain'); } @@ -370,6 +452,7 @@ export function recordSyncFailures( const entry: SyncFailure = { path: f.path, error: f.error, + code: classifyErrorCode(f.error), commit, line: f.line, ts: now, @@ -380,28 +463,42 @@ export function recordSyncFailures( } } +export interface AcknowledgeResult { + count: number; + summary: Array<{ code: string; count: number }>; +} + /** * Mark all unacknowledged failures as acknowledged. Used by - * `gbrain sync --skip-failed`. Returns the number newly acknowledged. + * `gbrain sync --skip-failed`. Returns count and a structured summary + * grouped by error code so the operator can see *why* files were skipped. * * We do not delete — acknowledged entries stay as historical record so * doctor can still show them under a "previously skipped" bucket. */ -export function acknowledgeSyncFailures(): number { +export function acknowledgeSyncFailures(): AcknowledgeResult { const entries = loadSyncFailures(); - if (entries.length === 0) return 0; + if (entries.length === 0) return { count: 0, summary: [] }; const now = new Date().toISOString(); let changed = 0; + const newlyAcked: SyncFailure[] = []; const updated = entries.map(e => { if (e.acknowledged) return e; changed++; - return { ...e, acknowledged: true, acknowledged_at: now }; + // Backfill code for entries that predate the code field. + const code = e.code ?? classifyErrorCode(e.error); + const acked = { ...e, code, acknowledged: true, acknowledged_at: now }; + newlyAcked.push(acked); + return acked; }); - if (changed === 0) return 0; + if (changed === 0) return { count: 0, summary: [] }; _mkdirSync(_failuresDir(), { recursive: true }); const fd = require('fs').writeFileSync; fd(syncFailuresPath(), updated.map(e => JSON.stringify(e)).join('\n') + '\n'); - return changed; + return { + count: changed, + summary: summarizeFailuresByCode(newlyAcked), + }; } /** Return only unacknowledged failures. */ diff --git a/test/brain-writer.test.ts b/test/brain-writer.test.ts index b9b60ef95..5710d0bf3 100644 --- a/test/brain-writer.test.ts +++ b/test/brain-writer.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test, beforeEach, afterEach } from 'bun:test'; +import { describe, expect, test, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test'; import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync, mkdirSync, symlinkSync } from 'fs'; import { join } from 'path'; import { tmpdir } from 'os'; @@ -9,6 +9,7 @@ import { BrainWriterError, } from '../src/core/brain-writer.ts'; import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; const fence = '---'; @@ -115,15 +116,24 @@ describe('scanBrainSources (PGLite)', () => { let tmp: string; let engine: PGLiteEngine; - beforeEach(async () => { - tmp = mkdtempSync(join(tmpdir(), 'brain-writer-scan-')); + // One PGLite per file — beforeEach wipes data only. PGLite cold-start is + // ~20s on CI; sharing one engine across 6 tests in this block saves ~2 min. + beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({}); await engine.initSchema(); }); - afterEach(async () => { + afterAll(async () => { await engine.disconnect(); + }); + + beforeEach(async () => { + await resetPgliteState(engine); + tmp = mkdtempSync(join(tmpdir(), 'brain-writer-scan-')); + }); + + afterEach(() => { rmSync(tmp, { recursive: true, force: true }); }); diff --git a/test/extract-incremental.test.ts b/test/extract-incremental.test.ts index 7116774f9..a4453d77c 100644 --- a/test/extract-incremental.test.ts +++ b/test/extract-incremental.test.ts @@ -7,28 +7,39 @@ * * All tests use PGLite/in-memory — no DB connection required. */ -import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test'; import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'fs'; import { tmpdir } from 'os'; import { join } from 'path'; import { runExtractCore } from '../src/commands/extract.ts'; import { PGLiteEngine } from '../src/core/pglite-engine.ts'; import type { BrainEngine } from '../src/core/engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +// One PGLite per file (beforeAll), wipe data per test (beforeEach). +// PGLite cold-start dominates wall-time; sharing the engine across all tests +// in this file cuts ~22s × 8 tests = ~3 min on CI. let engine: PGLiteEngine; let tempDir: string; -beforeEach(async () => { +beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({ engine: 'pglite' }); await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); tempDir = mkdtempSync(join(tmpdir(), 'gbrain-extract-test-')); mkdirSync(join(tempDir, 'people'), { recursive: true }); mkdirSync(join(tempDir, 'companies'), { recursive: true }); }); -afterEach(async () => { - await engine.disconnect(); +afterEach(() => { rmSync(tempDir, { recursive: true, force: true }); }); diff --git a/test/helpers/reset-pglite.ts b/test/helpers/reset-pglite.ts new file mode 100644 index 000000000..747fe2a84 --- /dev/null +++ b/test/helpers/reset-pglite.ts @@ -0,0 +1,43 @@ +/** + * Wipe per-test data on a connected PGLite engine without dropping the schema. + * Used by tests that share one engine across the file (beforeAll) and need a + * clean slate per test (beforeEach). + * + * Why this exists: PGLite WASM cold-start + initSchema() is ~20s on CI runners. + * Spinning up a fresh engine per test (the prior beforeEach pattern) multiplies + * that across every test in every file. Sharing one engine and wiping data + * is two orders of magnitude faster. + * + * Implementation: + * 1. TRUNCATE every public table CASCADE, including `sources` (so tests + * that register their own sources don't leak rows into the next test). + * 2. Re-seed the default source row that pages.source_id's DEFAULT FKs + * against. Without this, the next page insert would fail FK validation. + * 3. Preserve `schema_version` — it carries the migration ledger that + * initSchema() populates; wiping it would make migration helpers think + * the brain is on v0. + * + * Identifier-quoted defensively against pathological table names. + */ +import type { PGLiteEngine } from '../../src/core/pglite-engine.ts'; + +const PRESERVE_TABLES = new Set(['schema_version']); + +export async function resetPgliteState(engine: PGLiteEngine): Promise { + const rows = await engine.executeRaw<{ tablename: string }>( + `SELECT tablename FROM pg_tables WHERE schemaname='public'`, + ); + const targets = rows + .map(r => r.tablename) + .filter(name => !PRESERVE_TABLES.has(name)); + if (targets.length === 0) return; + const quoted = targets.map(t => `"${t.replace(/"/g, '""')}"`).join(', '); + await engine.executeRaw(`TRUNCATE ${quoted} RESTART IDENTITY CASCADE`); + // Re-seed the default source row that initSchema() inserts. Mirrors the + // INSERT in src/core/pglite-schema.ts so the FK target survives reset. + await engine.executeRaw( + `INSERT INTO sources (id, name, config) + VALUES ('default', 'default', '{"federated": true}'::jsonb) + ON CONFLICT (id) DO NOTHING`, + ); +} diff --git a/test/sync-failures.test.ts b/test/sync-failures.test.ts index 7fa9a85ef..6f56e0aed 100644 --- a/test/sync-failures.test.ts +++ b/test/sync-failures.test.ts @@ -76,18 +76,19 @@ describe('Bug 9 — sync-failures JSONL helpers', () => { { path: 'b.md', error: 'err2' }, ], 'commit1'); - const n = acknowledgeSyncFailures(); - expect(n).toBe(2); + const result = acknowledgeSyncFailures(); + expect(result.count).toBe(2); + expect(result.summary.length).toBeGreaterThan(0); const after = loadSyncFailures(); expect(after.every(e => e.acknowledged === true)).toBe(true); expect(after.every(e => typeof e.acknowledged_at === 'string')).toBe(true); // Second ack: nothing new to mark. - expect(acknowledgeSyncFailures()).toBe(0); + expect(acknowledgeSyncFailures().count).toBe(0); // Adding a fresh failure then ack: only the new one flips. recordSyncFailures([{ path: 'c.md', error: 'err3' }], 'commit2'); - expect(acknowledgeSyncFailures()).toBe(1); + expect(acknowledgeSyncFailures().count).toBe(1); expect(loadSyncFailures().length).toBe(3); expect(loadSyncFailures().every(e => e.acknowledged === true)).toBe(true); }); @@ -158,3 +159,274 @@ describe('Bug 9 — sync.ts CLI flag wiring', () => { expect(source).toContain('recordSyncFailures'); }); }); + +describe('classifyErrorCode — error message to code mapping', () => { + test('classifies SLUG_MISMATCH from error message', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode( + 'Frontmatter slug "my-friend-mike" does not match path-derived slug "2008-03-20-my-friend-mike"' + )).toBe('SLUG_MISMATCH'); + }); + + test('classifies YAML_PARSE from error message', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('YAML parse failed: unexpected colon in title')).toBe('YAML_PARSE'); + }); + + test('classifies YAML_DUPLICATE_KEY', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('YAMLException: duplicated mapping key')).toBe('YAML_DUPLICATE_KEY'); + }); + + test('classifies STATEMENT_TIMEOUT', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('canceling statement due to statement timeout')).toBe('STATEMENT_TIMEOUT'); + }); + + test('classifies NULL_BYTES', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('invalid UTF-8: null byte at position 3770')).toBe('NULL_BYTES'); + }); + + test('classifies INVALID_UTF8', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('invalid UTF-8 sequence at position 500')).toBe('INVALID_UTF8'); + }); + + test('returns UNKNOWN for unrecognized errors', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('something completely different')).toBe('UNKNOWN'); + }); +}); + +describe('summarizeFailuresByCode — grouped summary', () => { + test('groups failures by classified code', async () => { + const { summarizeFailuresByCode } = await import('../src/core/sync.ts'); + const summary = summarizeFailuresByCode([ + { error: 'Frontmatter slug "a" does not match path-derived slug "b"' }, + { error: 'Frontmatter slug "c" does not match path-derived slug "d"' }, + { error: 'YAML parse failed: bad colon' }, + { error: 'something unknown' }, + ]); + expect(summary).toEqual([ + { code: 'SLUG_MISMATCH', count: 2 }, + { code: 'YAML_PARSE', count: 1 }, + { code: 'UNKNOWN', count: 1 }, + ]); + }); + + test('respects pre-classified code field', async () => { + const { summarizeFailuresByCode } = await import('../src/core/sync.ts'); + const summary = summarizeFailuresByCode([ + { error: 'anything', code: 'SLUG_MISMATCH' }, + { error: 'anything', code: 'SLUG_MISMATCH' }, + { error: 'anything', code: 'YAML_PARSE' }, + ]); + expect(summary).toEqual([ + { code: 'SLUG_MISMATCH', count: 2 }, + { code: 'YAML_PARSE', count: 1 }, + ]); + }); + + test('returns empty array for no failures', async () => { + const { summarizeFailuresByCode } = await import('../src/core/sync.ts'); + expect(summarizeFailuresByCode([])).toEqual([]); + }); +}); + +describe('acknowledgeSyncFailures — structured return', () => { + test('returns count and code summary', async () => { + const { recordSyncFailures, acknowledgeSyncFailures } = await import('../src/core/sync.ts'); + recordSyncFailures([ + { path: 'a.md', error: 'Frontmatter slug "x" does not match path-derived slug "y"' }, + { path: 'b.md', error: 'Frontmatter slug "p" does not match path-derived slug "q"' }, + { path: 'c.md', error: 'YAML parse failed: bad' }, + ], 'commit1'); + + const result = acknowledgeSyncFailures(); + expect(result.count).toBe(3); + expect(result.summary).toEqual([ + { code: 'SLUG_MISMATCH', count: 2 }, + { code: 'YAML_PARSE', count: 1 }, + ]); + }); +}); + +describe('recordSyncFailures — code field', () => { + test('records classified code alongside error message', async () => { + const { recordSyncFailures, loadSyncFailures } = await import('../src/core/sync.ts'); + recordSyncFailures([ + { path: 'a.md', error: 'Frontmatter slug "x" does not match path-derived slug "y"' }, + ], 'commit1'); + + const entries = loadSyncFailures(); + expect(entries[0].code).toBe('SLUG_MISMATCH'); + }); +}); + +// classifyErrorCode disambiguates Postgres unique-constraint errors from +// YAML duplicate-key errors. Pre-fix, every "duplicate.*key" string mapped +// to YAML_DUPLICATE_KEY, which mislabels DB-layer failures during sync. +describe('classifyErrorCode — DB vs YAML duplicate-key disambiguation', () => { + test('Postgres unique-constraint violation classifies as DB_DUPLICATE_KEY', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode( + 'duplicate key value violates unique constraint "pages_slug_key"' + )).toBe('DB_DUPLICATE_KEY'); + }); + + test('YAML duplicated mapping key still classifies as YAML_DUPLICATE_KEY', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('YAMLException: duplicated mapping key "title"')) + .toBe('YAML_DUPLICATE_KEY'); + }); + + test('DB pattern is checked BEFORE YAML so DB errors are not mislabeled', async () => { + // Both patterns historically matched /duplicate.*key/i — order matters now. + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode( + 'duplicate key value violates unique constraint on table "pages"' + )).toBe('DB_DUPLICATE_KEY'); + expect(classifyErrorCode( + 'duplicate key value violates unique constraint on table "pages"' + )).not.toBe('YAML_DUPLICATE_KEY'); + }); +}); + +// classifyErrorCode matches the canonical messages emitted by +// collectValidationErrors() in src/core/markdown.ts. Pre-fix, the regexes +// keyed off "missing open" / "missing close" / "empty frontmatter" — none +// of which are produced upstream. Today these all classify correctly. +describe('classifyErrorCode — canonical message coverage', () => { + test('MISSING_OPEN matches "File is empty or whitespace-only"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode( + 'File is empty or whitespace-only; expected frontmatter starting with ---' + )).toBe('MISSING_OPEN'); + }); + + test('MISSING_OPEN matches "Frontmatter must start with ---"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode( + 'Frontmatter must start with --- on the first non-empty line' + )).toBe('MISSING_OPEN'); + }); + + test('MISSING_CLOSE matches "No closing --- delimiter"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('No closing --- delimiter found')).toBe('MISSING_CLOSE'); + }); + + test('MISSING_CLOSE matches "Heading at line N found inside frontmatter"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode( + 'Heading at line 5 found inside frontmatter zone (closing --- comes after)' + )).toBe('MISSING_CLOSE'); + }); + + test('EMPTY_FRONTMATTER matches "Frontmatter block is empty"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('Frontmatter block is empty')).toBe('EMPTY_FRONTMATTER'); + }); + + test('NULL_BYTES matches "Content contains null bytes"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('Content contains null bytes (likely binary corruption)')) + .toBe('NULL_BYTES'); + }); + + test('NESTED_QUOTES matches "Nested double quotes"', async () => { + const { classifyErrorCode } = await import('../src/core/sync.ts'); + expect(classifyErrorCode('Nested double quotes in YAML value at line 3')) + .toBe('NESTED_QUOTES'); + }); +}); + +// acknowledgeSyncFailures backfills `code` on legacy entries that were +// recorded before the code field existed (~/.gbrain/sync-failures.jsonl +// from pre-PR brains). Without this branch, upgraded users see "UNKNOWN" +// for every previously-recorded failure even when the message is parseable. +describe('acknowledgeSyncFailures — backfill on legacy entries', () => { + test('backfills code on entries that predate the code field', async () => { + const { acknowledgeSyncFailures, loadSyncFailures, syncFailuresPath } = + await import('../src/core/sync.ts'); + + // Hand-write a legacy entry with no `code` field. Mimics a pre-PR + // ~/.gbrain/sync-failures.jsonl row that exists on real upgrades. + const { mkdirSync } = await import('fs'); + const { dirname } = await import('path'); + mkdirSync(dirname(syncFailuresPath()), { recursive: true }); + writeFileSync( + syncFailuresPath(), + JSON.stringify({ + path: 'a.md', + error: 'Frontmatter slug "x" does not match path-derived slug "y"', + commit: 'old', + ts: '2025-01-01T00:00:00Z', + }) + '\n', + ); + + const result = acknowledgeSyncFailures(); + expect(result.count).toBe(1); + expect(result.summary).toEqual([{ code: 'SLUG_MISMATCH', count: 1 }]); + + const after = loadSyncFailures(); + expect(after).toHaveLength(1); + expect(after[0].code).toBe('SLUG_MISMATCH'); + expect(after[0].acknowledged).toBe(true); + }); + + test('preserves existing code field; never reclassifies', async () => { + const { acknowledgeSyncFailures, loadSyncFailures, syncFailuresPath } = + await import('../src/core/sync.ts'); + + const { mkdirSync } = await import('fs'); + const { dirname } = await import('path'); + mkdirSync(dirname(syncFailuresPath()), { recursive: true }); + // Pre-classified entry — should NOT be re-run through classifier. + writeFileSync( + syncFailuresPath(), + JSON.stringify({ + path: 'a.md', + error: 'some message that would otherwise classify as UNKNOWN', + code: 'CUSTOM_CODE', + commit: 'x', + ts: '2025-01-01T00:00:00Z', + }) + '\n', + ); + + const result = acknowledgeSyncFailures(); + expect(result.summary).toEqual([{ code: 'CUSTOM_CODE', count: 1 }]); + expect(loadSyncFailures()[0].code).toBe('CUSTOM_CODE'); + }); +}); + +// formatCodeBreakdown is the DRY helper used by both the failures-array +// path (sync.ts blocked-by-failures + full-sync stderr) and the pre-summarized +// AcknowledgeResult.summary path (--skip-failed ack message). One renderer, +// two input shapes. +describe('formatCodeBreakdown — dual input shape', () => { + test('renders raw failures by classifying internally', async () => { + const { formatCodeBreakdown } = await import('../src/core/sync.ts'); + const out = formatCodeBreakdown([ + { error: 'Frontmatter slug "a" does not match path-derived slug "b"' }, + { error: 'Frontmatter slug "c" does not match path-derived slug "d"' }, + { error: 'YAML parse failed: bad' }, + ]); + expect(out).toBe(' SLUG_MISMATCH: 2\n YAML_PARSE: 1'); + }); + + test('renders pre-summarized {code, count} input directly', async () => { + const { formatCodeBreakdown } = await import('../src/core/sync.ts'); + const out = formatCodeBreakdown([ + { code: 'SLUG_MISMATCH', count: 5 }, + { code: 'YAML_PARSE', count: 2 }, + ]); + expect(out).toBe(' SLUG_MISMATCH: 5\n YAML_PARSE: 2'); + }); + + test('returns empty string for empty input', async () => { + const { formatCodeBreakdown } = await import('../src/core/sync.ts'); + expect(formatCodeBreakdown([])).toBe(''); + }); +}); diff --git a/test/sync.test.ts b/test/sync.test.ts index a8f7a8a2b..33a61c924 100644 --- a/test/sync.test.ts +++ b/test/sync.test.ts @@ -1,10 +1,11 @@ -import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test'; import { buildSyncManifest, isSyncable, pathToSlug } from '../src/core/sync.ts'; import { mkdtempSync, writeFileSync, rmSync, mkdirSync } from 'fs'; import { join } from 'path'; import { execSync } from 'child_process'; import { tmpdir } from 'os'; import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; describe('buildSyncManifest', () => { test('parses A/M/D entries from single commit', () => { @@ -204,11 +205,20 @@ describe('performSync dry-run never writes', () => { let engine: PGLiteEngine; let repoPath: string; - beforeEach(async () => { + // One PGLite per file — beforeEach wipes data only. Each test still gets a + // fresh git repo via mkdtempSync, but skips the ~20s PGLite cold-start. + beforeAll(async () => { engine = new PGLiteEngine(); await engine.connect({}); await engine.initSchema(); + }); + afterAll(async () => { + await engine.disconnect(); + }); + + beforeEach(async () => { + await resetPgliteState(engine); repoPath = mkdtempSync(join(tmpdir(), 'gbrain-sync-dryrun-')); execSync('git init', { cwd: repoPath, stdio: 'pipe' }); execSync('git config user.email "test@test.com"', { cwd: repoPath, stdio: 'pipe' }); @@ -233,8 +243,7 @@ describe('performSync dry-run never writes', () => { execSync('git add -A && git commit -m "initial"', { cwd: repoPath, stdio: 'pipe' }); }); - afterEach(async () => { - await engine.disconnect(); + afterEach(() => { if (repoPath) rmSync(repoPath, { recursive: true, force: true }); });