Repository navigation
feat: populate remote session result summary - #154
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR enriches ChangesSession Result Enrichment with Failure Details and Tool Activity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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)
coding-agent/tests/ui/test_http_server.py (1)
3518-3538: ⚡ Quick winAdd a contract test for failed turns without persisted failure details.
Current coverage validates explicit
last_failure_details, but not the fallback message path whenturn_status == "failed"and details are absent. Adding that case will lock down the new fallback behavior.Suggested additional test
+ async def test_session_result_uses_default_failure_details_when_missing( + self, client: AsyncClient + ) -> None: + session = register_session( + "result-failed-default-details", + execution_binding=CloudWorkspaceBinding( + workspace_url="docker://agent-ws-result/workspace", + workspace_id="ws-failed-default-result", + ), + ) + session.turn_status = "failed" + session.last_failure_details = None + + response = await client.get(f"/sessions/{session.id}/result") + + assert response.status_code == 200 + data = response.json() + assert ( + data["failure_details"] + == "Session turn failed; no failure details were recorded." + )🤖 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 `@coding-agent/tests/ui/test_http_server.py` around lines 3518 - 3538, Add a test case that covers the fallback message when a turn failed but no persisted details exist: duplicate or extend test_session_result_includes_recorded_failure_details to create a session via register_session (with CloudWorkspaceBinding), set session.turn_status = "failed" and set session.last_failure_details = None or "" (falsy), call GET /sessions/{session.id}/result, assert status_code == 200, data["status"] == "failed" and data["turn_status"] == "failed", and assert data["failure_details"] equals the expected fallback failure message produced by the result endpoint (the fallback that the code returns when last_failure_details is absent).
🤖 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 `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 1805-1812: The current _session_result_latest_turn can raise if
_session_result_tape, tape.snapshot() or extract_turns throws, causing
/sessions/{id}/result to 500; wrap the await _session_result_tape(session),
tape.snapshot() and extract_turns(...) calls in a try/except that catches
exceptions, logs a debug/error (including exception message) and returns None on
failure so the endpoint degrades gracefully and returns session metadata with
final_answer/verification_summary as None; modify _session_result_latest_turn to
handle these failures (and consider using _session_result_tape and extract_turns
names to locate the calls).
---
Nitpick comments:
In `@coding-agent/tests/ui/test_http_server.py`:
- Around line 3518-3538: Add a test case that covers the fallback message when a
turn failed but no persisted details exist: duplicate or extend
test_session_result_includes_recorded_failure_details to create a session via
register_session (with CloudWorkspaceBinding), set session.turn_status =
"failed" and set session.last_failure_details = None or "" (falsy), call GET
/sessions/{session.id}/result, assert status_code == 200, data["status"] ==
"failed" and data["turn_status"] == "failed", and assert data["failure_details"]
equals the expected fallback failure message produced by the result endpoint
(the fallback that the code returns when last_failure_details is absent).
🪄 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 Plus
Run ID: ff3734d8-d067-4bda-91f0-06c0250fbefd
📒 Files selected for processing (3)
coding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_http_server.py
| async def _session_result_latest_turn(session: Session) -> TurnTrace | None: | ||
| tape = await _session_result_tape(session) | ||
| if tape is None: | ||
| return None | ||
| turns = extract_turns(tape.snapshot()) | ||
| if not turns: | ||
| return None | ||
| return turns[-1] |
There was a problem hiding this comment.
Harden /result against tape read/parse failures.
On Line 1806 and Line 1809, an exception from tape restore/snapshot extraction will currently bubble up and turn /sessions/{id}/result into a 500. This endpoint should degrade gracefully and still return session metadata with final_answer/verification_summary as None.
Proposed fix
async def _session_result_latest_turn(session: Session) -> TurnTrace | None:
- tape = await _session_result_tape(session)
+ try:
+ tape = await _session_result_tape(session)
+ except Exception:
+ logger.exception(
+ "Failed to load tape for session result session_id=%s",
+ session.id,
+ )
+ return None
if tape is None:
return None
- turns = extract_turns(tape.snapshot())
+ try:
+ turns = extract_turns(tape.snapshot())
+ except Exception:
+ logger.exception(
+ "Failed to extract turns for session result session_id=%s",
+ session.id,
+ )
+ return None
if not turns:
return None
return turns[-1]🤖 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 `@coding-agent/src/coding_agent/ui/http_server.py` around lines 1805 - 1812,
The current _session_result_latest_turn can raise if _session_result_tape,
tape.snapshot() or extract_turns throws, causing /sessions/{id}/result to 500;
wrap the await _session_result_tape(session), tape.snapshot() and
extract_turns(...) calls in a try/except that catches exceptions, logs a
debug/error (including exception message) and returns None on failure so the
endpoint degrades gracefully and returns session metadata with
final_answer/verification_summary as None; modify _session_result_latest_turn to
handle these failures (and consider using _session_result_tape and extract_turns
names to locate the calls).
There was a problem hiding this comment.
Skipped this change after checking the endpoint contract. Tape read/parse failure means the auditable result source is unavailable or corrupt; returning metadata with null result fields would hide that failure. The endpoint should fail visibly until we add a dedicated result_extraction_error field or equivalent explicit partial-result contract.
There was a problem hiding this comment.
@delete-cloud, that's a solid rationale — surfacing a 500 is more honest than silently returning null fields when the authoritative result source is unavailable or corrupt. Understood, skipping this change.
Would you like me to open a follow-up GitHub issue to track adding a result_extraction_error field (or an equivalent explicit partial-result contract) to SessionResultResponse? That would make it easier to revisit graceful degradation once the contract is properly defined.
(◕‿◕✿) 🐇
✏️ Learnings added
Learnt from: delete-cloud
Repo: delete-cloud/Coding-Agent-Loop PR: 154
File: coding-agent/src/coding_agent/ui/http_server.py:1805-1812
Timestamp: 2026-05-14T06:43:40.163Z
Learning: In `coding-agent/src/coding_agent/ui/http_server.py`, the `/sessions/{id}/result` endpoint (`get_session_result`) intentionally lets tape read/parse errors propagate as 500s rather than degrading gracefully with null `final_answer`/`verification_summary` fields. The design decision is that a tape failure means the auditable result source is unavailable or corrupt, and silent null fields would hide that failure. Graceful degradation should only be added when a dedicated `result_extraction_error` field or an explicit partial-result contract is introduced in `SessionResultResponse`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
Tests