fix(jobs): embed --dry-run --background embedded for real (#3594 class) (#3956)

* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-08-14 00:48:18 +00:00 · 2026-08-10 20:59:53 +07:00
parent ea17f5911c
commit 69472c24e7
2 changed files with 139 additions and 2 deletions
@@ -1530,11 +1530,16 @@ export async function registerBuiltinHandlers(
    // readable via `gbrain jobs get <id>`). 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) => {
@@ -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<string, (j: unknown) => Promise<unknown>>;
+  }).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<void> {
+  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<number> {
+  return await engine.countStaleChunks({});
+}
+
+async function embeddedChunkCount(): Promise<number> {
+  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);
+});