Deferred #843 findings: listener counters, the refusal log, and the usage a failed batch consumed

[philcunliffe]

Six of the ten review findings deferred from PR #818 onto issue #843: the ones that are a bug with a reproducing test, rather than a decision. Each fix has a regression test that fails on master and passes here. The remaining four items are listed at the bottom and stay open on the issue, so this PR says Refs #843, not Fixes.

Fixed here

Finding 5 - a retried batch lost its usage and cost. The usage index outlives one POST on purpose: the exporter flushes on a timer, so a turn's api_request (tokens, cost_usd) and the assistant_response that names its request_id routinely arrive in different flushes. Projection claims the entry; before this, a recordProjectedExchange that then threw left it claimed, and the exporter's retry re-projected against a drained index and wrote the same assistant rows with no attributes.usage and no claude.cost_usd - permanently, in exactly the batch that had already failed once. The catch now puts back only what this batch consumed, so an api_request whose response has not arrived yet is untouched. (telemetry/source.js)

Finding 7 - client_attach_stale fired at an otel-attached client. An otel marker records the gateway port like every other marker does, but nothing that client sends goes there. A routine gateway rebind printed "attached at port X but the gateway is now bound to Y - re-attach" at a client whose capture the rebind did not touch. The comparison is now gated on the attach mode; client_telemetry_stale still watches the port this mode actually depends on. (src/core/daemon/status.js)

Finding 8 - body_ref logged verbatim. A refused ref is out-of-spool by definition and arrived over the wire, so the warn line carried an unvalidated absolute path into a signal an operator's own sink may ship off the machine. It is now a 12-hex body_ref_sha256, which still correlates repeats of one ref across lines, per LLP 0257 S23. (telemetry/bodies.js, telemetry/source.js)

Finding 9a - deleteSpooledBodies counted ENOENT as deleted. fs.rm(..., { force: true }) succeeds on a path that is not there, so bodies_deleted and bodies_dropped over-reported every already-evicted ref. It now stats before the unlink and counts only what it removed. The pinned assertion in test/plugins/claude-telemetry-bodies.test.js is updated to the honest contract.

Finding 9b - spool_bytes drifted high across a drop. suppressSession deletes body files unread, and nothing subtracted them, so the gauge only came back down at the next sweep (up to a minute later) while hyp status reported bytes for content already removed on the user's say-so. deleteSpooledBodies now returns bytesRemoved and the drop arm subtracts it.

Finding 9c - lastEventAt was maxed by string comparison. event.timestamp arrives in mixed shapes (...:24Z, ...:24.500Z, and a legal OTLP numeric offset), and by text ...:24Z sorts after ...:24.500Z. The published last_event_at is the baseline hyp status measures a capture gap from, so a max that runs backwards invents a gap. Now compared as instants, with the old string compare kept as the fallback for an unparseable value.

Tests

• test/plugins/claude-telemetry-listener-accounting.test.js (new) - drives a real listener on an ephemeral port with a fake gateway and storage behind it and POSTs OTLP/JSON at it: findings 5, 8, 9a, 9b, 9c. All five fail on master.
• test/core/status-otel-attach-stale.test.js (new) - finding 7, plus a companion case proving a base_url attach in the same shape still gets the warning.
• test/plugins/claude-telemetry-bodies.test.js - the two deleteSpooledBodies assertions updated for the counting fix.

npm test and npm run typecheck are green locally, with one pre-existing exception: no tool transcript is tracked in the repo fails on master too, because x/npm-test.log and x/typecheck.log were committed in #785 (adb448ab). It is untouched by this PR and left for whoever owns that tree.

Still open on #843

No code here, and no Fixes, because each of these needs a call this PR cannot make:

1. The picker still composes proxy mode - two Accepted docs (LLP 0243 and the forward-refs on LLP 0242/0262) disagree. Needs a new decision naming LLP 0243 before the code and its two pinned tests move.
2. dedupeStoredPartIds scans the whole dataset per live write - bounding it (partition pruning, or a part_id index) is a dedupe-contract design call.
3. hyp session status asks only the gateway - the fix changes SessionStatusReport's shape, and hyp status names the CA's permitted hosts, uninstall clears every duplicate root (#793) #800 is still open on the same neighbourhood.
4. lastEventAt is stamped before both policy gates - the ordering itself is the question the issue flags (moving the stamp false-alarms on deliberately-hypignored machines); the honest fix it names is a new diagnostic over events_undetermined. Note 9c above changes only how two timestamps are compared, not when the stamp happens.
5. Telemetry endpoint desync, prevention half - "refuse" vs "write and repair on next daemon start" vs "keep writing the default" is a decision on an Accepted doc's consequence.
6. The unparseable-body grace period, and hyp purge's consent sentence - both are shapes the issue itself marks as needing a decision.

Refs #843

[philcunliffe]

Review of fix/issue-843 at 40e40f9d

Verdict: findings. Six findings, two fixed on the branch (pushed as e19462bb), four left open with reasoning below. npm test (4489 tests) and npm run typecheck were both already green on the reviewed head, and both are green after the fixes.

The three fixes I checked and did not find fault with: the probe.mode !== 'otel' guard in status.js is correct (the otel attach writes no ANTHROPIC_BASE_URL/HTTPS_PROXY, and the else if chain still reaches client_attached_not_configured for the !configured case), the deleteSpooledBodies stat-then-unlink counting is sound with every caller updated, and bodyRefDigest is applied at both refusal sites.

---

1. High - the finding-5 restore is inert against the real writer: a retried batch writes no assistant row at all, not one missing usage

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:698

The added comment says the retry "writes the same assistant rows with no attributes.usage and no claude.cost_usd". It does not. aiGatewayRowsFromProjectedExchange commits state.seenMessages.add(identity.messageId) at hypaware-core/plugins-workspace/ai-gateway/src/message_projector.js:772, during row expansion, and createProjectedExchangeWriter.record() only calls storage.appendRows afterwards (hypaware-core/plugins-workspace/ai-gateway/src/exchange_writer.js:59-63). Nothing rolls that set back when the append throws, and the writer is memoized for the process (ai-gateway/src/api.js:127). So the retry hits the seenMessages.has(...) continue at message_projector.js:737, gets rows.length === 0, and returns without appending.

Reproduced directly against the real writer with a storage stub whose first appendRows throws:

attempt1 threw: dataset unavailable
attempt2 result: { rowsWritten: 0, rowsSkipped: 0 } stored rows: 0

So the whole failed batch's rows are lost permanently, usage or not, and restoring usageByRequestId restores nothing an operator can observe. test/plugins/claude-telemetry-listener-accounting.test.js does not catch this because startListener stubs recordProjectedExchange with a plain function and asserts on the projection object rather than driving createProjectedExchangeWriter.

Left open, deliberately. The bug is pre-existing on master and lives in the dataset owner's contract, not in this PR's lines. Making expansion and append atomic means either rolling back seenMessages, the per-thread chain.seen/chain.last, and conversationStartedAt, or changing aiGatewayRowsFromProjectedExchange to return { rows, commit } - a change to the shared live-projector path the proxy also runs on, with part_id identity and LLP 0252 dedupe implications. That is a decision, not a review-rung edit, and it belongs on the open list on #843 alongside the other four items.

The restore in this PR is not wrong - it is the correct half of the picture, and it becomes load-bearing the moment the writer is made atomic. Only the comment's claim about what a retry currently does is inaccurate.

2. Medium - one malformed event.timestamp pinned last_event_at for the daemon's lifetime. Fixed.

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:781 (as reviewed)

event.timestamp is taken off the wire with no validation (telemetry/events.js:175, stringOf(attributes['event.timestamp'])). The new newerEventTimestamp fell back to the string compare whenever either side failed to parse, and that fallback is symmetric, so a value like unknown wins on arrival (nothing an ISO stamp can begin with sorts above u) and then beats every genuinely newer event that follows. assessCaptureHealth (src/core/daemon/status.js:2107) parses the result to undefined, falls back to listener_started_at, and raises a capture_gap warning, then an error, against a listener that is capturing fine.

Fixed on the branch: a value that names an instant now beats one that names none, whichever side it arrives on; the string compare survives only for the pair where neither parses. Regression case a malformed event.timestamp does not pin last_event_at added to test/plugins/claude-telemetry-listener-accounting.test.js; it fails on 40e40f9d and passes on e19462bb.

3. Low - the projected-body arm still subtracted bytes read, not bytes removed. Fixed.

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:735 (as reviewed)

The PR added bytesRemoved precisely so the gauge reflects what left the disk, then the success arm kept state.spoolBytes -= spooled.consumedBytes and ignored it. A body whose fs.rm fails (EPERM, a read-only spool) is still occupying the cap, so deducting its bytes under-reports spool_bytes until the next sweep restates it: the drop arm's bug in the other direction, in the same commit that fixed the drop arm. Now uses removed.bytesRemoved.

4. Low - the restore bypasses USAGE_INDEX_LIMIT and inverts eviction order

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:700

rememberUsage (telemetry/projection.js:264) trims the map to USAGE_INDEX_LIMIT oldest-first; the catch block re-inserts with a bare .set(). If projection remembered new api_requests that evicted older entries, the restore resurrects the evicted ones, so the map can transiently reach ~2x the cap, and because Map iterates in insertion order the resurrected stale entries now sort as newest - the next rememberUsage trims genuinely fresh unclaimed entries first.

Left open. It self-heals: the next rememberUsage runs while (index.size > USAGE_INDEX_LIMIT) and trims back down, and the overshoot is a few hundred small objects. The obvious fix (route the restore through rememberUsage) fixes the cap but not the ordering inversion, so it is not clearly better than what is here. Worth a note rather than a change.

5. Low - the snapshot/restore widens an existing cross-request race on the shared usage index

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:661

src/core/otlp/server.js:129 awaits handler.handle(...) per request with no serialization, and the handler awaits file reads and dataset writes, so two overlapping /v1/logs POSTs interleave. Request A snapshots the map, request B consumes R1 and writes its row successfully, then A's write fails and restores R1 - resurrecting usage that has already been spent. It leaks rather than corrupts, since the request_id never recurs, but the entry occupies a capped slot until eviction.

Left open. Inherent to a shared mutable index behind an unserialized handler, and the pre-existing exposure is the larger half.

6. Low - the unparseable-body arm still leaves spool_bytes drifted high

hypaware-core/plugins-workspace/claude/src/telemetry/bodies.js:104

loadSpooledBodies deletes an unparseable body with fs.rm(file, { force: true }) and counts it in unparseable, but its bytes are never subtracted from state.spoolBytes - exactly the gauge drift finding 9b fixed for the policy-drop arm, on a third arm.

Left open. The fix means widening loadSpooledBodies's return contract with an unparseableBytes field, and it is a distinct arm from the two #843 enumerated. Better as a seventh item on the issue than as unscoped growth in this PR.

---

Checks

• npm test: 4491 pass, 0 fail, 1 skipped (was 4490/0/1 on 40e40f9d; +1 for the new regression case).
• npm run typecheck: clean.
• Neither was failing on the unmodified head. The no tool transcript is tracked in the repo failure the PR description mentions is already resolved on this branch by 9b27c7a6.

Pushed

e19462bb - findings 2 and 3, plus the regression case. Both fixes are confined to code this PR introduced.

[philcunliffe]

Neutral review round: PR #851 @ e19462bb

Verdict: changes were needed and have been pushed. Two findings (1 medium, 1 low), both fixed on the branch at 1378a82c. Full suite green (4493 pass / 0 fail / 1 skipped) and npm run typecheck clean after the fixes.

Scope reviewed: hypaware-core/plugins-workspace/claude/src/telemetry/bodies.js, hypaware-core/plugins-workspace/claude/src/telemetry/source.js, src/core/daemon/status.js, and the three test files.

Verified correct (no change needed)

• The probe.mode !== 'otel' gate in src/core/daemon/status.js: otel mode is claude-only and writes no gateway-routing env keys, so the else if chain falls through cleanly and client_telemetry_stale covers the port that matters.
• newerEventTimestamp's three-way parse comparison, so a malformed stamp cannot pin last_event_at.
• The stat-before-unlink accounting in deleteSpooledBodies and all of its callers, and bodyRefDigest.
• state.spoolBytes under-reporting on the drop arm between sweeps is real but pre-existing and symmetric (bodies written after a sweep were never added to the gauge either); the cap is enforced independently by the sweep, and the PR documents the tradeoff. Not reported.

Finding 1 (medium, fixed): the failed-write restore reopened the usage-index cap

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:697-700 (at e19462bb)

The catch-block restore reinstated every key present in usageBeforeProjection but absent from usageByRequestId. That set is not "what projection consumed": it also contains the keys USAGE_INDEX_LIMIT (512, projection.js:19) evicted during the batch, because rememberUsage trims oldest-first as it remembers. The two are indistinguishable from the after-state, so the loop put the evictions back too.

That matters only when writes keep failing, and then it matters a lot. During a sustained dataset-write outage every POST throws, so nothing is ever claimed, nothing ever shrinks the index, and each failed batch adds its api_request entries on top of a map that can no longer trim itself. Reproduced against the real projectClaudeTelemetryEvents: batches of 64 api_request events with the write failing every time push the index to 576 entries by round 8 and it grows linearly from there. The code comment ("Put back only what this batch consumed") stated the correct intent; the loop did not implement it.

Fix: the restore is now restoreUnclaimedUsage(index, snapshot), exported from projection.js, which puts the snapshot back and then re-applies the cap via the extracted trimUsageIndex that rememberUsage already used. Restored entries land at the tail, so a claimed entry that the retry needs survives the re-trim; only the cap's own overflow is shed.

Regression test: test/plugins/claude-telemetry-usage-index.test.js drives 40 failed batches through the real projector and asserts the index settles at USAGE_INDEX_LIMIT instead of growing past it, plus a direct case that the claimed entry does come back and a newly remembered one is left alone. Confirmed the new test fails on the pre-fix logic (round 8 left the index at 576, over the 512 cap) and passes after.

Finding 2 (low, fixed): an @ref cited S18 for something S18 does not say

hypaware-core/plugins-workspace/claude/src/telemetry/source.js:697 and test/plugins/claude-telemetry-listener-accounting.test.js:153 (at e19462bb)

Both glossed @ref LLP 0257#failure-modes as "S18 - a retried batch is re-projected from the same inputs, so its inputs have to survive". S18 (llp/0257-claude-telemetry-listener-source.spec.md:109) actually reads: "Delivery is best effort. A down daemon loses events; content is recovered by transcript backfill, and behavioral-event loss in that window is accepted." Nothing there states a retry-input-survival requirement. CLAUDE.md's "Keep refs honest" rule makes this a defect, not a nitpick.

Fix: both annotations removed. LLP 0257 is Accepted, so per CLAUDE.md the spec is not edited to fit the code; the explanatory prose above each construct already carries the full reasoning and does not need a ref to tell you something new. The other two 0257#failure-modes refs in the tree (events_dataset.js:84, claude-telemetry-events-dataset.test.js:114) cover S20 and are accurate; they were left alone.

Stick it with this question: the behavior the removed refs described (a retried batch must re-project against surviving inputs) is a real requirement with no clause in LLP 0257 that states it. If a maintainer agrees it belongs in the spec, it wants a new request that extends 0257 rather than an edit to the Accepted doc, and then the two removed annotations can come back pointing at the new clause.

[philcunliffe]

Neutral triage at head 1378a82c: the review fix-loop hit the round cap with residual findings, all re-verified against the head and judged non-blocking (the one high-severity item, silent batch loss on a failed-write retry, is pre-existing on master in the shared writer path and already tracked as #879; the rest are a self-healing index-slot leak, a sweep-corrected gauge drift, a deliberate ordering tradeoff, and a spec-process question). Deferred findings are enumerated with file:line evidence in follow-up issue #904. This PR is cleared to merge.