Conversation
- Add Message::System handling in 8 locations (exhaustive match) - System messages converted to user role for provider APIs - Token estimation and transcript rendering handle system messages - Add unit tests for Message::System conversion
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughSystem messages ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/agent/worker.rs (1)
1137-1137: Add a targeted worker-path test for system-message compaction behavior.Line 1137 changes behavior in the worker’s stateful compaction/recap flow; please add a focused test that exercises worker history compaction + transcript persistence/resume with
Message::Systempresent.As per coding guidelines "For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to
just gate-pr."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/worker.rs` at line 1137, Add a focused async worker-path test that reproduces the stateful compaction/recap flow with Message::System present: create a test (e.g., test_worker_history_compaction_with_system_message) that spins up the worker instance, appends a sequence of messages including rig::message::Message::System into the worker transcript, triggers the compaction/recap routine (the same path exercised by the worker state machine), persists the transcript, then shuts down and restarts the worker to verify transcript resume and that system messages were handled/compacted as expected (assert the persisted/transmitted transcript contains/omits system entries per the new behavior); target the async worker lifecycle helpers used in the file (the compaction/recap entry point and transcript persistence/resume functions) so the test explicitly covers race/terminal-state scenarios for cancellation and restart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/conversation/worker_transcript.rs`:
- Around line 465-469: The code currently serializes
rig::message::Message::System as TranscriptStep::UserText, losing system
semantics on restore; update TranscriptStep to add a SystemText variant and
change the serialization branch that handles rig::message::Message::System (the
match arm currently producing TranscriptStep::UserText) to produce
TranscriptStep::SystemText, and update the restore logic that converts
TranscriptStep::UserText into rig::message::Message::User to instead convert
TranscriptStep::SystemText back into rig::message::Message::System (keeping
other TranscriptStep mappings unchanged) so system messages preserve their role
end-to-end.
---
Nitpick comments:
In `@src/agent/worker.rs`:
- Line 1137: Add a focused async worker-path test that reproduces the stateful
compaction/recap flow with Message::System present: create a test (e.g.,
test_worker_history_compaction_with_system_message) that spins up the worker
instance, appends a sequence of messages including rig::message::Message::System
into the worker transcript, triggers the compaction/recap routine (the same path
exercised by the worker state machine), persists the transcript, then shuts down
and restarts the worker to verify transcript resume and that system messages
were handled/compacted as expected (assert the persisted/transmitted transcript
contains/omits system entries per the new behavior); target the async worker
lifecycle helpers used in the file (the compaction/recap entry point and
transcript persistence/resume functions) so the test explicitly covers
race/terminal-state scenarios for cancellation and restart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d01a1ba-8704-4b0d-80b7-93ac5da0fa92
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (4)
src/agent/compactor.rssrc/agent/worker.rssrc/conversation/worker_transcript.rssrc/llm/model.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| (!parts.is_empty()) | ||
| .then(|| serde_json::json!({"role": "assistant", "content": parts})) | ||
| } | ||
| Message::System { content } => Some(serde_json::json!({ |
There was a problem hiding this comment.
Small guard: if Message::System ever comes through empty (or whitespace), skipping it avoids emitting an empty Anthropic message.
| Message::System { content } => Some(serde_json::json!({ | |
| Message::System { content } => (!content.trim().is_empty()).then(|| serde_json::json!({ | |
| "role": "user", | |
| "content": content, | |
| })), |
| result.extend(tool_results); | ||
| } | ||
| Message::System { content } => { | ||
| result.push(serde_json::json!({ |
There was a problem hiding this comment.
Worth double-checking the source of Message::System here: stream_openai() already injects request.preamble as a role: system message. If rig v0.33 is emitting that same preamble as Message::System, this would send it twice (and demote it to user). If Message::System is meant as additional system content, mapping to role: system might preserve instruction priority.
| } | ||
| } | ||
| } | ||
| rig::message::Message::System { content } => { |
There was a problem hiding this comment.
Message::System is getting persisted as TranscriptStep::UserText, which transcript_to_history() later reconstructs as Message::User on resume. If that’s intentional (e.g., providers without a system role), a quick comment here would help avoid someone “fixing” this later and changing behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tools/worker_inspect.rs (1)
135-140: Inconsistent indentation on line 135.The
UserTextmatch arm has extra leading whitespace compared to sibling arms (Action,SystemText,ToolResult). TheSystemTexthandling logic itself is correct.🔧 Suggested fix
- worker_transcript::TranscriptStep::UserText { text } => { + worker_transcript::TranscriptStep::UserText { text } => { summary.push_str(&format!("**User:** {text}\n\n")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/worker_inspect.rs` around lines 135 - 140, Normalize the indentation of the match arms so the worker_transcript::TranscriptStep::UserText arm aligns with its sibling arms (e.g., Action, SystemText, ToolResult); specifically adjust the leading whitespace before the line containing worker_transcript::TranscriptStep::UserText { text } => { and its summary.push_str(&format!("**User:** {text}\n\n")); so it matches the indentation level used by worker_transcript::TranscriptStep::SystemText { text } => { and others, leaving the logic unchanged.src/conversation/worker_transcript.rs (1)
29-33: Add#[serde(other)]catch-all to handle unknown transcript variants safely.The new
SystemTextvariant is the correct semantic fix, but without a catch-all, old code cannot deserialize transcripts written by new code. This breaks the critical conversation-resume path inagent/channel_dispatch.rs(lines 1053-1055) if rollbacks occur. A similar pattern already exists insrc/opencode/types.rs:358. Consider adding:#[serde(other)] Unknown,to
TranscriptStepand handle it intranscript_to_history(), or ensure deployments are atomic and rollbacks are impossible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/worker_transcript.rs` around lines 29 - 33, TranscriptStep's new SystemText variant can break deserialization of older code; add a serde catch-all by adding an #[serde(other)] Unknown variant to the TranscriptStep enum (alongside SystemText) and update transcript_to_history() to explicitly handle Unknown (e.g., ignore or log and skip) so transcripts written by newer code deserialize safely in older deployments; reference the TranscriptStep enum, SystemText variant, and transcript_to_history() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/conversation/worker_transcript.rs`:
- Around line 29-33: TranscriptStep's new SystemText variant can break
deserialization of older code; add a serde catch-all by adding an
#[serde(other)] Unknown variant to the TranscriptStep enum (alongside
SystemText) and update transcript_to_history() to explicitly handle Unknown
(e.g., ignore or log and skip) so transcripts written by newer code deserialize
safely in older deployments; reference the TranscriptStep enum, SystemText
variant, and transcript_to_history() when making the change.
In `@src/tools/worker_inspect.rs`:
- Around line 135-140: Normalize the indentation of the match arms so the
worker_transcript::TranscriptStep::UserText arm aligns with its sibling arms
(e.g., Action, SystemText, ToolResult); specifically adjust the leading
whitespace before the line containing
worker_transcript::TranscriptStep::UserText { text } => { and its
summary.push_str(&format!("**User:** {text}\n\n")); so it matches the
indentation level used by worker_transcript::TranscriptStep::SystemText { text }
=> { and others, leaving the logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1760488c-2c22-492a-8aa9-8469465a3e62
📒 Files selected for processing (2)
src/conversation/worker_transcript.rssrc/tools/worker_inspect.rs
- System messages now convert to SystemText in convert_history - SystemText now converts back to Message::System in transcript_to_history - Add unit tests for System message round-trip in all three converters
vsumner
force-pushed
the
upgrade/rig-0.33
branch
from
cee1b17 to
ef2b390
Compare
2 participants
Summary
rig-corefrom v0.31 to v0.33Message::Systemhandling in 8 locations (exhaustive match requirement from rig v0.33)Message::SystemconversionTesting
cargo check --all-targets— passescargo fmt --all— appliedcargo test --no-run— compiles (execution blocked by local onnxruntime env)Message::Systemconversion:convert_messages_to_anthropic_maps_system_to_user_roleconvert_messages_to_openai_maps_system_to_user_roleconvert_messages_to_openai_responses_maps_system_to_user_roleNotes
rig-core v0.33 adds a new
Message::System { content: String }variant to theMessageenum. This change is internal —.preamble()API remains unchanged, but internally createsMessage::Systemvariants that our code must handle in exhaustive match statements.