fix(protocol): fail-soft on malformed Claude assistant transcript rows#196
fix(protocol): fail-soft on malformed Claude assistant transcript rows#196Kunde21 wants to merge 5 commits into
Conversation
RawAgentOutputDataUnknownSchema carried a refine() that rejected any output `type` present in the known set. Combined with union([Known, Unknown]), a known-type row whose body did not match its known variant (e.g. a Claude `assistant` row missing `message`, with `message.role` missing, or with `content: null`) failed BOTH branches, so the whole record failed validation and the UI rendered "[Unparsed agent message]". Remove the refine so malformed known-type rows fall through to the catch-all. union([Known, Unknown]) tries Known first, so well-formed rows still get their rich Known variant (no behavior change for the happy path). The branded .transform() is preserved so TS narrowing on `data.type` keeps working. This realizes the stated intent of commit 4dd41fc ("fail-soft transcript normalization"), whose Unknown catch-all was meant to absorb malformed records but was blocked by the collide-refine. Add regression tests for the three real-world failing shapes plus a no-regression test for well-formed rows. unparsed_message_error.md documents the full root-cause analysis. All 43 pre-existing protocol tests + the 4 new tests pass.
WalkthroughThe transcript schema now preserves malformed assistant output through an opaque branch. Claude direct-session mapping attaches message-role metadata, and UI normalization and transcript import consume it while protocol, mapper, role-resolution, and normalization tests cover the updated behavior. ChangesFail-soft output handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeJSONL
participant ClaudeMapper
participant DirectTranscriptSchema
participant TranscriptImporter
participant UINormalizer
ClaudeJSONL->>ClaudeMapper: parse assistant or user record
ClaudeMapper->>ClaudeMapper: resolve messageRole
ClaudeMapper->>DirectTranscriptSchema: emit raw record with messageRole
DirectTranscriptSchema->>TranscriptImporter: validate imported transcript
TranscriptImporter->>TranscriptImporter: commit messageRole
DirectTranscriptSchema->>UINormalizer: validate and forward direct transcript
UINormalizer->>UINormalizer: filter event rows and normalize output content
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai @greptile-apps please review the latest changes. The PR now includes cross-layer protocol, CLI role-classification, and UI normalization regression coverage, plus cleanup of the opaque fallback naming and stale RCA document. |
|
@coderabbitai review |
|
@greptile-apps review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 196 --repo happier-dev/happier --json title,body,files,additions,deletions,commits -q '.'
echo "---DIFF---"
gh pr diff 196 --repo happier-dev/happierLength of output: 19434 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -i "unparsed_message_error" || true
git -C . log --oneline -3 2>/dev/null || true
gh pr diff 196 --repo happier-dev/happier --name-statusLength of output: 1131 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -i "unparsed" .
echo "---"
gh pr diff 196 --repo happier-dev/happier --name-onlyLength of output: 537 Confirmed the RCA doc is gone from the repo, and the diff matches the PR description exactly.
Test coverage
Housekeeping
Everything in the diff lines up with the PR description's claims, the union ordering guarantees no happy-path regression, and the cross-layer tests (protocol → CLI role classification → UI normalization) give good confidence in the fail-soft behavior at each boundary. I don't see any outstanding concerns. [approve] |
Greptile SummaryThis PR makes Claude transcript parsing tolerate incomplete provider payloads while keeping the shared envelope strict. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| packages/protocol/src/sessionMessages/transcriptRawRecordV1.ts | Adds an opaque fallback after strict known-row parsing while retaining shared-envelope validation. |
| packages/protocol/src/directSessions/daemonRpcV1.ts | Adds optional validated message-role metadata to direct transcript items. |
| apps/cli/src/backends/claude/directSessions/mapClaudeJsonlLineToDirectMessages.ts | Classifies parsed Claude rows and attaches the canonical role to direct transcript messages. |
| apps/ui/sources/sync/runtime/directSessions/normalizeDirectTranscriptMessages.ts | Forwards direct transcript role metadata into canonical message normalization. |
| apps/cli/src/api/directSessions/import/importDirectSessionTranscript.ts | Preserves direct transcript roles when committing imported messages to managed storage. |
Reviews (5): Last reviewed commit: "fix(transcript): retain roles during dir..." | Re-trigger Greptile
Greptile SummaryThis PR makes Claude transcript parsing fail softly for malformed known output rows. The main changes are:
Confidence Score: 5/5The changed flow looks mergeable after aligning direct-session handling with synced-session event filtering.
packages/protocol/src/sessionMessages/transcriptRawRecordV1.ts and the direct-session normalization path
|
| Filename | Overview |
|---|---|
| packages/protocol/src/sessionMessages/transcriptRawRecordV1.ts | Allows malformed known output bodies through the opaque branch while retaining shared-field validation. |
| packages/protocol/src/sessionMessages/transcriptRawRecordV1.test.ts | Covers malformed known rows, invalid shared fields, and well-formed assistant rows. |
| apps/cli/src/api/session/messageRole/resolveSessionMessageRole.test.ts | Adds role expectations for malformed assistant payloads. |
| apps/ui/sources/sync/typesRaw/normalize.outputInvalidKnownTypeFallback.test.ts | Covers synced-session filtering and content recovery but not direct-session normalization. |
Reviews (2): Last reviewed commit: "refactor(protocol): clarify opaque outpu..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .transform((value) => ({ ...value, type: value.type as OpaqueOutputDataType })); | ||
|
|
||
| const RawAgentOutputDataSchema = z.union([RawAgentOutputDataKnownSchema, RawAgentOutputDataUnknownSchema]); | ||
| const RawAgentOutputDataSchema = z.union([RawAgentOutputDataKnownSchema, RawAgentOutputDataOpaqueSchema]); |
There was a problem hiding this comment.
When this fallback admits a message-less or null-content assistant row, direct-session normalization has no messageRole and cannot apply the event filter used by synced sessions. Those rows therefore appear as [Unsupported agent output] instead of being filtered, so the same transcript has different visible output depending on which normalization path loads it.
Context Used: apps/ui/CLAUDE.md (source)
There was a problem hiding this comment.
Confirmed. Direct transcript items did not carry the producer-owned message role, so the UI could not apply the same event filtering as synced transcripts. Fixed in 72466d5 by adding validated optional messageRole metadata to DirectTranscriptRawMessageV1, populating it through the existing Claude role classifier, and passing it into the canonical UI normalizer. RED was reproduced in protocol, CLI direct mapping, and UI direct normalization; focused GREEN results are protocol 55/55, CLI 16/16, and UI 13/13.
Greptile SummaryThis PR makes Claude transcript parsing fail softly for incomplete provider payloads. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| packages/protocol/src/sessionMessages/transcriptRawRecordV1.ts | Allows malformed known output bodies to use the opaque fallback while retaining strict shared-field validation. |
| packages/protocol/src/sessionMessages/transcriptRawRecordV1.test.ts | Adds protocol tests for malformed known rows, invalid envelope fields, and valid assistant rows. |
| apps/cli/src/api/session/messageRole/resolveSessionMessageRole.test.ts | Adds role-classification tests for incomplete Claude assistant rows. |
| apps/ui/sources/sync/typesRaw/normalize.outputInvalidKnownTypeFallback.test.ts | Adds downstream tests for filtering, recoverable text, future output types, and string content. |
Reviews (3): Last reviewed commit: "refactor(protocol): clarify opaque outpu..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@greptile-apps review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
@greptile-apps review |
✅ Action performedReview finished.
|
2 participants
Problem
Claude unified-terminal transcripts can contain known row types whose payload body is incomplete, such as an
assistantrow withoutmessage, with a missing nested role, or withcontent: null. The strict known branch rejects these shapes, while the previous fallback also rejected every knowntype, causing the entire transcript record to fail validation.Goal and implementation
Keep the shared transcript envelope strict while making provider payload bodies forward-compatible:
uuid, still fail validation;The obsolete known-type collision set was removed, and the fallback is named
Opaquerather thanUnknownbecause it can intentionally contain either a future type or a known type whose body failed strict parsing.Observable behavior
eventis accepted by the protocol and intentionally filtered by the UI.agent.messageRolemetadata as synced messages, so non-renderable event rows are filtered consistently in both paths.This avoids claiming that every malformed known row renders an unsupported placeholder; the actual result correctly depends on the canonical role classification and normalization logic.
Tests and verification
git diff --checkpassed.The current TypeScript 7 CLI-package check cannot proceed because this older branch's existing
apps/cli/tsconfig.jsonstill declares removed optionbaseUrl(TS5102); that configuration predates and is outside this PR.Files changed
packages/protocol/src/sessionMessages/transcriptRawRecordV1.ts: known-first opaque fallback and naming cleanup.packages/protocol/src/sessionMessages/transcriptRawRecordV1.test.ts: protocol boundary and regression coverage.apps/cli/src/api/session/messageRole/resolveSessionMessageRole.test.ts: malformed-row role contracts.apps/ui/sources/sync/typesRaw/normalize.outputInvalidKnownTypeFallback.test.ts: observable downstream contracts.packages/protocol/src/directSessions/daemonRpcV1.ts, the Claude direct mapper, and the UI direct normalizer: validated role metadata and consistent event filtering for direct transcripts.apps/cli/src/api/directSessions/import/importDirectSessionTranscript.ts: role preservation across import/takeover into managed storage.The draft RCA document was removed because it was not an appropriate root-level project artifact and its claimed UI outcome was not universally correct.
Residual validation
The reporter's exact privacy-safe payload was unavailable, so the regression is validated with representative shapes already admitted by the Claude transcript producer. A live replay of the original session remains useful release validation, but is not required to establish the parser defect or the cross-layer contracts covered here.
Note
Fail-soft on malformed Claude assistant transcript rows during parsing
RawAgentOutputDataUnknownSchemawith a permissiveRawAgentOutputDataOpaqueSchemaintranscriptRawRecordV1.ts, so malformed known output payloads and unrecognized future types are accepted as opaque data instead of being rejected.messageRole(validated bySessionMessageRoleSchema) as an optional field onDirectTranscriptRawMessageV1Schema, propagating it through the Claude JSONL mapper and normalization pipeline.resolveClaudeSessionMessageRoleand tagged with a role of'event'or'agent', enabling downstream filtering in UI sync.messageRole: 'event'are now filtered out during normalization rather than passed through as renderable content.Macroscope summarized 4d47574.
Summary by CodeRabbit
messageRolethrough direct transcript ingestion, mapping, and normalization so API-error rows are correctly filtered.messageRoleclassification and malformed “known” assistant/output shapes.