Skip to content

fix: adversarial review round 5 — release v0.1.7#9

Merged
ejwhite7 merged 1 commit into
mainfrom
fix/review-round5
May 28, 2026
Merged

fix: adversarial review round 5 — release v0.1.7#9
ejwhite7 merged 1 commit into
mainfrom
fix/review-round5

Conversation

@ejwhite7

Copy link
Copy Markdown
Owner

Summary

Addresses all findings from the round-5 code review of v0.1.6. Each fix has a regression test; the full suite passes (470 examples, 0 failures).

Severity Finding Fix
🔴 Critical Compaction emitted an :assistant first message → Anthropic HTTP 400 ("first message must use the 'user' role") whenever the first preserved message was :user or the preserved window emptied Summary is now always :user; merged into the first preserved message when that is also :user (no consecutive same-role); empty window → lone :user summary. Extracted to Compaction#build_compacted_history
🟡 Minor Compaction "mirror case" branch was unreachable dead code (the preceding while guarantees preserved.first is never :tool) Removed it and the redundant second droppable.empty? guard
🟡 Minor Deterministic ProviderError (missing tool_call_id, invalid arg JSON — raised before any HTTP call) was retried through the backoff schedule Removed ProviderError from the retry set; Fallback failover unaffected (rescues the RubyPi::Error superclass)
🟡 Minor before_tool_call/after_tool_call hooks saw string-keyed args while events & tool_calls_made saw symbol keys Loop#act rebuilds each ToolCall with symbol-keyed args once, so all consumers agree
🟡 Minor Anthropic streaming finish_reason clobbered to nil by a trailing message_delta with no stop_reason Guarded assignment, matching OpenAI/Gemini
🟡 Minor Gemini finishReason.downcase assumed a String Both paths coerce via to_s, now consistent
⚪ Cleanup Dead streamed_content accumulator + inaccurate comment in Loop#think Removed; :provider_fallback event still fires
⚪ Docs Fallback class docstring still described removed happy-path buffering Rewritten to describe direct streaming + :fallback_start

Investigated, no change

Streaming error bodies via env.status — verified against the actual stack (faraday 2.14.1 / faraday-net_http 3.3.0): the net_http adapter calls save_http_response (sets env.status) before response.read_body streams chunks, and Env#stream_response passes that same populated env to the on_data proc. env.status is reliably available before the first chunk, so the existing error_body recovery works. No fix needed — documented in CHANGELOG.

Testing

  • New spec/ruby_pi/fixes/review_round5_spec.rb (9 examples) covering compaction ordering/merge/empty-window, retry policy (incl. an ApiError-still-retries control), Anthropic finish_reason, and Gemini finishReason robustness.
  • New hook-key-consistency example in spec/ruby_pi/agent/loop_spec.rb.
  • Updated two compaction_spec examples that encoded the old :assistant-first behavior.
  • bundle exec rspec → 470 examples, 0 failures.

🤖 Generated with Claude Code

Address all findings from the round-5 review of v0.1.6:

- Compaction: summary is now always a :user message (valid as the first
  Anthropic message), merged into the first preserved message when that is
  also :user to avoid consecutive same-role messages; empty preserved window
  yields a lone :user summary. Removed the unreachable mirror-case branch.
- BaseProvider: stop retrying deterministic ProviderError (raised during
  request construction); ApiError/RateLimitError/TimeoutError still retry.
- Loop: rebuild ToolCalls with symbol-keyed arguments so hooks, events,
  tool_calls_made, and the tool block all observe the same shape. Removed the
  dead streamed_content accumulator.
- Anthropic: guard streaming finish_reason against a trailing message_delta
  with no stop_reason clobbering it to nil.
- Gemini: coerce finishReason via to_s before downcase on both paths.
- Fallback: correct the stale class docstring (direct streaming, not buffering).

Verified env.status is available in faraday-net_http 3.3.0 on_data, so the
streaming error_body recovery needs no change.

Regression specs added in spec/ruby_pi/fixes/review_round5_spec.rb and
spec/ruby_pi/agent/loop_spec.rb; updated two compaction specs that encoded the
old :assistant-first behavior. Full suite: 470 examples, 0 failures.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@ejwhite7 ejwhite7 merged commit 63d8e7c into main May 28, 2026
3 checks passed
@ejwhite7 ejwhite7 deleted the fix/review-round5 branch June 9, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant