Skip to content

Follow-up: deferred review findings from PR #818 #843

Description

@philcunliffe

Deferred findings from the two review rounds and triage of PR #818 (otel-attach-0245, OTEL attach replaces proxy for Claude Code). Triage at head 9df31a01 verified the one true blocker from the earlier stuck report (spool byte cap unenforced while the daemon is down) is fixed: hyp claude-hook session-context now runs enforceClaudeBodySpoolCap last on every invocation (hypaware-core/plugins-workspace/claude/src/hook_command.js), pinned by test/plugins/claude-hook-spool-cap.test.js (removing the sweep call fails 5 of its 9 tests). Everything below was re-verified against the tree at 9df31a01 and judged non-blocking; each needs either a design decision or a small contract change, so they are recorded here rather than dropped.

Medium

  1. The Claude picker still composes proxy mode, contradicting the corpus's own forward-refs. hypaware-core/plugins-workspace/claude/hypaware.plugin.json:58 still declares "gateway_proxy_mode": true and the preset still writes proxy_mode: true (hypaware-core/plugins-workspace/claude/src/index.js:548), while llp/0243-picker-composes-proxy-mode.decision.md:11 (Accepted) and llp/0242-fresh-installs-attach-base-url.issue.md:9 now carry forward-refs saying the opposite. A Claude-only hyp init still turns the gateway into a forward proxy and mints an interception CA for a client that no longer routes through it, and maybeOfferProxyModeMigration (src/core/commands/clients.js:821, offered at :308) can still print a false "attaches claude by base URL" claim and offer a proxy migration immediately after an OTEL attach (pinned by test/core/attach-proxy-migration.test.js:256, test/core/init-proxy-mode-default.test.js:56). Two Accepted docs disagree; needs a new decision naming LLP 0243, then the code change plus the two pinned tests. Not a blocker: OTEL attach itself is unaffected and the CA mint sits behind its own consent gate.

  2. dedupeStoredPartIds scans the whole dataset plus the spool on every live write. hypaware-core/plugins-workspace/ai-gateway/src/dataset.js:539 calls scanExistingPartIds(storage, batchKeys); the only early exit (dataset.js:731) fires only when every key of the batch was found, which never happens for genuinely new rows, the normal case for a live listener. Runs once per projection per POST (exchange_writer.js:60). Degrades with dataset size ("the listener got slow after a month"). Bounding it (partition pruning by date, or a part_id index) is a dedupe-contract design call.

  3. hyp session status asks only the gateway. hypaware-core/plugins-workspace/ai-gateway/src/session_command.js:256 resolves only resolveGatewayEndpointForCli, while runMutation (:339 on) posts to every recorder advertising the route. An ignore that landed on the Claude telemetry listener alone reads back status: unknown / ignored: null. The drop itself lands; this is a reporting inconsistency. Coordinate with open PRs hyp daemon status cleans what it reads out of status.json, plus the etag and probe-error residue from #681 (#776) #777 and hyp status names the CA's permitted hosts, uninstall clears every duplicate root (#793) #800, which touch the same file; the fix changes SessionStatusReport's shape.

  4. lastEventAt is stamped before both policy gates, so a fully-withheld capture reports healthy. hypaware-core/plugins-workspace/claude/src/telemetry/source.js:561 (metrics) and :604 (logs) advance state.lastEventAt before partitionIgnoredSessionEvents (:568, :613) and applyUsagePolicy (:575, :625). A machine whose SessionStart hook never landed withholds everything as undetermined, writes zero rows, and capture health says ok. Moving the stamp would false-alarm on deliberately-hypignored machines; the honest fix is a new diagnostic reading the already-published events_undetermined counter, which no hyp status surface reads today.

  5. A retried batch loses usage and cost attributes. hypaware-core/plugins-workspace/claude/src/telemetry/projection.js:251 and bodies.js (claimUsage) consume usageByRequestId during projection, before the write at source.js:673. If recordProjectedExchange throws and the exporter retries the batch, re-projected assistant rows carry no attributes.usage / claude.cost_usd. Fix is to consume the map only after the writes succeed, or snapshot and restore on throw: a projector-contract change.

  6. Telemetry endpoint desync: prevention half. Round 2 added detection (client_telemetry_stale, src/core/daemon/status.js:1213 on) but prevention was deliberately left: resolveAttachTelemetryPort writes the default 4319 endpoint when no live daemon publishes a port, and a foreign process holding 4319 at daemon bind time receives full prompt/response text. One cheap sub-case worth settling on its own: telemetry.listen_port: 0 plus a daemon-down attach writes 4319, an address that config guarantees will never be bound, so capture is permanently zero. Choosing between "refuse", "write and repair on next daemon start", and "keep writing the default" is a design call on an Accepted decision's consequence.

Low

  1. client_attach_stale fires against the gateway for an otel client. src/core/daemon/status.js:1158 compares the marker's gateway port mode-independently, so an OTEL-attached claude gets a misleading gateway-rebind diagnostic. Repair is idempotent and client_telemetry_stale covers the port that matters.

  2. body_ref logged verbatim. hypaware-core/plugins-workspace/claude/src/telemetry/source.js:415 and :651 log a refused (by definition out-of-spool, possibly attacker-supplied) path into a warn line an operator sink may ship. The rest of the PR logs basenames and counts only. A hash or basename plus reason code keeps it diagnosable.

  3. Counting nits. deleteSpooledBodies counts ENOENT as deleted (bodies.js:133-145, fs.rm with force: true; over-reports bodies_deleted, pinned by test/plugins/claude-telemetry-bodies.test.js:166-168); suppressSession (source.js:400) deletes body files without decrementing state.spoolBytes, so spool_bytes drifts high between sweeps; lastEventAt is maxed by string comparison over mixed timestamp shapes (source.js:561, :604), which mis-orders Z vs .000Z forms and non-Z offsets, and it feeds the capture-health baseline.

  4. Two shapes worth a decision. An unparseable (possibly still-being-written) body file is deleted on first sighting (bodies.js:102-107); a one-batch or age-based grace would avoid losing content to a write race. hyp purge's consent sentence (src/core/commands/purge.js:66-68) does not say the entire capture spool is swept (purge.js:108), which is intentional per LLP 0253 but unstated in the text the user agrees to.

Carried context for whoever picks this up

Backlink: PR #818 (triage at head 9df31a017c9b4e3ecf862c4db3fb4f9d2a4b4882).

Metadata

Metadata

Assignees

No one assigned

    Labels

    neutral:fixDelegate this issue to neutral for an autonomous fix attempt (reproduce -> fix -> PR)

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions