From 69472c24e7e15d1397430acdc62ea50fbd9b1d76 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Mon, 10 Aug 2026 22:59:53 +0900 Subject: [PATCH] fix(jobs): embed --dry-run --background embedded for real (#3594 class) (#3956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(jobs): embed --dry-run --background embedded for real `runEmbed` serializes the flag into the job payload (`dryRun: cleanArgs.includes('--dry-run')`), but the registered `embed` worker handler never read it back — it forwards slug/slugs/all/stale/sourceId/pace and onProgress, and nothing else. So `runEmbedCore` ran without `dryRun`: a backgrounded preview called the embedding provider and wrote vectors. API spend and NULL->vector mutation from an invocation whose entire purpose was neither. One line: the handler now passes `dryRun: !!job.data.dryRun`. Fourth instance of the class in #3594, which lists three (jobs prune, unify-types, sync) and says "filing the class, because fixing them one at a time will not stop the fourth". The shape here is a fourth variant: the guard is neither late nor defaulted wrong — it simply is not wired to the flag the CLI already sends. The test mirrors test/jobs-unify-types-default-dryrun.test.ts (the #1575 fix in that same issue) and asserts the side effect rather than the return value, which is #3594's stated reason this class escapes tests. It seeds real stale chunks first and asserts countStaleChunks > 0, so the dry-run path cannot short-circuit and pass the write assertion vacuously. Two independent signals, so it bites in either environment. Without embedding credentials a real run throws EmbeddingCredentialError from the preflight, so reaching the assertion at all proves the dry-run branch was taken — a preview has no business needing an API key. With credentials the run would succeed and write, and the chunk count catches it. Verification on 130d321d (v0.42.76.0): - bun test jobs-embed-background-dryrun + jobs-unify-types-default-dryrun + jobs-nice-flag -> 7 pass / 0 fail - red check: with upstream/master's jobs.ts -> 0 pass / 1 fail - bun run typecheck -> clean - bun run verify -> 34/34 green Co-Authored-By: Claude Opus 5 * fix(jobs): report what the embed job did, and make the guard hermetic Codex, both verified: - The handler returned a constant `{ embedded: true }`, so after the dryRun fix a dry run reported that it embedded — the same lie in miniature, visible through `gbrain jobs get`. It now returns the real counts. `embedded` keeps its key and stays truthy on a real run (it is the count now, 0 on a dry run). - The guard was only sound where no embedding provider is configured: runEmbedCore catches provider failures into `failures` and writes nothing, so an unfixed handler pointed at a broken provider would also leave the write count unchanged and pass. A mocked transport with a call counter closes that window — a real run must reach embedBatch, a dry run must not. Verification on 130d321d: typecheck clean, verify 34/34, focused jobs+embed tests 36 pass / 0 fail, and still 0 pass / 1 fail against upstream/master's jobs.ts. Co-Authored-By: Claude Opus 5 * test(jobs): rename to *.serial.test.ts — mock.module needs it check:test-isolation rule R2: mock.module() leaks across files inside a shard process, so a file using it must be serial. The hermetic embed transport added in the previous commit introduced one; embed.serial.test.ts is serial for the same reason. Caught by `bun run verify`, which I ran after pushing rather than before — same sequencing mistake as earlier on this branch. Gates now, all green: typecheck clean, verify 34/34, guard 1 pass / 0 fail, and 0 pass / 1 fail against upstream/master's jobs.ts. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- src/commands/jobs.ts | 18 ++- ...obs-embed-background-dryrun.serial.test.ts | 123 ++++++++++++++++++ 2 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 test/jobs-embed-background-dryrun.serial.test.ts diff --git a/src/commands/jobs.ts b/src/commands/jobs.ts index b0811fc08..9e89f92b4 100644 --- a/src/commands/jobs.ts +++ b/src/commands/jobs.ts @@ -1530,11 +1530,16 @@ export async function registerBuiltinHandlers( // readable via `gbrain jobs get `). Stderr from the worker daemon // only emits coarse job-start / job-done lines; per-page detail lives // in the DB. Per Codex review #20. - await runEmbedCore(engine, { + const embedResult = await runEmbedCore(engine, { slug: typeof job.data.slug === 'string' ? job.data.slug : undefined, slugs: Array.isArray(job.data.slugs) ? (job.data.slugs as string[]) : undefined, all: !!job.data.all, stale: job.data.all ? false : (job.data.stale !== false), + // `embed --background` serializes dryRun into the payload (embed.ts's + // job-args builder). Not reading it back here meant a backgrounded + // preview embedded for real: API spend and NULL->vector writes from an + // invocation whose whole point was to do neither. + dryRun: !!job.data.dryRun, sourceId: typeof job.data.sourceId === 'string' ? job.data.sourceId : undefined, // CX1+CX5: pace overrides ride in the job payload as explicit overrides // only; runEmbedCore re-resolves env > config > bundle at execution so @@ -1553,7 +1558,16 @@ export async function registerBuiltinHandlers( job.updateProgress({ done, total, embedded, phase: 'embed.pages' }).catch(() => {}); }, }); - return { embedded: true }; + // Report what happened, not a constant. `embedded: true` claimed a dry run + // had embedded, which is the same lie in miniature: `gbrain jobs get` + // showed it. `embedded` stays the key it always was and stays truthy on a + // real run (it is now the count, 0 on a dry run). + return { + embedded: embedResult.embedded, + dry_run: !!embedResult.dryRun, + would_embed: embedResult.would_embed, + failures: embedResult.failures, + }; }); worker.register('lint', async (job) => { diff --git a/test/jobs-embed-background-dryrun.serial.test.ts b/test/jobs-embed-background-dryrun.serial.test.ts new file mode 100644 index 000000000..86740fa82 --- /dev/null +++ b/test/jobs-embed-background-dryrun.serial.test.ts @@ -0,0 +1,123 @@ +/** + * `gbrain embed --stale --dry-run --background` ran for real. + * + * The CLI serializes the flag into the job payload (`dryRun: + * cleanArgs.includes('--dry-run')` in embed.ts's job-args builder), but the + * registered `embed` worker handler never read it back, so `runEmbedCore` was + * invoked without `dryRun`. A backgrounded preview therefore called the + * embedding provider and wrote vectors — API spend and NULL→vector mutation + * from an invocation whose whole point was to do neither. + * + * Fourth instance of the class in #3594 ("fixing them one at a time will not + * stop the fourth"), and a different shape from the three listed there: the + * guard is neither late nor defaulted wrong, it is simply not wired to the + * flag the CLI already sends. Same test shape as + * jobs-unify-types-default-dryrun.test.ts (#1575). + * + * Behavioral pin: a job whose data carries dryRun must not embed or write. + */ +import { describe, it, expect, beforeAll, afterAll, mock } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; + +/** + * Hermetic transport. Without it the guard is only sound where no embedding + * provider is configured: runEmbedCore catches provider failures into + * `failures` and writes nothing, so an unfixed handler pointed at a broken + * provider would also leave the count unchanged and pass. Counting calls + * removes that window — a real run must call embedBatch, a dry run must not. + */ +let embedCalls = 0; +mock.module('../src/core/embedding.ts', () => ({ + embedBatch: async (texts: string[]) => { + embedCalls++; + return texts.map(() => new Float32Array(1536)); + }, + currentEmbeddingSignature: () => 'test:model:1536', +})); +import { MinionWorker } from '../src/core/minions/worker.ts'; +import { registerBuiltinHandlers } from '../src/commands/jobs.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +async function embedHandler() { + const worker = new MinionWorker(engine, { concurrency: 1 }); + await registerBuiltinHandlers(worker, engine); + const handler = (worker as unknown as { + handlers: Map Promise>; + }).handlers.get('embed'); + if (!handler) throw new Error('embed handler not registered'); + return handler; +} + +/** Chunks that exist and carry no embedding — the input a real run consumes. */ +async function seedStalePage(slug: string): Promise { + await engine.putPage(slug, { + title: slug, + type: 'note' as never, + compiled_truth: 'body long enough to chunk and to survive any contentless backstop guard', + timeline: '', + frontmatter: {}, + source_path: `${slug}.md`, + }); + // putPage alone leaves no chunks, and countStaleChunks would then return 0 — + // the dry-run path would short-circuit and the write assertion below would + // hold vacuously. Write the chunks directly, with no embedding, so the run + // has real work to skip. + await engine.upsertChunks(slug, [ + { chunk_index: 0, chunk_text: 'first chunk of the page', chunk_source: 'compiled_truth' }, + { chunk_index: 1, chunk_text: 'second chunk of the page', chunk_source: 'compiled_truth' }, + ]); +} + +async function staleChunkCount(): Promise { + return await engine.countStaleChunks({}); +} + +async function embeddedChunkCount(): Promise { + const rows = (await engine.executeRaw( + 'SELECT count(*)::int AS n FROM content_chunks WHERE embedding IS NOT NULL', + )) as Array<{ n: number }>; + return rows[0]?.n ?? 0; +} + +describe('embed worker honours dryRun from the job payload (#3594 class)', () => { + it('a dryRun job embeds nothing', async () => { + embedCalls = 0; + await seedStalePage('bg-dryrun-check'); + // Coverage: without real stale chunks the dry-run path short-circuits and + // every assertion below would hold for the wrong reason. + expect(await staleChunkCount()).toBeGreaterThan(0); + const before = await embeddedChunkCount(); + + const handler = await embedHandler(); + // Exactly what `embed --stale --dry-run --background` queues. + const result = (await handler({ + id: 1, + data: { stale: true, dryRun: true }, + updateProgress: async () => {}, + })) as { embedded: number; dry_run: boolean; would_embed: number }; + + // The job result must not claim work it did not do: `gbrain jobs get` + // showed `embedded: true` for a dry run. + expect(result.dry_run).toBe(true); + expect(result.embedded).toBe(0); + expect(result.would_embed).toBeGreaterThan(0); + + // The call counter is the primary signal: a real run must reach the + // transport, a dry run must not. The write count is the secondary one — + // #3594's point is that a dry-run test asserting only the return value + // passes while the side effect happens underneath. + expect(embedCalls).toBe(0); + expect(await embeddedChunkCount()).toBe(before); + }, 60_000); +});