From 63c2de712831cf2440c4dc12714be919043fa4a6 Mon Sep 17 00:00:00 2001 From: dtzp555-max Date: Wed, 15 Jul 2026 21:40:13 +1000 Subject: [PATCH] fix(tui): clamp stream holdback to safe floor (A1) + record cc_entrypoint before honesty gates (A3) (#164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two OCP-internal correctness fixes on the TUI streaming/observation path, surfaced by an independent (Codex) re-review. Neither touches the cli.js wire. A1 — OCP_TUI_STREAM_HOLDBACK now has an enforced floor (DEFAULT_HOLDBACK_CHARS=100). The C-1 auth-banner gate's first-message guarantee rests on the holdback being at least the default banner detector's 100-char reach. The env var's own doc said "Only raise it", but the code trusted the operator: a sub-floor value (e.g. 50) or a NaN typo ("unlimited") let the first chars of a real auth banner stream to the client before the end-of-turn detector could classify the whole message and reject the turn. resolveStreamHoldback() clamps UP to the floor and returns {value, clamped}; server.mjs emits a boot WARNING when it had to clamp. Default (unset) is unchanged and unflagged. A3 — recordTuiEntrypoint() now runs the moment runTuiTurn() returns, BEFORE the honesty gates (wall-clock truncation / auth banner / stream divergence) that throw. The entrypoint (cli vs sdk-cli) is which billing pool the turn consumed; a turn that then fails a gate STILL spent that pool, and those failed turns are exactly the ones most likely to signal a silent degrade to the metered Agent SDK pool. The old placement recorded only on the success path, so /health's lastEntrypoint and entrypointMismatches were blind to every failed turn — the billing-drift signal missed the cases it most needed to catch. recordModelSuccess stays on the success path. The catch block does not record the entrypoint, so there is no double count; a client-disconnect (TuiAbortError) throws from inside runTuiTurn before the destructure, so no phantom entrypoint is recorded. ALIGNMENT.md Rule 2 (No Invention): no cli.js citation applies. cli.js does not perform either operation — both are proxy-internal. A1 hardens OCP's own SSE holdback (a safety mechanism on the Class B.1 OpenAI-compat streaming surface; wire format authority is the OpenAI spec via ADR 0006). A3 reorders when OCP records its own /health observability counters (Class B.2, grandfathered under ADR 0006; the TUI spawn authority is ADR 0007). No endpoint, header, request field, or response field is added or altered; the bytes to and from cli.js are byte-identical. This is observation/safety-layer hardening, not extension. Tests: +5 mutation-proven unit tests for resolveStreamHoldback (deleting the floor clamp fails 3 of them). Full suite 325 passed / 0 failed (was 320). A3 is a server.mjs control-flow reorder; server.mjs is not imported by the test suite, so A3 is verified by reviewer inspection of the diff, stated honestly here rather than vouched for by a test. Version bump + CHANGELOG deliberately omitted: this is a fix PR, consolidated into a later chore(release) PR per the repo's #148/#149/#150 -> #151 (v3.21.1) convention. Co-authored-by: dtzp555 Co-authored-by: Claude --- lib/tui/stream.mjs | 15 +++++++++++++++ server.mjs | 42 +++++++++++++++++++++++++++++------------- test-features.mjs | 38 +++++++++++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 14 deletions(-) diff --git a/lib/tui/stream.mjs b/lib/tui/stream.mjs index 797f6dc..7a4cbf0 100644 --- a/lib/tui/stream.mjs +++ b/lib/tui/stream.mjs @@ -45,6 +45,21 @@ import { detectTuiUpstreamError } from "./transcript.mjs"; // Default holdback before the first byte is released to the client. See TuiDeltaAssembler. export const DEFAULT_HOLDBACK_CHARS = 100; +// Resolve OCP_TUI_STREAM_HOLDBACK to a SAFE value. The whole C-1 auth-banner guarantee rests +// on the holdback being at least the default banner detector's max message length — which is +// exactly DEFAULT_HOLDBACK_CHARS. So this is a FLOOR, not a hint: a smaller value (or a NaN +// typo like "unlimited"/"5MB") would let a real banner fragment release before the terminal +// detector could classify the whole message, silently reopening the leak the assembler exists +// to prevent. The env var's own doc says "Only raise it"; this enforces that instead of trusting +// it. Returns { value, clamped } so the caller can warn when it had to clamp — a silent floor is +// less honest than a noticed one. +export function resolveStreamHoldback(raw, floor = DEFAULT_HOLDBACK_CHARS) { + const parsed = parseInt(raw ?? "", 10); + if (!Number.isFinite(parsed)) return { value: floor, clamped: raw != null && String(raw).trim() !== "" }; + if (parsed < floor) return { value: floor, clamped: true }; + return { value: parsed, clamped: false }; +} + // The hook script. POSIX sh, no interpreter startup beyond /bin/sh, one fork (`cat`). // // - `printf` is a shell BUILTIN in sh/dash/bash, so the newline costs no fork. diff --git a/server.mjs b/server.mjs index 179f317..8baf469 100644 --- a/server.mjs +++ b/server.mjs @@ -47,7 +47,7 @@ import { runTuiTurn, reapStaleTuiSessions, resolveTuiHome, bootTuiPane, tuiPaneH import { detectTuiUpstreamError } from "./lib/tui/transcript.mjs"; import { TuiSemaphore, SemaphoreAbortError, recordTuiEntrypoint, buildTuiHealthBlock } from "./lib/tui/semaphore.mjs"; import { TuiPanePool, resolvePoolSize, POOL_MAX_SIZE } from "./lib/tui/pool.mjs"; -import { TuiDeltaAssembler, DEFAULT_HOLDBACK_CHARS } from "./lib/tui/stream.mjs"; +import { TuiDeltaAssembler, DEFAULT_HOLDBACK_CHARS, resolveStreamHoldback } from "./lib/tui/stream.mjs"; import { createSerialMutex, createTtlCache, isTokenExpiring, orderLabelsLastGoodFirst } from "./lib/spawn-auth.mjs"; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -379,7 +379,20 @@ const TUI_STREAM_DIR = process.env.OCP_TUI_STREAM_DIR || `${process.env.HOME}/.o // exceeds this, which puts it out of the default banner detector's <=100-char reach — the // FIRST of the two halves of the guarantee (see the assembler's class comment for the second: // no further emission at all once a message boundary follows an emit). Only raise it. -const TUI_STREAM_HOLDBACK = parseInt(process.env.OCP_TUI_STREAM_HOLDBACK || String(DEFAULT_HOLDBACK_CHARS), 10); +// resolveStreamHoldback enforces the DEFAULT_HOLDBACK_CHARS floor: the "Only raise it" comment +// above is now load-bearing, not advisory. A sub-floor value (or garbage) is clamped UP to the +// floor and reported via `_holdback.clamped`, because a holdback below the default banner +// detector's 100-char reach would let the first chars of a real auth banner stream before the +// end-of-turn gate rejects the turn (the A1 leak). We can only ever raise the guarantee, never +// weaken it below the detector's bound. +const _holdback = resolveStreamHoldback(process.env.OCP_TUI_STREAM_HOLDBACK); +const TUI_STREAM_HOLDBACK = _holdback.value; +if (TUI_MODE && TUI_STREAM && _holdback.clamped) { + console.error( + `[tui] WARNING: OCP_TUI_STREAM_HOLDBACK=${JSON.stringify(process.env.OCP_TUI_STREAM_HOLDBACK)} is below the\n` + + ` safe floor (${DEFAULT_HOLDBACK_CHARS}) or not a number; clamped up to ${DEFAULT_HOLDBACK_CHARS}. The holdback can only be raised.` + ); +} if (TUI_MODE && TUI_STREAM && process.env.CLAUDE_TUI_ERROR_PATTERNS != null && TUI_STREAM_HOLDBACK <= DEFAULT_HOLDBACK_CHARS) { // The holdback's FIRST-MESSAGE half (see TuiDeltaAssembler) is sound for the DEFAULT // auth-banner detector (which cannot match a message longer than 100 chars). An @@ -1494,6 +1507,17 @@ async function callClaudeTui(model, messages, _conversationId, _keyName, res, st streamDir: TUI_STREAM ? TUI_STREAM_DIR : null, abortSignal: streamCtx ? streamCtx.signal : null, }); + // ── Billing-pool observation (issue #115, #133) — A3 fix: record the entrypoint the moment + // runTuiTurn returns, BEFORE the honesty gates below that can throw. The entrypoint (cli vs + // sdk-cli) is which BILLING POOL the turn consumed; a turn that then fails a gate (wall-clock + // truncation, auth banner, stream divergence) STILL spent that pool — and those failed turns + // are exactly the ones most likely to signal a silent degrade to the metered Agent SDK pool. + // Recording only on the success path (the old placement) blinded /health's entrypointMismatches + // and lastEntrypoint to every failed turn. recordModelSuccess still runs later, only on success. + if (recordTuiEntrypoint(tuiStats, entrypoint, TUI_ENTRYPOINT)) { + logEvent("warn", "tui_entrypoint_mismatch", { expected: "cli", got: entrypoint, model: cliModel }); + } + // ── Honesty gates (issue #133) ─ run BEFORE recordModelSuccess / cache write-back. // A throw here propagates to the catch below (recordModelError + reject), so the // result never reaches the downstream setCachedResponse / singleflight / SUCCESS path. @@ -1571,17 +1595,9 @@ async function callClaudeTui(model, messages, _conversationId, _keyName, res, st } recordModelSuccess(cliModel, 0); // elapsed not measurable here; wallclock at reader level - // Assert the subscription-pool classification. TUI exists to keep cc_entrypoint=cli - // (subscription pool); a silent degrade to sdk-cli (metered Agent SDK pool) would still - // return text but cost money — warn loudly so it's visible. (issue #115) - // C-5: also surface the observation on /health. recordTuiEntrypoint sets lastEntrypoint - // unconditionally (operators can poll it to confirm cli) and increments - // entrypointMismatches when expected=cli but observed≠cli — the same condition the - // journald warning already covers — so a silent metered-pool drift is visible on /health - // without tailing logs. - if (recordTuiEntrypoint(tuiStats, entrypoint, TUI_ENTRYPOINT)) { - logEvent("warn", "tui_entrypoint_mismatch", { expected: "cli", got: entrypoint, model: cliModel }); - } + // Entrypoint/billing-pool observation was already recorded above, right after runTuiTurn + // returned — see the A3-fix comment there (it must cover failed turns too, so it cannot live + // on this success-only path). return text; } catch (err) { // A mid-turn client disconnect (streaming path only — abortSignal) is NOT an upstream diff --git a/test-features.mjs b/test-features.mjs index b85529c..c4c2f72 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -3453,7 +3453,7 @@ async function runAsyncTests() { // ── TUI real streaming: MessageDisplay hook sink (backlog #2) ─────────────── // Pure-logic coverage for lib/tui/stream.mjs: sink parsing, the concat===T assertion, // prefix-stability, the auth-banner holdback, message scoping, and the error paths. -import { TuiDeltaAssembler, parseDeltaChunk, buildStreamSettings, streamFilePath, HOOK_SCRIPT, prepareStreamHook } from "./lib/tui/stream.mjs"; +import { TuiDeltaAssembler, parseDeltaChunk, buildStreamSettings, streamFilePath, HOOK_SCRIPT, prepareStreamHook, resolveStreamHoldback, DEFAULT_HOLDBACK_CHARS } from "./lib/tui/stream.mjs"; test("stream: parseDeltaChunk consumes only COMPLETE lines (a torn write stays unread)", () => { const p = (i, d, final = false) => JSON.stringify({ hook_event_name: "MessageDisplay", session_id: "s", message_id: "m", index: i, final, delta: d }); @@ -3521,6 +3521,42 @@ test("stream: holdback releases once past the detector's reach, and only then", assert.equal(a.push(mdFire(2, "tail")), "tail", "subsequent deltas stream straight through"); }); +// ── resolveStreamHoldback: the FLOOR under OCP_TUI_STREAM_HOLDBACK (A1 fix) ──────────── +// The C-1 auth-banner guarantee holds only while the holdback >= the default detector's +// 100-char reach. These tests pin that the resolver CLAMPS UP to the floor. They are +// mutation-proof: delete the `parsed < floor` branch and the sub-floor cases below fail +// (a 50 would pass straight through, reopening the leak). The clamped flag drives the boot +// warning in server.mjs, so its truthiness is asserted alongside every value. +test("holdback: a sub-floor value is clamped UP to the floor and flagged", () => { + assert.deepEqual(resolveStreamHoldback("50"), { value: DEFAULT_HOLDBACK_CHARS, clamped: true }); + assert.deepEqual(resolveStreamHoldback("0"), { value: DEFAULT_HOLDBACK_CHARS, clamped: true }); + assert.deepEqual(resolveStreamHoldback("-5"), { value: DEFAULT_HOLDBACK_CHARS, clamped: true }); + assert.deepEqual(resolveStreamHoldback("99"), { value: DEFAULT_HOLDBACK_CHARS, clamped: true }); +}); + +test("holdback: garbage / NaN falls back to the floor and is flagged (not silently 0)", () => { + assert.deepEqual(resolveStreamHoldback("unlimited"), { value: DEFAULT_HOLDBACK_CHARS, clamped: true }); + assert.deepEqual(resolveStreamHoldback("5MB"), { value: DEFAULT_HOLDBACK_CHARS, clamped: true }); +}); + +test("holdback: an above-floor value passes through unchanged and is NOT flagged", () => { + assert.deepEqual(resolveStreamHoldback("200"), { value: 200, clamped: false }); + assert.deepEqual(resolveStreamHoldback("101"), { value: 101, clamped: false }); + assert.deepEqual(resolveStreamHoldback(String(DEFAULT_HOLDBACK_CHARS)), { value: DEFAULT_HOLDBACK_CHARS, clamped: false }); +}); + +test("holdback: an unset env var takes the floor WITHOUT flagging (no spurious boot warning)", () => { + assert.deepEqual(resolveStreamHoldback(undefined), { value: DEFAULT_HOLDBACK_CHARS, clamped: false }); + assert.deepEqual(resolveStreamHoldback(null), { value: DEFAULT_HOLDBACK_CHARS, clamped: false }); + assert.deepEqual(resolveStreamHoldback(""), { value: DEFAULT_HOLDBACK_CHARS, clamped: false }); + assert.deepEqual(resolveStreamHoldback(" "), { value: DEFAULT_HOLDBACK_CHARS, clamped: false }); +}); + +test("holdback: the floor is a parameter, so a deployment can raise (never lower) it", () => { + assert.deepEqual(resolveStreamHoldback("150", 200), { value: 200, clamped: true }, "custom floor still clamps up"); + assert.deepEqual(resolveStreamHoldback("300", 200), { value: 300, clamped: false }); +}); + test("stream: a short answer never passes the holdback and is delivered whole at terminal", () => { const T = "The capital of France is Paris."; const a = new TuiDeltaAssembler();