Skip to content

Follow-up: deferred review findings from PR #895 #920

Description

@philcunliffe

Deferred, non-blocking findings from the neutral review rounds on PR #895 (head 30f60d27), triaged at the review-round cap. Every actionable defect found in review was fixed on the branch (a091f802, 30f60d27) and verified; the three items below are the residuals, each deliberate or out of scope for that PR.

1. Subagent body-derived gap rows read null attribution (deliberate trade-off, needs an empirical check)

hypaware-core/plugins-workspace/claude/src/telemetry/projection.js (~line 123): gap messages take attribution solely from their own body event (attributeMessageToEvent(gap, event)), so subagent gap rows carry agent_id / is_sidechain only if api_request_body / api_response_body events carry agent.name. Nothing in the repo shows that they do. Post-PR, a subagent turn's tool_use/tool_result gap rows read null attribution while its text row is sidechain-labeled.

This was left as-is on purpose: deriving attribution from a sibling event would reintroduce the batch-composition-dependent identity the PR removes (the content-hash message_id is scoped by agent_id). Follow-up: on a real Claude Code, confirm whether the body events carry agent.name, and record the answer in the claude_otel_shape_check procedure (docs/ACCEPTANCE.md). If they do not, document the gap as permanent on this path rather than papering over it.

2. is_sidechain is true or null on the OTEL path, never false (design decision, wants its own request)

projection.js (attributeMessageToEvent) sets is_sidechain only for a positively identified subagent, matching the proxy producer (hypaware-core/plugins-workspace/claude/src/projector.js:354) per the LLP 0252 parity requirement, and deviating from issue #881's acceptance text (is_sidechain: false on main-loop rows). The transcript producer writes the real boolean (transcripts.js:692), so a consumer filtering where is_sidechain = false silently drops every OTEL and proxy main-loop row while keeping transcript ones.

Decision needed (a change to the settled shape in LLP 0252 / 0262, so a new LLP request, not a drive-by): either both Claude producers start stamping is_sidechain: false for positively identified main-loop rows, or the three-valued column stays and query guidance moves to is null / is_not_distinct_from.

3. Content-hash scope shift across the upgrade causes bounded one-time duplicates (release-notes line)

Removing the exchange-level agent_id changes the fallback content-hash scope for body-derived blocks with no native uuid. A session live across the upgrade replays its request-body history under the new scope, so previously written blocks land a second time, and because the rows differ in agent_id, compaction's _hyp_cache_row_id dedupe will not collapse them (the live path has no pre-write part_id dedupe; that is backfill-only, hypaware-core/plugins-workspace/ai-gateway/src/dataset.js:578). Bounded and one-time; needs a line in the release notes for the release that ships PR #895.

Backlink: PR #895 (review records in the PR comments carry full repro detail for each item).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    neutral:stuckneutral attempted this but cannot complete it autonomously — needs a human

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions