Uh oh!
There was an error while loading. Please reload this page.
Forward Claude OTEL datasets to central - #997
Conversation
philcunliffe
commented
Aug 24, 2026
Dual-agent review — |
| Source | Finding (severity, evidence) | Intersects |
|---|---|---|
| Claude | Guard removal silently opts every derived dataset into central export (major, sink.js:176) | Risk 1 (privacy/export seam); corroborated by Codex cat-6 |
| Codex | 6) Security Surface: registration drops localOnlyContentColumns privacy contract (major, sink.js:582) | Risk 1 (privacy/export seam); corroborated by Claude |
| Claude | proto.md wire-contract doc stale (major, central/proto.md:133) | Risk 2 (cross-repo wire contract) |
| Claude | No test pins registration failure / once-only / 401 (minor, central-forward-chunking.test.js:289) | Risk 3 (missing coverage) |
| Codex | 1) Behavioral Correctness: forwardingTarget throws outside per-partition catch (minor, sink.js:93) | Risk 4 (lifecycle gap; driver-caught, effectively unreachable) |
| Claude | Mode-staleness defeats no-rerun-on-parse-miss guarantee (minor, action_attach.js:280) | Risk 4 (reconciler settling) |
| Claude | Open-dataset forwarding decision has no LLP (minor, sink.js:169) | Risk 2 (contract documented nowhere) |
| Claude | Stale "unknown signal" comment example (nit, sink.js:116) | (comment hygiene; no risk surface) |
Codex review
Fix Validations
Claude OTEL central forwarding
- Status: correct
- Evidence:sink.js, sink.js, central-forward-chunking.test.js
- Assessment: Claude telemetry now resolves to arbitrary-dataset forwarding, registers
claude_telemetry_events, and POSTs under the dataset name. The targeted test covers registration-before-ingest for that dataset.
Same-port Claude proxy-to-OTEL migration
- Status: correct
- Evidence:clients.js, action_attach.js, action_attach.js, attach-endpoint-drift.test.js
- Assessment: Both manual daemon-managed attach and config-control reconciliation now treat non-
otelClaude markers as stale when the endpoint is current, and the marker records the adapter-emitted mode after migration.
Findings
6) Security Surface
- Severity: major
- Confidence: medium
- Evidence:sink.js, hypaware-plugin-kernel-types.d.ts, datasets.js, sink.js
- Why it matters: Arbitrary dataset registration sends only schema/source/timestamp metadata, so datasets like context graph that declare
localOnlyContentColumnslose the privacy visibility contract when forwarded to central. - Suggested fix: Include the relevant
DatasetRegistrationprivacy metadata in the registration payload and add a regression with a dataset declaringlocalOnlyContentColumns; if the central protocol cannot preserve it yet, skip or reject forwarding those datasets until it can.
1) Behavioral Correctness
- Severity: minor
- Confidence: high
- Evidence:sink.js, sink.js, sink.js, driver.js
- Why it matters: A missing local dataset registration now throws before the per-partition catch, so one bad partition becomes a whole-batch driver failure instead of the partition-granular retry path.
- Suggested fix: Move
forwardingTarget()inside the partitiontry, and log/retry only that partition when target resolution fails.
No Finding
- Contract & Interface Fidelity, 3) Change Impact / Blast Radius, 4) Concurrency, Ordering & State Safety, 5) Error Handling & Resilience, 7) Resource Lifecycle & Cleanup, 8) Release Safety, 9) Test Evidence Quality, 10) Architectural Consistency, 11) Debuggability & Operability.
Evidence Bundle
- Changed hot paths: central
exportBatchtarget resolution and registration; central NDJSON ingest path; manualhyp attach claude; config-control attach marker freshness. - Impacted callers:central/index.js, driver.js, driver.js
- Impacted tests:central-forward-chunking.test.js, central-forward-chunking.test.js, attach-endpoint-drift.test.js,
test/core/attach-daemon-managed-tails.test.js:312 - Unresolved uncertainty: I did not verify central-server behavior outside this worktree, and I did not run the test suite during this review.
Claude review
Claude review
Guard removal silently opts every derived dataset into central export
- Severity: major
- Confidence: 85
- Evidence: hypaware-core/plugins-workspace/central/src/sink.js:176
- Why it matters: The removed
KNOWN_SIGNALSguards were the only barrier keeping non-signal datasets off the wire: the sink driver hands the central sink partitions from every registered dataset (src/core/sinks/driver.js:164, no dataset scoping), so context-graphnode/edge, context-graph-enrich, and gascity flip from never-exported (loud per-partition failure) to auto-registered-and-forwarded, and the export seam (readRowsSince, src/core/cache/storage.js:244) enforces only cwd/attribution withholding -localOnlyContentColumnssuppression (LLP 0105) lives solely in the query seam, so graph content columns (label,props,source_keys, which "can carry session-derived text and paths" aggregated from local-only sessions, context-graph/src/datasets.js:75-78) ship to the org server unredacted. - Suggested fix: Gate
forwardingTarget's open-dataset branch on an explicit opt-in (e.g. only forward datasets that declare asourceSignal, which coversclaude_telemetry_eventsvia'claude_telemetry'and matches the PR's stated scope) or applylocalOnlyContentColumnsnulling inreadRowsSincebefore any cwd-less derived dataset is allowed onto the wire.
Wire-contract doc proto.md left stale by the new dataset protocol
- Severity: major
- Confidence: 90
- Evidence: hypaware-core/plugins-workspace/central/proto.md:133
- Why it matters: LLP 0014 (llp/0014-sinks.spec.md:97) names
proto.mdas the home of the client-server wire contract, and CLAUDE.md's living-docs rule says a design change lands its doc edit in the same commit; after this PR the doc is wrong on two points it states as contract:{signal}"is one of" exactly logs|traces|metrics|proxy (the sink now also PUTs/v1/datasets/{name}and POSTs/v1/ingest/{dataset}for everything else), and the signal "resolves from the dataset'ssourceSignal" (a non-legacy dataset likeclaude_telemetry_eventsnow ingests under its dataset name even though it declaressourceSignal: claude_telemetry). - Suggested fix: Update proto.md in this PR: document
PUT /v1/datasets/{name}(body fields, idempotency, 401 refresh, once-per-sink-lifetime announcement) and amend the Ingest section so the four legacy signals are the special case and every other dataset POSTs under its dataset name after registration.
No test pins the dataset-registration failure or once-only contract
- Severity: minor
- Confidence: 85
- Evidence: test/plugins/central-forward-chunking.test.js:289
- Why it matters: The PR's new wire step has three behavioral claims its own comment makes (a failed PUT leaves the partition retryable, a 401 refreshes once, registration fires once per dataset per sink instance), and none is tested - the only registration tests are the two happy paths, so a regression (e.g. dropping the
registeredDatasetsmemo, or movingensureDatasetRegisteredout of the try so a PUT failure escapes the partition-granular retry) would pass the suite silently. - Suggested fix: Add three tests: (1) responder fails the PUT (500) -> assert
status: 'failed',retryPartitions.length === 1, no/v1/ingest/POST; (2) twoexportBatchcalls on the same sink -> exactly one PUT total; (3) responder 401s the first PUT -> oneidentityClient.refresh()and a second PUT with the same body.
Mode-staleness check defeats the documented no-rerun-on-parse-miss guarantee
- Severity: minor
- Confidence: 80
- Evidence: src/core/config/action_attach.js:280
- Why it matters:
parseAttachOutput's contract (action_attach.js:585-587) is that a payload parse miss "recordsdonewithout detail rather than re-running a successful attach", but a detail-less claude marker now failsmarker.mode !== 'otel'inisCurrent, so any claude attach whose report payload fails to parse re-runs the attach and rewrites the user's settings.json on every reconcile pass, never settling. - Suggested fix: In
perform(), whenclient === 'claude'and the attach succeeded butparsed?.modeis absent, recorddetail.mode = 'otel'anyway (the claude adapter has exactly one attach mode by LLP 0258), and update the parseAttachOutput doc comment.
Open-dataset forwarding is a real design decision with no LLP
- Severity: minor
- Confidence: 80
- Evidence: hypaware-core/plugins-workspace/central/src/sink.js:169
- Why it matters: This is a settled chose-X-over-Y (register-then-ingest under the dataset name, rather than widening
KNOWN_SIGNALSor minting a fixedclaude_telemetrysignal) that also removes a deliberately guarded invariant, and the nearest existing doc, LLP 0255's "the rows forward centrally by the same rules message rows follow", no longer describes what shipped, so per CLAUDE.md the choice is undocumented. - Suggested fix: Mint one small decision LLP (Systems: Sinks) settling the open-dataset forwarding protocol and the legacy-four split, add it to LLP 0255's forward-refs (
Extended-by:), and anchorforwardingTarget/ensureDatasetRegisteredwith@refannotations to it.
Stale "unknown signal" example in the failure-annotation comment
- Severity: nit
- Confidence: 82
- Evidence: hypaware-core/plugins-workspace/central/src/sink.js:116
- Why it matters: The comment explains undefined
batch_id/chunks_sent"for pre-stream failures like an unknown signal", but this PR deleted the unknown-signal throw, so the named example is now impossible. - Suggested fix: Change the example to "pre-stream failures like a failed dataset registration".
Reports: /Users/phil/workspace/hypaware/.git/worktrees/hypaware-pr997-review.jGJRMs/dual-review/pr-997
… docs Review round 1 on PR #997 at d20d807. - `exportBatch` resolved the wire target OUTSIDE the per-partition try. `forwardingTarget` now throws (unresolvable dataset), and that throw escaped `exportBatch` into the driver's outer catch, which respools the whole batch and reports zero exported, discarding partitions already POSTed in the same loop. Resolve inside the try so one bad partition costs one partition, which is the contract the loop documents. - The registration path escaped the dataset name and the ingest path did not, so with an arbitrary dataset name in `/v1/ingest/{signal}` the two calls could name different resources (and `joinUrl`'s `new URL()` would normalize a `..` segment out). Escape both the same way; the four legacy signals are encode-invariant, so their URLs are unchanged. - `TELEMETRY_EVENTS_SOURCE_SIGNAL`'s docstring still said rows POST to `/v1/ingest/claude_telemetry` and that the declaration exists so forwarding "never falls back to the dataset name", which is now exactly what happens. Corrected; its LLP 0255 ref still holds. - `proto.md` documented no `PUT /v1/datasets/{name}` and still said `{signal}` is one of four. Document the announce call and the two ways a signal now resolves, and mark the server side unverified from this repo. - `isCurrent`'s JSDoc said two things can go stale; there are three now. Document the mode axis, and give `ActionMarker.mode` a typed field beside its `endpoint` / `assets_key` siblings instead of leaving it `unknown` under the index signature. - Tests pinning the claimed bounds rather than restating them: the announce happens once per sink instance across two ticks; a rejected announce fails the partition, never reaches ingest, and is NOT remembered (so the next tick re-announces); an unresolvable dataset fails only its own partition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Aug 24, 2026
Review round 1: |
philcunliffe
commented
Aug 24, 2026
Stuck: this change needs a decision only you can makeWhat neutral was doing. Review rung on PR #997 (change set Why it cannot proceed1. The sink forwards every registered dataset, not just the Claude OTEL ones (high). 2. The first tick forwards the entire local history, contradicting the PR description (medium). 3. Unverifiable from this repo (medium). Whether the deployed central server actually serves 4. Latent re-attach loop (medium). What neutral needs from you
Also worth your attention: no LLP covers thisThe attach half is covered by LLP 0262 and both sites are annotated. The central-forwarding half carries no How to unstickReply with a comment on this PR, or push to the branch. Neutral monitors this thread and will re-engage with your guidance on its next tick. |
philcunliffe
commented
Aug 24, 2026
Re-engaging after the new head: open-dataset forwarding now fails closed for datasets that declare local-only content columns, preserving the privacy contract while keeping Claude OTEL eligible. |
philcunliffe
commented
Aug 24, 2026
Dual-agent review - |
philcunliffe
commented
Aug 24, 2026
Neutral review round 2Verdict on the reviewed head: Findings
FixedAll findings were fixed in Validation: focused central/attach tests passed, |
… lost watermark Round-2 neutral review of #997. A dataset ruled ineligible (one declaring `localOnlyContentColumns`, or one whose name a legacy ingest path reserves) was rejected by throwing, which routed it into `retryPartitions`. That is a permanent verdict, not a transport failure: `@hypaware/context-graph` is default-bundled, so on a joined machine every tick wrote an outbox file the driver never drains and held the sink at `partial` for a condition LLP 0305 calls correct. It is now a skip, named once per sink instance in `central.forward.dataset_withheld` (info for the privacy outcome LLP 0305 names, warn for the reserved-name plugin bug). Nothing is announced and nothing is ingested either way, so the privacy guarantee is unchanged. `SinkWatermarkStore.read` returns `null` for both "no watermark was ever written" and "a watermark is on disk but could not be read or parsed", and it never throws, so the caller's catch cannot tell them apart. The open-dataset baseline treated the second as the first: a truncated or transiently unreadable watermark on a host that had been forwarding for weeks would jump the cursor to the current high-water and permanently drop every row in between, silently, while reporting `exported`. That turns LLP 0040's at-least-once degradation into at-most-once loss. The baseline now stats the watermark path and refuses when a file is there, which fails only that partition and retries next tick. Also: `isCurrent`'s JSDoc still said `perform()` records the mode the adapter reports, which f07c161 changed, and did not mention that a machine below the LLP 0258 version floor now flips to a terminal `refused` marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Aug 24, 2026
Review round 2: |
philcunliffe
commented
Aug 24, 2026
Stuck: one privacy question is still open, and it is yoursWhat neutral was doing. Triage rung on PR #997 at head What your pushes closed (settled ground, do not re-litigate)Your pushes (
The blocker
Neutral is not re-opening LLP 0305 §eligibility; the declaration-keyed contract stands. The blocker is that the fix for this one plugin needs a column classification only you can make, and shipping without it would knowingly merge a silent privacy defect. What neutral needs from youAre
Non-blockers, recorded so you see the whole PRThese are preferences or accepted trades; they move to a follow-up issue the moment the blocker clears:
How to unstickReply on this PR with the forwardability answer (and the column classification if the answer is no), or push the declaration to the branch yourself. Neutral monitors this thread and re-engages on its next tick. |
philcunliffe
commented
Aug 24, 2026
Acknowledged and fixed on exact head |
philcunliffe
commented
Aug 24, 2026
Neutral capped-round triagePrivacy blocker O1 is closed on exact head The remaining O2 and O3 findings cannot cause captured-data loss in this patch and are non-blocking operational recovery/status issues. They are deferred to #999. The server-route availability remains the accepted external attestation already recorded in this PR. |
philcunliffe
commented
Aug 24, 2026
Final-head ship-risk assessmentChanged surface: central arbitrary-dataset registration/ingest, durable per-sink rollout manifests and watermarks, overlapping export serialization, Claude proxy-to-OTEL attach reconciliation, and privacy classification for context-graph-enrich datasets. Critical safety fact: enabling open-dataset forwarding must neither backfill pre-rollout Claude OTEL history nor forward datasets containing unprovenanced local-only content. The exact head establishes a durable start-now baseline before remote registration, fails closed on missing/corrupt established progress, serializes overlapping exports, and withholds every dataset declaring Executable evidence on exact head: Confirmed residual risks: an older server may look healthy while retaining unsupported open-dataset rows locally, and a version-floor refusal requires manual reattach after upgrade. Both are deferred to #999; neither loses existing capture. Server route availability remains an external deployment attestation. Cleared risks: no historical open-dataset backfill, no silent rebaseline after established progress is lost, no duplicate first-row forwarding under overlapping ticks, no endless outbox for deliberately withheld datasets, no reserved legacy-path impersonation, and no context-graph-enrich privacy leak. Classification: high. The patch changes privacy/export behavior, durable data progress, concurrency, and a client-server wire contract. Executable proof clears the known defect paths but cannot reduce those inherent consequence classes below high. |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Why
HypAware 1.25.0 captures
claude_telemetry_eventslocally but the central sink rejects itsclaude_telemetrysource signal. Existing proxy-mode markers can also short-circuit the proxy-to-OTEL migration when the gateway port is unchanged.Verification
npm test: 5,062 passed, 2 skippednpm run typechecknpm run smoke -- claude_telemetry_capturenpm run smoke -- client_attach_idempotentNo server change is required; the server already implements dataset registration and arbitrary-dataset ingest.