relayburn-sdk: reader cleanup deferred subset (#346)#371
Merged
Conversation
Remaining reader-module cleanups deferred from #352: - codex.rs / opencode.rs: replace the manual repack of the incremental result with `From<ParseCodexIncrementalResult> for ParseCodexResult` (and the opencode equivalent), and have `parse_*_session` delegate via `.map(...::from)`. - codex.rs: drop the two `let _ = (...)` blocks at the tail of `parse_codex_buffer`. The audit confirms each `committed_*` snapshot is read by either the resume state or the emitted records, so the trailing working values are genuinely dead and removing the silencing blocks introduces no compiler warnings. - codex.rs: rename `memchr_newline` to `find_newline` so the name no longer implies a `memchr`-crate optimization that isn't there. The claude.rs parser-duplication item from #346 stays deferred: the non-incremental `ParseState` emits in-progress turns (records with `stop_reason.is_none()`), while `run_incremental` deliberately defers them to keep the next call's start cursor honest. Collapsing the two paths needs a flag on `run_incremental` and broader fixture validation; out of scope for this safe-subset PR. https://claude.ai/code/session_01GHEd4Sv87QoUTd1BN6FbAF
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo reader parsers (codex and opencode) are refactored to delegate to incremental variants with new ChangesParser Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
2 tasks
3 tasks
willwashburn
added a commit
that referenced
this pull request
May 10, 2026
) * relayburn-sdk: drop reintroduced let _ block in parse_codex_buffer PR #371 dropped the two `let _ = (...)` warning-silencers at the tail of `parse_codex_buffer`, but #372 (JSONL streaming refactor) re-added the larger one when it restructured the loop. Audit re-confirms each `committed_*` mirror is read either by the emitted records or by the resume state, so the live values are dead at function exit and modern rustc does not flag them. Refs #346. * changelog: trim parse_codex_buffer entry to impact-first style Per project changelog guidelines and CodeRabbit feedback on #405. * changelog: drop noop dead-code removal entry --------- Co-authored-by: Claude <noreply@anthropic.com>
3 tasks
willwashburn
added a commit
that referenced
this pull request
May 11, 2026
…) (#417) * relayburn-sdk: collapse parse_claude_session onto run_incremental (#346) Last remaining cleanup from #346: the non-incremental claude parser kept its own `ParseState` machinery in parallel with `run_incremental`, both reading the same JSONL shape but along separate codepaths. PR #371 deferred this because the two paths disagreed on trailing in-progress turns (incremental skips them; non-incremental emits them). This commit adds an `emit_in_progress` flag to `run_incremental`: - false (incremental callers): keep the existing "back up end_offset to the earliest in-progress message and skip those turns" behavior, so the next resume call re-reads them. - true (full-parse caller): use `cursor_offset` for end_offset and emit every working record. This matches the prior `ParseState::finish` output, including assistants without a `stop_reason`. `parse_claude_session_with_counter` now builds a `ParseIncrementalOptions { start_offset: Some(0), .. }` and calls `run_incremental(.., true)`, then converts via a new `From<ParseIncrementalResult> for ParseResult`. The ParseState struct, its impl, the `record_root` / `collect_explicit_claude_relationships` non-incremental helpers, and `derive_file_session_id` are gone — net -374 LoC in claude.rs. The full workspace test suite (claude reader + 600+ other tests) passes unchanged, validating that the single-shot output matches what `ParseState` produced. * relayburn-sdk: emit final unterminated line in single-shot claude parse Devin Review flagged that switching `parse_claude_session_with_counter` to delegate to `run_incremental` regressed one edge case: the prior `ParseState` path used `BufReader::read_line`, which surfaces the final line at EOF even without a trailing `\n`. `run_incremental`'s `read_until(b'\n')` loop guards against partial trailing lines because the incremental caller resumes at `cursor_offset` next tick — but a single-shot full parse has no next tick, so a syntactically complete final JSON line lacking `\n` (mid-write truncation, unflushed writer) was silently dropped. Fix: gate the partial-line `break` on `!emit_in_progress`. When the single-shot caller asks us to emit in-progress records, treat a non-newline-terminated tail as a complete line and bump `cursor_offset` past its body so the per-record offset filters still admit it. Regression test `full_parse_emits_final_line_without_trailing_newline` writes simple-turn.jsonl with the trailing `\n` stripped and asserts the turn still lands. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Picks up the items from #346 that #352 explicitly deferred as "riskier."
parse_codex_session/parse_opencode_sessionwithFrom<ParseCodexIncrementalResult> for ParseCodexResult(and the opencode equivalent). The session entry points now just.map(...::from).let _ = (...)"suppress unused warnings" blocks at the tail ofparse_codex_buffer. Audit confirms everycommitted_*snapshot they justified is read post-loop (resume state or emitted records), so the trailing working values are genuinely dead. Removing the silencing blocks introduces no new compiler warnings.memchr_newlinetofind_newline. The previous name implied amemchr-crate optimization that wasn't there (the body isiter().position(|&b| b == b'\n')); not adding the crate, just dropping the misleading name.Deferred
claude.rs parser duplication (item 1, claude path) stays deferred. The non-incremental
ParseStateemits in-progress turns (records withstop_reason.is_none()), whilerun_incrementaldeliberately defers them so the next call's start cursor is honest. Collapsing the two paths would change observable output for partially-written sessions and needs a flag onrun_incrementalplus broader fixture validation.string-field helpers (item 2),
verb()two-arm match (item 4),resolve_uncachedfallback (item 5) all already landed in #352.Test plan
cargo build --workspacecleancargo test --workspace— 618 SDK tests + integration + doc tests passcargo clippy --workspace --all-targets— no new warnings introduced (pre-existing warnings unrelated to this change)https://claude.ai/code/session_01GHEd4Sv87QoUTd1BN6FbAF
Generated by Claude Code