Accept core runtime health fallback#4
Conversation
📝 WalkthroughWalkthroughAdds harness-or-core fallback probing for doctor/health_probe, new chat shell session-tree commands and matching CLI args, session payload builders for clone/tree/export/compact, a detailed example config, and README/setup/test updates. ChangesRuntime probes, session-tree ops, payloads, and docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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 (3)
src/payload.rs (2)
421-444: 💤 Low valueOptional: cover the
Some("")system_prompt branch.The current tests verify
Noneomits the field andSome("custom")includes it, but the new.filter(|s| !s.is_empty())branch (empty string also omitted) is untested. A one-line assertion would lock in that behavior so a future refactor of the filter doesn't silently regress.♻️ Suggested addition
#[test] fn build_run_payload_preserves_system_prompt_override() { @@ assert_eq!(payload["system_prompt"], "custom"); } + + #[test] + fn build_run_payload_omits_empty_system_prompt() { + let payload = build_run_payload(&RunPayloadParams { + session_id: "s1".into(), + messages: vec![], + provider: "anthropic".into(), + model: "claude-sonnet-4-6".into(), + system_prompt: Some(String::new()), + approval_required: vec![], + image: "python".into(), + idle_timeout_secs: 120, + max_turns: 3, + cwd: "/tmp/project".into(), + cwd_hash: "abc".into(), + }); + assert!(payload.get("system_prompt").is_none()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/payload.rs` around lines 421 - 444, The test suite lacks coverage for the new .filter(|s| !s.is_empty()) branch in build_run_payload; add a test (or extend build_run_payload_preserves_system_prompt_override) that calls build_run_payload with RunPayloadParams.system_prompt set to Some("".into()) and assert that payload.get("system_prompt").is_none() to ensure empty-string system prompts are omitted; reference the build_run_payload function and the RunPayloadParams struct so the assertion targets the same payload map shape as the existing tests.
105-122: 💤 Low valueImplementation is correct; verify downstream harness handles missing
system_promptkey.The tests confirm the intended behavior:
system_promptis omitted from the payload whenNoneor empty. Any downstream consumer (harness, router, session persistence) that previously read this field must now treat its absence the same as the old default behavior. This is outside the scope of the current codebase but worth confirming before merge.Additionally, consider documenting this behavior in the README or API docs—specifically that
Some("")is now silently treated asNone, allowing users to clear the override and fall back to the canonical prompt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/payload.rs` around lines 105 - 122, build_run_payload now omits the "system_prompt" key when RunPayloadParams.system_prompt is None or Some("") which requires downstream code to treat a missing key as the same as None/empty; update any consumers (harness, router, session persistence) that read payload["system_prompt"] to handle absence by falling back to the canonical prompt (i.e., treat missing key as None/empty) and add a short note to the API/README that Some("") is treated as clearing the override so callers can rely on omission semantics.src/cli.rs (1)
90-95: 💤 Low valueAdd
conflicts_with = "session_id"to the--newflag definition.Both flags can be supplied together (
chat --session-id s1 --new) and clap silently accepts it, leaving precedence to the dispatch layer. Using clap'sconflicts_withattribute (already used elsewhere in this file) makes the intent explicit and gives users a clear error message instead of silently ignoring one argument.♻️ Suggested change
#[arg(long)] pub session_id: Option<String>, - #[arg(long, help = "Start a fresh session instead of resuming this cwd")] + #[arg( + long, + conflicts_with = "session_id", + help = "Start a fresh session instead of resuming this cwd" + )] pub new: bool,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.rs` around lines 90 - 95, The --new flag currently lacks a clap conflict declaration so users can pass both --session-id and --new; update the #[arg(...)] attribute on the pub new: bool field to include conflicts_with = "session_id" (i.e., add conflicts_with = "session_id" to the #[arg(...)] on the new field) so clap will produce a clear error when both session_id and new are supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app.rs`:
- Around line 763-776: status_session/print_session_status must handle the
state::get envelope the same way as load_last_cwd_session and
extract_session_messages: check if the returned JSON is an object with a "value"
key and, if so, set value = value["value"] before accessing fields; update
print_session_status (or the caller status_session) to defensively peek for and
unwrap the {"value": ...} wrapper so session_id, turn_count, max_turns,
pending_function_calls, last_assistant, and updated_at_ms are read from the
inner object when present.
---
Nitpick comments:
In `@src/cli.rs`:
- Around line 90-95: The --new flag currently lacks a clap conflict declaration
so users can pass both --session-id and --new; update the #[arg(...)] attribute
on the pub new: bool field to include conflicts_with = "session_id" (i.e., add
conflicts_with = "session_id" to the #[arg(...)] on the new field) so clap will
produce a clear error when both session_id and new are supplied.
In `@src/payload.rs`:
- Around line 421-444: The test suite lacks coverage for the new .filter(|s|
!s.is_empty()) branch in build_run_payload; add a test (or extend
build_run_payload_preserves_system_prompt_override) that calls build_run_payload
with RunPayloadParams.system_prompt set to Some("".into()) and assert that
payload.get("system_prompt").is_none() to ensure empty-string system prompts are
omitted; reference the build_run_payload function and the RunPayloadParams
struct so the assertion targets the same payload map shape as the existing
tests.
- Around line 105-122: build_run_payload now omits the "system_prompt" key when
RunPayloadParams.system_prompt is None or Some("") which requires downstream
code to treat a missing key as the same as None/empty; update any consumers
(harness, router, session persistence) that read payload["system_prompt"] to
handle absence by falling back to the canonical prompt (i.e., treat missing key
as None/empty) and add a short note to the API/README that Some("") is treated
as clearing the override so callers can rely on omission semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afd25e3e-e69b-405a-9b1b-bd32b5e30f31
📒 Files selected for processing (5)
README.mdconfig.example.yamlsrc/app.rssrc/cli.rssrc/payload.rs
| fn status_session<R: CommandRunner, W: Write>( | ||
| client: &IiiClient<R>, | ||
| args: StatusArgs, | ||
| out: &mut W, | ||
| ) -> Result<()> { | ||
| let value = client | ||
| .trigger( | ||
| "state::get", | ||
| build_state_get_payload("agent", &format!("session/{}/turn_state", args.session_id)), | ||
| 5_000, | ||
| ) | ||
| .context("load durable turn state")?; | ||
| print_session_status(&value, out) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate state::get handler / contract to confirm response shape.
rg -nP -C5 '"state::get"|fn\s+state_get|state::get\b' --type=rust
fd -e md -e rst | xargs rg -nP -C3 'state::get' 2>/dev/nullRepository: rohitg00/iii-code
Length of output: 2679
🏁 Script executed:
# Find print_session_status implementation - it's defined somewhere in the file
rg -n "fn print_session_status" --type=rust
# Find extract_session_messages implementation
rg -n "fn extract_session_messages" --type=rust
# Look for test fixtures or test data showing state::get response format
rg -n "state::get|turn_state|session.*message" --type=rust -B2 -A2 | head -100Repository: rohitg00/iii-code
Length of output: 4688
🏁 Script executed:
# Search for tests that show actual response structures
rg -n "#\[test\]|#\[tokio::test\]" --type=rust -A 20 | grep -A 20 "state::get\|turn_state\|session"Repository: rohitg00/iii-code
Length of output: 14164
🏁 Script executed:
# View print_session_status implementation
sed -n '1326,1380p' src/app.rs
# View extract_session_messages implementation
sed -n '1404,1425p' src/app.rsRepository: rohitg00/iii-code
Length of output: 2519
🏁 Script executed:
# Search for tests that exercise status_session or print_session_status
rg -n "status_session\|print_session_status\|turn_state" --type=rust -B3 -A5Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Check if there are integration tests or test fixtures showing state::get responses
rg -n "session.*turn_state\|turn_state.*json\|status.*test" --type=rust -B2 -A3Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Search for mock responses or test fixtures showing response structure
rg -n "\"value\"\s*:" --type=rust -B1 -A1 | head -50
# Look for any documentation about state::get
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rs" \) | xargs rg -l "state::get\|turn_state" 2>/dev/null | head -10Repository: rohitg00/iii-code
Length of output: 434
🏁 Script executed:
# Check if status_session is actually tested or used anywhere
rg -n "Status|status.*session" --type=rust src/cli.rs src/app.rs | grep -i "status\|Status"Repository: rohitg00/iii-code
Length of output: 639
🏁 Script executed:
# Check StatusArgs and command parsing
sed -n '304,315p' src/cli.rs
# Check if there are any tests for the status command behavior
rg -n "#\[test\]" --type=rust -A 30 src/app.rs | grep -A 30 "status"Repository: rohitg00/iii-code
Length of output: 10320
🏁 Script executed:
# Look for documentation or comments about state::get response format
rg -n "state::get\|turn_state" --type=rust -B2 -A2 src/app.rs | head -80Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Look for any explicit test data or documentation showing response structure
rg -n "turn_state\|session.*status\|\"messages\"" --type=rust src/app.rs | head -40
# Search for where state::set is called to understand payload patterns
rg -n "state::set\|build_state_set" --type=rust -B2 -A2Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Look at the build_state_get_payload to understand contract
rg -n "build_state_get_payload" --type=rust src/payload.rs -B2 -A5Repository: rohitg00/iii-code
Length of output: 740
🏁 Script executed:
# Check if there are any real examples or integration tests showing state::get response
find . -type f -name "*.rs" -o -name "*.json" -o -name "*.txt" | xargs grep -l "session.*turn_state\|turn_state.*json" 2>/dev/null
# Look for chat session status output or expected format
rg -n "session:" --type=rust -B3 -A3 src/app.rs | head -50Repository: rohitg00/iii-code
Length of output: 1784
🏁 Script executed:
# Search for comments or docs about state get/set behavior
rg -n "state::get\|state::set" --type=rust -B5 -A1 src/app.rs | grep -E "//|fn build|trigger" | head -30Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Search for actual test mocking of state::get responses or real response examples
rg -n "MockRunner::ok.*session\|MockRunner::ok.*state" --type=rust -A1 | head -60
# Check if there's any documentation in comments about response envelope
rg -n "//" --type=rust src/app.rs | grep -i "state\|get\|value\|envelope" | head -20Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Look at how load_last_cwd_session is tested to understand response shape
sed -n '2100,2200p' src/app.rsRepository: rohitg00/iii-code
Length of output: 3829
🏁 Script executed:
# Search for any tests of the status command or turn_state
rg -n "turn_state\|Status\(" --type=rust src/app.rs -B2 -A4
# Check if there are integration tests or fixtures
find . -type f \( -name "*test*" -o -name "*fixture*" \) | head -20Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Look at the chat session commands that use state::get to see if there's a pattern
sed -n '745,800p' src/app.rsRepository: rohitg00/iii-code
Length of output: 1523
🏁 Script executed:
# Look for any code that sets turn_state to understand what value shape is stored
rg -n "turn_state\|session.*turn_state" --type=rust -B3 -A3
# Check if there are any references to the response envelope in comments
rg -n "envelope\|wrapped\|{\"value" --type=rust src/app.rs | head -20Repository: rohitg00/iii-code
Length of output: 190
🏁 Script executed:
# Look at other state::get callers to see if there's a consistent pattern
rg -n "\.trigger.*state::get" --type=rust -A5 src/app.rsRepository: rohitg00/iii-code
Length of output: 306
🏁 Script executed:
# Check the repair_session function which also uses state::get for messages
sed -n '900,920p' src/app.rs
# Also check load_session_messages which uses state::get
sed -n '1379,1402p' src/app.rsRepository: rohitg00/iii-code
Length of output: 1345
🏁 Script executed:
# Look for any definition or schema documentation
find . -name "*.md" -o -name "*.txt" -o -name "*.toml" | xargs grep -l "state::get\|turn_state" 2>/dev/nullRepository: rohitg00/iii-code
Length of output: 73
🏁 Script executed:
# Check README for state::get documentation
grep -n "state::get\|turn_state" README.md -B2 -A2
# Also search the entire README
cat README.md | grep -i "state\|get\|response\|envelope"Repository: rohitg00/iii-code
Length of output: 629
🏁 Script executed:
# Let me analyze the code pattern. Look at extract_session_messages more carefully
# and compare it to print_session_status
sed -n '1404,1420p' src/app.rsRepository: rohitg00/iii-code
Length of output: 524
🏁 Script executed:
# Check if there are any tests for status_session or print_session_status
rg -n "status\|Status" --type=rust src/app.rs | grep -i test
# Look for chat command tests that might exercise status
sed -n '2100,2250p' src/app.rs | grep -A 20 testRepository: rohitg00/iii-code
Length of output: 5263
🏁 Script executed:
# Key question: does extract_session_messages also need envelope unwrap?
# Let's trace what load_session_messages returns
rg -n "extract_session_messages\|load_session_messages" --type=rust -B2 -A2 src/app.rs | head -50Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Final check: look for any chat session status command tests
rg -n "chat.*status\|/status" --type=rust src/app.rs -B2 -A5Repository: rohitg00/iii-code
Length of output: 43
🏁 Script executed:
# Check the chat shell command dispatcher for status handling
sed -n '500,600p' src/app.rs | grep -A 20 "status"Repository: rohitg00/iii-code
Length of output: 756
print_session_status should defensively unwrap potential state::get envelope, consistent with other handlers in this file.
Other consumers of state::get responses defensively peek inside a {"value": ...} wrapper (see load_last_cwd_session at lines 448–452 and extract_session_messages at lines 1404–1415). However, print_session_status calls value.as_object() on the raw response and immediately reads session_id, turn_count, max_turns, pending_function_calls, last_assistant, and updated_at_ms at the top level. If state::get returns the stored turn state inside a value envelope, the function will match the outer object but silently render missing fields as "unknown"—a misleading status output.
Apply the same defensive pattern already used elsewhere in the file:
fn print_session_status<W: Write>(value: &Value, out: &mut W) -> Result<()> {
- let Some(object) = value.as_object() else {
+ let inner = value.get("value").unwrap_or(value);
+ let Some(object) = inner.as_object() else {
return print_json(value, out);
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app.rs` around lines 763 - 776, status_session/print_session_status must
handle the state::get envelope the same way as load_last_cwd_session and
extract_session_messages: check if the returned JSON is an object with a "value"
key and, if so, set value = value["value"] before accessing fields; update
print_session_status (or the caller status_session) to defensively peek for and
unwrap the {"value": ...} wrapper so session_id, turn_count, max_turns,
pending_function_calls, last_assistant, and updated_at_ms are read from the
inner object when present.
1 participant
Summary
Validation
Summary by CodeRabbit
doctoroutput/exit behavior.--newoption for chat sessions; export/compact output options documented.