From 38f92d56ee6edc1f14915352c551897813c995e4 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Fri, 17 Jul 2026 18:14:51 +0200 Subject: [PATCH] fix(#407): make term-fidelity byte meta commensurable + retain per-attempt artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two harness debts from the term-fidelity program. (1) meta bytes/offset incommensurable (exact-2.0 artifact). `quiet.activity.bytes` (client-received UTF-8 bytes of broker:pty-chunk strings, from probe install) and `brokerOffset` (raw PTY bytes from worker start) were dropped into meta as a bare pair that read as an exact-2.0 "double delivery" signal in 5/5 codex bundles — proven NOT to be double delivery (headless probe: client/offset = 1.0000; one IPC send per worker_stream event). New dependency-free `byte-accounting.ts` derives a self-documenting `byteAccounting` block: each figure declares its baseline + unit, and `clientToBrokerByteRatio` is computed on a shared agent-start baseline (probe installs before spawn) so ~1.0 is the meaningful one-to-one value. An embedded `note` states a near-integer ratio is a derivation artifact, never a mechanism claim. Unit-tested (5 cases: 1.0, rounding, the 2.0 forensic case + note, null-on-no-offset, divide-by-zero guard). (2) retry-then-pass erased first-attempt artifacts. The failed first attempt is exactly the REAL divergence event we need. Divergence + telemetry bundles now write under `attempt-/` (threaded from test.info().retry) so a retry never overwrites prior-attempt data; playwright config adds `preserveOutput: 'always'` so Playwright's own error-context/trace for a failed attempt survive a later pass. README updated. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 1 + playwright.term-fidelity.config.ts | 11 +++ tests/term-fidelity/README.md | 23 ++++-- tests/term-fidelity/byte-accounting.test.ts | 66 ++++++++++++++++ tests/term-fidelity/byte-accounting.ts | 83 +++++++++++++++++++++ tests/term-fidelity/harness.ts | 9 ++- tests/term-fidelity/oracle.ts | 29 ++++++- tests/term-fidelity/term-fidelity.spec.ts | 8 +- vitest.config.mjs | 4 +- 9 files changed, 220 insertions(+), 14 deletions(-) create mode 100644 tests/term-fidelity/byte-accounting.test.ts create mode 100644 tests/term-fidelity/byte-accounting.ts diff --git a/.gitignore b/.gitignore index 14a0517b..b8a9fa78 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ node_modules/ +node_modules out/ dist/ bin/relayfile-mount diff --git a/playwright.term-fidelity.config.ts b/playwright.term-fidelity.config.ts index ad6f198d..2dc5e3fa 100644 --- a/playwright.term-fidelity.config.ts +++ b/playwright.term-fidelity.config.ts @@ -13,7 +13,18 @@ export default defineConfig({ timeout: 30_000 }, outputDir: 'test-results/term-fidelity/playwright', + // Retain every attempt's output on a retry-then-pass. A flaky first attempt is + // exactly the REAL divergence event we need to examine; `failures-only` would + // delete the whole (eventually-passing) test's dirs, including the failed + // attempt's error-context.md and trace. The harness also segregates its own + // divergence/telemetry bundles under attempt-/ (see oracle.ts) so a + // retry never overwrites the first attempt's data. + preserveOutput: 'always', use: { + // Records a trace per attempt and keeps it for any attempt that failed + // (dropped only for clean passes). On retry-then-pass the failed first + // attempt's trace is retained; combined with preserveOutput:'always' its + // error-context survives too. trace: 'retain-on-failure' }, reporter: [['list']] diff --git a/tests/term-fidelity/README.md b/tests/term-fidelity/README.md index 0df55693..4d487367 100644 --- a/tests/term-fidelity/README.md +++ b/tests/term-fidelity/README.md @@ -109,7 +109,7 @@ reconciler subsequently repairs the visible grid. A mismatched checkpoint writes: ```text -test-results/term-fidelity/// +test-results/term-fidelity///attempt-/ renderer.txt broker.txt diff.txt @@ -117,8 +117,19 @@ test-results/term-fidelity/// meta.json ``` -`meta.json` includes dimensions, cursors, timestamps, broker offset, quiet-gate -state, installed Relay package and broker versions, isolated instance details, -and reconciler telemetry observed during the workload. A telemetry-only failure -writes its screenshot and metadata under -`test-results/term-fidelity//reconciler-telemetry/`. +Bundles are segregated by Playwright attempt (`attempt-0/` is the first run, +`attempt-1/` the first retry, …) so a retry-then-pass never overwrites a real +first-attempt divergence. The config also sets `preserveOutput: 'always'` + +`trace: 'retain-on-failure'` so Playwright's own error-context and trace for a +failed attempt survive even when a later attempt passes. + +`meta.json` includes dimensions, cursors, timestamps, quiet-gate state, installed +Relay package and broker versions, isolated instance details, and reconciler +telemetry observed during the workload. Byte delivery is recorded under +`byteAccounting`, which pairs the client-received IPC bytes with the broker's raw +PTY snapshot offset **on a shared agent-start baseline** and states each figure's +baseline + unit. Its `clientToBrokerByteRatio` is ~1.0 on a faithful one-to-one +pipeline; the embedded `note` warns that a near-integer ratio (e.g. the historic +exact-2.0) is a derivation artifact, never proof of double delivery. A +telemetry-only failure writes its screenshot and metadata under +`test-results/term-fidelity//reconciler-telemetry/attempt-/`. diff --git a/tests/term-fidelity/byte-accounting.test.ts b/tests/term-fidelity/byte-accounting.test.ts new file mode 100644 index 00000000..f1722945 --- /dev/null +++ b/tests/term-fidelity/byte-accounting.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from 'vitest' +import { BYTE_ACCOUNTING_NOTE, deriveByteAccounting } from './byte-accounting' + +// Pins the client-vs-broker byte derivation used in divergence-bundle meta. +// The whole point of the module is that the exact-2.0 codex artifact can no +// longer reach meta as an unlabeled, misreadable pair — so these tests assert +// the labels, the shared-baseline ratio, and the guard rails. +describe('deriveByteAccounting', () => { + it('reports a ~1.0 ratio for one-to-one delivery on a shared baseline', () => { + const acc = deriveByteAccounting({ + clientBytesReceived: 4096, + clientChunks: 12, + snapshotOffset: 4096 + }) + expect(acc.clientToBrokerByteRatio).toBe(1) + expect(acc.commensurable).toBe(true) + expect(acc.snapshotOffset).toBe(4096) + }) + + it('rounds the ratio to 4 dp', () => { + const acc = deriveByteAccounting({ + clientBytesReceived: 1000, + clientChunks: 3, + snapshotOffset: 3000 + }) + expect(acc.clientToBrokerByteRatio).toBe(0.3333) + }) + + it('surfaces (does not hide) the historic exact-2.0 reading, with the do-not-misread note', () => { + const acc = deriveByteAccounting({ + clientBytesReceived: 8192, + clientChunks: 20, + snapshotOffset: 4096 + }) + // The number is preserved for forensics — but it is explicitly labeled and + // carries the note so it can never again be read as "double delivery". + expect(acc.clientToBrokerByteRatio).toBe(2) + expect(acc.note).toBe(BYTE_ACCOUNTING_NOTE) + expect(acc.note).toMatch(/NOT proof of double PTY delivery/) + expect(acc.clientUnit).not.toBe(acc.brokerUnit) + expect(acc.clientBaseline).toMatch(/probe-install/) + expect(acc.brokerBaseline).toMatch(/worker-start/) + }) + + it('yields a null ratio and non-commensurable flag when the broker predates offsets', () => { + const acc = deriveByteAccounting({ + clientBytesReceived: 500, + clientChunks: 4, + snapshotOffset: undefined + }) + expect(acc.snapshotOffset).toBeNull() + expect(acc.clientToBrokerByteRatio).toBeNull() + expect(acc.commensurable).toBe(false) + }) + + it('yields a null ratio when the offset is zero (avoids divide-by-zero)', () => { + const acc = deriveByteAccounting({ + clientBytesReceived: 0, + clientChunks: 0, + snapshotOffset: 0 + }) + expect(acc.clientToBrokerByteRatio).toBeNull() + // The offset was present (0) so the baselines are still comparable. + expect(acc.commensurable).toBe(true) + }) +}) diff --git a/tests/term-fidelity/byte-accounting.ts b/tests/term-fidelity/byte-accounting.ts new file mode 100644 index 00000000..a0d43ff7 --- /dev/null +++ b/tests/term-fidelity/byte-accounting.ts @@ -0,0 +1,83 @@ +// Pure derivation of the client-vs-broker byte accounting recorded in each +// divergence-bundle `meta.json`. Kept dependency-free (no Playwright, no xterm) +// so it is unit-testable in isolation — and so the two byte figures can never +// again be dropped into meta as a bare, unlabeled pair that reads as an +// "exact-2.0 double-delivery" signal. +// +// Background (term-fidelity program, 2026-07-17): `quiet.activity.bytes` and +// `brokerOffset` read exactly 2.0 apart in 5/5 codex bundles. That was PROVEN +// NOT to be double delivery — a headless probe measured client-bytes / +// snapshot-offset = 1.0000, and BrokerManager does exactly one IPC send per +// worker_stream event. The 2.0 was a derivation artifact of pairing two figures +// that measure different things on (historically) unstated baselines. This +// module makes what each figure measures explicit and computes the ratio on a +// SHARED baseline so the number is meaningful. + +export interface ByteAccountingInput { + // Client side: cumulative UTF-8 byte length of every `broker:pty-chunk` STRING + // this renderer received for the agent since the activity probe was installed. + // The probe is installed at harness launch, BEFORE the agent is spawned, so + // this series starts at the agent's very first byte (baseline = agent start). + clientBytesReceived: number + clientChunks: number + // Broker side: the attach snapshot's cumulative per-worker byte `offset` — raw + // PTY bytes the broker grid had consumed at capture, counted from worker start + // (offset 0). `undefined` on brokers that predate stream-offset support. + snapshotOffset: number | undefined +} + +export interface ByteAccounting { + clientBytesReceived: number + clientChunks: number + snapshotOffset: number | null + // clientBytesReceived / snapshotOffset, rounded to 4 dp. `null` when the + // offset is absent or zero (no meaningful ratio). ~1.0 on a faithful + // one-IPC-send-per-chunk pipeline. + clientToBrokerByteRatio: number | null + // True when both figures share the agent-start baseline and are therefore + // directly comparable. (Always true when snapshotOffset is present, because + // the probe is installed before spawn; recorded explicitly so a future change + // that installs the probe mid-stream can flip it to false rather than silently + // producing an incomparable ratio.) + commensurable: boolean + clientBaseline: string + brokerBaseline: string + clientUnit: string + brokerUnit: string + note: string +} + +export const BYTE_ACCOUNTING_NOTE = + 'clientToBrokerByteRatio compares client-received IPC bytes to the broker raw-PTY ' + + 'offset on a shared agent-start baseline. ~1.0 means one-to-one delivery. A ratio ' + + 'near an integer such as 2.0 is NOT proof of double PTY delivery — historically it ' + + 'was a derivation artifact (UTF-8 re-encoding of the decoded IPC string vs raw PTY ' + + 'bytes, and differing baselines). Never infer a delivery mechanism from this number; ' + + 'confirm duplicate delivery by counting BrokerManager IPC sends per worker_stream event.' + +function round4(value: number): number { + return Math.round(value * 10_000) / 10_000 +} + +export function deriveByteAccounting(input: ByteAccountingInput): ByteAccounting { + const hasOffset = + typeof input.snapshotOffset === 'number' && + Number.isFinite(input.snapshotOffset) + const snapshotOffset = hasOffset ? (input.snapshotOffset as number) : null + const ratio = + snapshotOffset !== null && snapshotOffset > 0 + ? round4(input.clientBytesReceived / snapshotOffset) + : null + return { + clientBytesReceived: input.clientBytesReceived, + clientChunks: input.clientChunks, + snapshotOffset, + clientToBrokerByteRatio: ratio, + commensurable: snapshotOffset !== null, + clientBaseline: 'activity-probe-install (installed pre-spawn ⇒ agent first byte)', + brokerBaseline: 'worker-start (snapshot offset 0)', + clientUnit: 'utf8-bytes-of-decoded-broker:pty-chunk-string', + brokerUnit: 'raw-pty-bytes-consumed-by-broker-grid', + note: BYTE_ACCOUNTING_NOTE + } +} diff --git a/tests/term-fidelity/harness.ts b/tests/term-fidelity/harness.ts index 28a7aabb..5a5cbce8 100644 --- a/tests/term-fidelity/harness.ts +++ b/tests/term-fidelity/harness.ts @@ -35,6 +35,11 @@ export interface FidelityHarness { telemetry: TelemetryRecord[] mainLogs: string[] currentWorkload: string | null + // Playwright retry index (0 = first attempt). Divergence + telemetry bundles + // are written under `attempt-/` so a retry never overwrites the prior + // attempt's artifacts (retry-then-pass used to erase real first-attempt + // divergence data). + attempt: number close(): Promise } @@ -169,7 +174,8 @@ async function validateConnection( export async function launchFidelityHarness( cli: FidelityCli, - repoRoot = resolve(__dirname, '../..') + repoRoot = resolve(__dirname, '../..'), + attempt = 0 ): Promise { // Keep the isolated tree outside the OS temp root. Agent sandboxes commonly // grant broad writes beneath TMPDIR, which would make the sibling userData @@ -319,6 +325,7 @@ export async function launchFidelityHarness( relayVersions, telemetry, mainLogs, + attempt, get currentWorkload() { return harnessState.currentWorkload }, diff --git a/tests/term-fidelity/oracle.ts b/tests/term-fidelity/oracle.ts index 5cf3b74a..3b533ad4 100644 --- a/tests/term-fidelity/oracle.ts +++ b/tests/term-fidelity/oracle.ts @@ -3,6 +3,7 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises' import { join } from 'node:path' import type { Page } from 'playwright' import { getActivity, type FidelityHarness } from './harness' +import { deriveByteAccounting } from './byte-accounting' export const QUIET_WINDOW_MS = 1_500 const QUIET_TIMEOUT_MS = 90_000 @@ -384,7 +385,18 @@ async function writeDivergenceBundle( quiet: QuietResult, options: CheckpointOptions ): Promise { - const artifactDir = join(harness.repoRoot, 'test-results', 'term-fidelity', harness.cli, workload) + // Segregate by Playwright attempt so a retry never overwrites the first + // attempt's bundle. A retry-then-pass previously clobbered the diverging + // first-attempt data (which twice turned out to be a REAL divergence event), + // leaving nothing to examine afterward. + const artifactDir = join( + harness.repoRoot, + 'test-results', + 'term-fidelity', + harness.cli, + workload, + `attempt-${harness.attempt}` + ) await mkdir(artifactDir, { recursive: true }) const telemetry = harness.telemetry.slice(options.telemetryAtStart) const meta = { @@ -407,7 +419,17 @@ async function writeDivergenceBundle( renderer: renderer.cursor, broker: broker.cursor }, - brokerOffset: broker.offset, + // Self-documenting client-vs-broker byte accounting. Replaces the former + // bare `brokerOffset` + `quiet.activity.bytes` pair, whose exact-2.0 reading + // in codex bundles was misread as double delivery (it was a derivation + // artifact — see byte-accounting.ts / the embedded note). Each figure now + // declares its baseline and unit, and the ratio is computed on a shared + // agent-start baseline so ~1.0 is the meaningful "one-to-one delivery" value. + byteAccounting: deriveByteAccounting({ + clientBytesReceived: quiet.activity.bytes, + clientChunks: quiet.activity.chunks, + snapshotOffset: broker.offset + }), quiet: { reached: quiet.reached, waitedMs: quiet.waitedMs, @@ -479,7 +501,8 @@ export async function writeTelemetryArtifact( 'test-results', 'term-fidelity', harness.cli, - 'reconciler-telemetry' + 'reconciler-telemetry', + `attempt-${harness.attempt}` ) await mkdir(artifactDir, { recursive: true }) const screenshot = await harness.page.screenshot({ animations: 'disabled', type: 'png' }) diff --git a/tests/term-fidelity/term-fidelity.spec.ts b/tests/term-fidelity/term-fidelity.spec.ts index 061376c9..40906aa5 100644 --- a/tests/term-fidelity/term-fidelity.spec.ts +++ b/tests/term-fidelity/term-fidelity.spec.ts @@ -1,4 +1,4 @@ -import { test } from '@playwright/test' +import { test, type TestInfo } from '@playwright/test' import { launchFidelityHarness, SUPPORTED_CLIS, @@ -18,12 +18,14 @@ function selectedClis(): FidelityCli[] { test.describe.configure({ mode: 'serial' }) for (const cli of selectedClis()) { - test(`${cli}: real Electron renderer matches isolated broker for all canonical workloads`, async () => { + test(`${cli}: real Electron renderer matches isolated broker for all canonical workloads`, async ({}, testInfo: TestInfo) => { let harness: FidelityHarness | null = null let workloadError: unknown = null let agentName = `tf-${cli}` try { - harness = await launchFidelityHarness(cli) + // Thread the retry index so divergence/telemetry bundles land under + // attempt-/ and a retry can't overwrite a prior attempt's artifacts. + harness = await launchFidelityHarness(cli, undefined, testInfo.retry) console.log( `[term-fidelity] ${cli}: instance=${harness.instanceName} port=${harness.apiPort} ` + `userData=${harness.userDataDir}` diff --git a/vitest.config.mjs b/vitest.config.mjs index a3841192..56712761 100644 --- a/vitest.config.mjs +++ b/vitest.config.mjs @@ -22,7 +22,9 @@ export default { include: [ 'src/main/**/*.test.ts', 'src/renderer/src/**/*.test.ts', - 'packages/**/*.test.ts' + 'packages/**/*.test.ts', + // Dependency-free term-fidelity harness units (e.g. byte-accounting). + 'tests/term-fidelity/**/*.test.ts' ], exclude: [ '**/node_modules/**',