From 4d0d3732840c8ef053a373185bbcdf0734853d65 Mon Sep 17 00:00:00 2001 From: Ed Heltzel <402910+edheltzel@users.noreply.github.com> Date: Thu, 25 Jun 2026 13:35:03 -0400 Subject: [PATCH] fix(consolidate): floor NULL demote target instead of nulling importance (#188) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The child re-clamp used MAX(1, MIN(importance, CAST(? AS INTEGER))), which writes NULL when the payload importance is NULL (CAST(NULL)->NULL, MIN/MAX with NULL->NULL) — so the comment's claimed floor-of-1 'regardless of the supplied value' did not actually hold for NULL. Wrap the bind in COALESCE(?, importance) so a NULL payload no-ops to the current value and MAX(1, ...) re-pins the floor for every input. Tighten the comment to match the real guarantee. Tests: prove a NULL newImportance no-ops (never nulls the column), and add the missing end-to-end spawn -> stderr -> parseConsolidateProgress link (child emits a progress marker then hangs past a short timeout; the parser recovers the committed counts from the killed child's captured stderr). Closes #188 --- hooks/lib/consolidate-core.ts | 9 +++---- tests/hooks/consolidate-core.test.ts | 16 +++++++++++++ tests/lib/consolidate.test.ts | 36 ++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/hooks/lib/consolidate-core.ts b/hooks/lib/consolidate-core.ts index b1980e3..2b4a0ff 100644 --- a/hooks/lib/consolidate-core.ts +++ b/hooks/lib/consolidate-core.ts @@ -185,13 +185,14 @@ export async function applyConsolidation( ]; // 4) Write + demote atomically. Sources are demoted, never deleted. - // Re-clamp in the child (defense-in-depth): MIN(importance, …) so a demote - // can only ever LOWER importance — a directly-piped payload can't raise it — - // and MAX(1, …) re-pins the absolute floor regardless of the supplied value. + // Re-clamp in the child (defense-in-depth): MIN(…, importance) so a demote can + // only ever LOWER importance — a directly-piped payload can't raise it. + // COALESCE(?, importance) makes a NULL payload a no-op (never nulls the column), + // and MAX(1, …) re-pins the absolute floor of 1 for every input, NULL included. const writeCluster = db.transaction(() => { insertLoa.run(...values); const demote = db.prepare( - `UPDATE ${cluster.table} SET importance = MAX(1, MIN(importance, CAST(? AS INTEGER))) WHERE id = ?`, + `UPDATE ${cluster.table} SET importance = MAX(1, MIN(CAST(COALESCE(?, importance) AS INTEGER), importance)) WHERE id = ?`, ); for (const r of cluster.records) demote.run(r.newImportance, r.id); }); diff --git a/tests/hooks/consolidate-core.test.ts b/tests/hooks/consolidate-core.test.ts index 5554f8b..42e564a 100644 --- a/tests/hooks/consolidate-core.test.ts +++ b/tests/hooks/consolidate-core.test.ts @@ -149,6 +149,22 @@ describe('applyConsolidation — child re-clamps the demote target (#145.1)', () // MAX(1, MIN(4, 0)) = 1 → never below the floor, even from a sub-floor payload. for (const id of ids) expect(importanceOf(id)).toBe(1); }); + + test('a NULL newImportance no-ops to the current value — never writes NULL (#188)', async () => { + // A NULL payload used to null the column (CAST(NULL)→NULL, MIN/MAX with NULL→NULL). + // COALESCE(?, importance) makes it a no-op so the floor-of-1 invariant holds for NULL too. + const ids = seedDecisions(['decision one', 'decision two', 'decision three'], 4); + const nullCluster: ConsolidateInputCluster = { + ...clusterFor(ids), + records: ids.map((id) => ({ id, text: `decision ${id}`, newImportance: null as unknown as number })), + }; + + const result = await applyConsolidation(dbPath, [nullCluster], { summarize }); + + expect(result.written).toBe(1); + // Unchanged at 4 (not NULL) — a non-null integer proves the column was not nulled. + for (const id of ids) expect(importanceOf(id)).toBe(4); + }); }); describe('applyConsolidation — NULL-lineage guard (#145.2)', () => { diff --git a/tests/lib/consolidate.test.ts b/tests/lib/consolidate.test.ts index c797d62..64d4a83 100644 --- a/tests/lib/consolidate.test.ts +++ b/tests/lib/consolidate.test.ts @@ -9,6 +9,10 @@ // - runConsolidate --execute: hands the plan to the injected apply seam. import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { execSync } from 'child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; import { setupTestDb, teardownTestDb } from '../helpers/setup'; import { getDb } from '../../src/db/connection'; import { addDecision, addLearning, createLoaEntry } from '../../src/lib/memory'; @@ -228,3 +232,35 @@ describe('parseConsolidateProgress', () => { expect(parseConsolidateProgress('')).toBeNull(); }); }); + +// End-to-end stderr→parser link (#188): the prior coverage proved the parser and +// the apply separately, but never the live `spawn → err.stderr → parse` path that +// spawnConsolidateChild relies on when it SIGTERMs a hung child on timeout. +describe('parseConsolidateProgress — live spawn → stderr → parse (#188)', () => { + // Mirrors PROGRESS_PREFIX emitted by hooks/lib/consolidate-core.ts's onProgress. + const FIXTURE = `process.stderr.write('RECALL_CONSOLIDATE_PROGRESS {"written":2,"demoted":5}\\n'); +setInterval(() => {}, 1000); // hang so the parent must kill us on timeout +`; + + test('recovers committed counts from a killed child\'s captured stderr', () => { + const dir = mkdtempSync(join(tmpdir(), 'recall-consolidate-e2e-')); + const script = join(dir, 'hang-after-progress.ts'); + writeFileSync(script, FIXTURE); + try { + // Same mechanism as spawnConsolidateChild: execSync with a timeout SIGTERMs + // the child, and the thrown error carries the stderr captured before the kill. + let captured: string | null = null; + try { + execSync(`${process.execPath} run ${script}`, { + encoding: 'utf-8', timeout: 1500, maxBuffer: 10 * 1024 * 1024, + }); + } catch (err: any) { + captured = String(err?.stderr ?? ''); + } + expect(captured).not.toBeNull(); // child was killed, not a clean exit + expect(parseConsolidateProgress(captured!)).toEqual({ written: 2, demoted: 5 }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +});