From 0c66715f908d0395cdb488de894089f78259db9c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 23 Jul 2026 11:01:29 -0700 Subject: [PATCH] Revert "fix(propose_takes): memoize empty extractions so zero-claim pages don't re-spend every cycle (#2514)" This reverts commit 1233051a206cc43f9106cd4a05e464fcfdfc6035. --- src/core/cycle/propose-takes.ts | 88 +----------------------- test/propose-takes.test.ts | 117 +------------------------------- 2 files changed, 3 insertions(+), 202 deletions(-) diff --git a/src/core/cycle/propose-takes.ts b/src/core/cycle/propose-takes.ts index f1a7b0e61..63ada141e 100644 --- a/src/core/cycle/propose-takes.ts +++ b/src/core/cycle/propose-takes.ts @@ -55,17 +55,6 @@ import type { PhaseStatus, CyclePhase } from '../cycle.ts'; */ export const PROPOSE_TAKES_PROMPT_VERSION = 'v0.36.1.0-tuned-cat15'; -/** - * Sentinel claim_text for the tombstone row written when a page extracts - * ZERO gradeable claims. Without a tombstone the idempotency tuple is never - * recorded, so every cycle re-spends an LLM call on unchanged zero-claim - * prose — the "unchanged page never re-spends tokens" contract only held - * for pages that produced >=1 claim. The tombstone is inserted with - * status='rejected' so no pending-review query surfaces it as a live - * proposal; its only job is to make the next cycle a cache hit. - */ -export const EMPTY_EXTRACTION_TOMBSTONE_TEXT = '(no gradeable claims)'; - /** * Tuned extractor prompt, validated against the hand-labeled synthetic * corpus at test/fixtures/calibration/. Measured F1 on first live run @@ -163,8 +152,6 @@ export interface ProposeTakesResult { cache_hits: number; cache_misses: number; proposals_inserted: number; - /** Idempotency rows written for pages that extracted zero claims. */ - tombstones_written: number; budget_exhausted: boolean; warnings: string[]; } @@ -247,43 +234,7 @@ export async function defaultExtractor( }); // ChatResult.text is already the concatenated text content. - const takes = parseExtractorOutput(result.text); - // A parse-level `[]` is AMBIGUOUS: it means either "the model genuinely - // found no gradeable claims" OR "the model returned malformed/prose/ - // truncated output we couldn't parse." The caller memoizes empty - // extractions with a tombstone, so a transient parse failure would - // PERMANENTLY suppress a page that actually has claims. Only a cleanly - // parsed empty array is a real "no claims" result worth memoizing; treat - // anything else as a transient error and throw, so the phase's catch - // retries the page next cycle (writing no tombstone). - if (takes.length === 0 && !isWellFormedEmptyExtraction(result.text)) { - throw new Error('propose_takes extractor: no parseable takes JSON (transient — retry)'); - } - return takes; -} - -/** - * True only when `raw` is a cleanly-parseable EMPTY JSON array — the - * well-behaved "no gradeable claims" response (the prompt instructs the model - * to return `[]`). Distinguishes a genuine empty extraction (safe to memoize - * via a tombstone) from malformed / prose / truncated output (transient — - * must be retried, never tombstoned). Mirrors parseExtractorOutput's - * fence-strip + first-array handling so both agree on what "the model - * returned []" means. - */ -export function isWellFormedEmptyExtraction(raw: string): boolean { - if (!raw || raw.trim().length === 0) return false; - let text = raw.trim(); - const fenced = text.match(/^```(?:json)?\s*\n?([\s\S]*?)\n?```$/); - if (fenced) text = (fenced[1] ?? '').trim(); - const arrStart = text.indexOf('['); - if (arrStart === -1) return false; - try { - const parsed = JSON.parse(text.slice(arrStart)); - return Array.isArray(parsed) && parsed.length === 0; - } catch { - return false; - } + return parseExtractorOutput(result.text); } /** @@ -363,7 +314,6 @@ class ProposeTakesPhase extends BaseCyclePhase { cache_hits: 0, cache_misses: 0, proposals_inserted: 0, - tombstones_written: 0, budget_exhausted: false, warnings: [], }; @@ -465,40 +415,6 @@ class ProposeTakesPhase extends BaseCyclePhase { ); result.proposals_inserted += 1; } - - // Memoize the empty case too. A page that extracted zero claims gets - // NO row from the loop above, so without this its idempotency tuple is - // never recorded and the next cycle re-spends an LLM call on unchanged - // prose (the idle-cost bug). Write one tombstone row keyed by the same - // (source, slug, content_hash, prompt_version) tuple. status='rejected' - // keeps it out of any pending-review query; its sole purpose is to make - // the next cycle a cache hit. Only reached on a SUCCESSFUL empty extract - // — the extractor-throw path `continue`s above, so failed pages are - // retried rather than tombstoned. - if (proposals.length === 0) { - await engine.executeRaw( - `INSERT INTO take_proposals - (source_id, page_slug, content_hash, prompt_version, proposal_run_id, - claim_text, kind, holder, weight, domain, dedup_against_fence_rows, model_id, status) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, 'rejected') - ON CONFLICT (source_id, page_slug, content_hash, prompt_version) DO NOTHING`, - [ - sourceId, - page.slug, - ch, - promptVersion, - proposalRunId, - EMPTY_EXTRACTION_TOMBSTONE_TEXT, - 'fact', - 'brain', - 0, - null, - JSON.stringify(existingTakes), - opts.model ?? 'claude-sonnet-4-6', - ], - ); - result.tombstones_written += 1; - } } if (opts.reporter) opts.reporter.finish(); @@ -532,7 +448,7 @@ class ProposeTakesPhase extends BaseCyclePhase { }); return { - summary: `propose_takes: scanned ${result.pages_scanned} pages, ${result.cache_hits} cached, ${result.proposals_inserted} new proposals, ${result.tombstones_written} empty (run ${proposalRunId})`, + summary: `propose_takes: scanned ${result.pages_scanned} pages, ${result.cache_hits} cached, ${result.proposals_inserted} new proposals (run ${proposalRunId})`, details: { ...result, proposal_run_id: proposalRunId, prompt_version: promptVersion }, status: result.budget_exhausted ? 'warn' : 'ok', }; diff --git a/test/propose-takes.test.ts b/test/propose-takes.test.ts index 9b8bba307..3c0ccb68d 100644 --- a/test/propose-takes.test.ts +++ b/test/propose-takes.test.ts @@ -21,9 +21,7 @@ import { contentHash, hasCompleteFence, extractExistingTakesForDedup, - isWellFormedEmptyExtraction, PROPOSE_TAKES_PROMPT_VERSION, - EMPTY_EXTRACTION_TOMBSTONE_TEXT, type ProposeTakesExtractor, type ProposedTake, } from '../src/core/cycle/propose-takes.ts'; @@ -61,15 +59,7 @@ function buildMockEngine(opts: { if (existing.has(key)) return [{ id: 1 } as unknown as T]; return []; } - // INSERT into take_proposals — persist the idempotency key so a - // subsequent cycle observes a cache hit, mirroring the real unique - // index on (source_id, page_slug, content_hash, prompt_version). - if (sql.includes('INSERT INTO take_proposals')) { - const [sourceId, slug, ch, pv] = params ?? []; - existing.add(`${sourceId}|${slug}|${ch}|${pv}`); - return []; - } - // Other writes — return nothing. + // INSERT — return nothing return []; }, } as unknown as BrainEngine; @@ -172,52 +162,6 @@ describe('parseExtractorOutput', () => { }); }); -// ─── isWellFormedEmptyExtraction ──────────────────────────────────── -// Guards the tombstone against permanently memoizing a transient parse -// failure as "no claims". Only a cleanly-parsed empty array counts as a -// genuine empty extraction; malformed/prose/truncated output must not. - -describe('isWellFormedEmptyExtraction', () => { - test('true for a clean empty array (the well-behaved "no claims" response)', () => { - expect(isWellFormedEmptyExtraction('[]')).toBe(true); - expect(isWellFormedEmptyExtraction(' [] ')).toBe(true); - expect(isWellFormedEmptyExtraction('[ ]')).toBe(true); - }); - - test('true for a fenced empty array', () => { - expect(isWellFormedEmptyExtraction('```json\n[]\n```')).toBe(true); - }); - - test('true for leading prose then an empty array', () => { - expect(isWellFormedEmptyExtraction('No gradeable claims.\n\n[]')).toBe(true); - }); - - test('false for empty / whitespace output (transient, must retry)', () => { - expect(isWellFormedEmptyExtraction('')).toBe(false); - expect(isWellFormedEmptyExtraction(' \n ')).toBe(false); - }); - - test('false for prose-only / non-JSON output (transient, must retry)', () => { - expect(isWellFormedEmptyExtraction('There are no gradeable claims here.')).toBe(false); - expect(isWellFormedEmptyExtraction('null')).toBe(false); - }); - - test('false for malformed / truncated JSON (transient, must retry)', () => { - expect(isWellFormedEmptyExtraction('[')).toBe(false); - expect(isWellFormedEmptyExtraction('[{"claim_text":"x"')).toBe(false); - }); - - test('false for a NON-empty array (has content — not an empty extraction)', () => { - expect(isWellFormedEmptyExtraction('[{"claim_text":"x","kind":"take","holder":"brain","weight":0.5}]')).toBe(false); - // Parseable but claim-less array is ambiguous garbage → not a genuine empty. - expect(isWellFormedEmptyExtraction('[{"foo":"bar"}]')).toBe(false); - }); - - test('false for an empty object (model ignored the array-format instruction)', () => { - expect(isWellFormedEmptyExtraction('{}')).toBe(false); - }); -}); - // ─── contentHash ──────────────────────────────────────────────────── describe('contentHash', () => { @@ -492,62 +436,3 @@ New prose appended here.`; } }); }); - -// ─── Empty-extraction memoization (idle-cost fix) ─────────────────── -// A page that yields zero gradeable claims must still record an -// idempotency row, or every cycle re-spends an LLM call on unchanged -// prose. Regression guard for the "empty result never memoized" bug. - -describe('runPhaseProposeTakes — empty extraction memoization', () => { - test('zero-claim page writes a tombstone row (proposals_inserted stays 0)', async () => { - const pages = [buildPage({ slug: 'test/embed-probe', body: '# probe\njust a test, nothing to grade.' })]; - const { engine, captured } = buildMockEngine({ pages }); - const extractor: ProposeTakesExtractor = async () => []; - const result = await runPhaseProposeTakes(buildCtx(engine), { extractor }); - - const details = result.details as Record; - expect(details.cache_misses).toBe(1); - expect(details.proposals_inserted).toBe(0); - expect(details.tombstones_written).toBe(1); - - const inserts = captured.filter(c => c.sql.includes('INSERT INTO take_proposals')); - expect(inserts).toHaveLength(1); - // Tombstone carries the sentinel claim_text and an out-of-queue status. - expect(inserts[0]!.params[5]).toBe(EMPTY_EXTRACTION_TOMBSTONE_TEXT); // claim_text - expect(inserts[0]!.sql).toContain("'rejected'"); - }); - - test('unchanged zero-claim page is a cache hit next cycle (no repeat LLM call)', async () => { - const pages = [buildPage({ slug: 'test/embed-probe', body: '# probe\njust a test, nothing to grade.' })]; - const { engine } = buildMockEngine({ pages }); - let extractorCalls = 0; - const extractor: ProposeTakesExtractor = async () => { - extractorCalls++; - return []; - }; - - // Cycle 1: cache miss → LLM call → tombstone written. - const r1 = await runPhaseProposeTakes(buildCtx(engine), { extractor }); - expect(extractorCalls).toBe(1); - expect((r1.details as Record).cache_misses).toBe(1); - expect((r1.details as Record).tombstones_written).toBe(1); - - // Cycle 2: same unchanged page → cache hit → extractor NOT called again. - const r2 = await runPhaseProposeTakes(buildCtx(engine), { extractor }); - expect(extractorCalls).toBe(1); // the whole point: no re-spend - expect((r2.details as Record).cache_hits).toBe(1); - expect((r2.details as Record).cache_misses).toBe(0); - }); - - test('extractor error does NOT write a tombstone (page retried next cycle)', async () => { - const pages = [buildPage({ slug: 'wiki/x', body: 'some prose' })]; - const { engine, captured } = buildMockEngine({ pages }); - const extractor: ProposeTakesExtractor = async () => { - throw new Error('LLM timeout'); - }; - const result = await runPhaseProposeTakes(buildCtx(engine), { extractor }); - - expect((result.details as Record).tombstones_written).toBe(0); - expect(captured.filter(c => c.sql.includes('INSERT INTO take_proposals'))).toHaveLength(0); - }); -});