Skip to content
PR Quorum
Sign inStart free
← Review gallery
HKUDS/Vibe-Trading #676openreviewed 5h ago

fix(session): enforce lifecycle and concurrent-run invariants

by tyj147454413-cmd+456 54 filesreviewed in 1m 28sView PR on GitHub
minor notesThe panel surfaced minor issues worth a look before merge.3 findings

Reviewer panel

Correctness
deepseek/deepseek-v4-pro
2 found
Security
deepseek/deepseek-v4-pro
0 found
Architecture
deepseek/deepseek-v4-pro
1 found

Findings

01Mmedconf 95%

Test helper _FakeMessage incompatible with _convert_messages_to_history

agent/tests/test_session_service_lifecycle.py

The helper class _FakeMessage provides role and content as plain attributes, but SessionService._convert_messages_to_history calls msg.get("content", "") and {**msg, "content": kept} on each message. Because _FakeMessage objects do not implement get and are not mappings, any test that feeds _FakeMessage instances into _convert_messages_to_history will raise an AttributeError (or TypeError for the dict‑unpack) and never exercise the intended logic. This means the new budget‑cap and fallback behaviour is not actually verified by the test suite, even though the PR claims 9 tests passed.

02Mmedconf 92%

Cancelled then failed fallthrough assigns premature terminal event

agent/src/session/service.py:220

When result.get("status") returns "cancelled", mark_cancelled is called and attempt.status becomes CANCELLED. Then update_attempt persists it. After the persistence block, the code checks attempt.status and falls through to attempt.failed only when the status is NOT COMPLETED and NOT CANCELLED. That path works correctly. However, if mark_cancelled succeeds but a subsequent step (e.g., self.store.update_attempt(attempt)) raises, the except clause at line 230-233 calls attempt.mark_failed(error=...) which overwrites the status to FAILED. This is handled and signals a failure. The more subtle issue is that within the try block after mark_cancelled, attempt.run_dir = result.get("run_dir") (line 219) and attempt.metrics = result.get("metrics") (line 220) are assigned before self.store.update_attempt(attempt) and the terminal event emission. If either of those assignments triggers an attribute assignment error (unlikely for a dataclass but theoretically possible due to descriptor issues or type mismatch), the except handler will catch it and call mark_failed, which again chains. While this is defensive, the more concrete concern is that attempt.run_dir and attempt.metrics are populated unconditionally after the status branch. For a "cancelled" status, we end up writing error in mark_cancelled, then we possibly write run_dir and metrics to the attempt (which may be stale or None from the result), and then persist and emit. This ordering means the cancelled attempt carries a run_dir from the agent, which might be a path that is later cleaned up or shared with another attempt, but the cancelled attempt is not expected to have a run_dir in the default case. This is a minor data correctness issue.

03Llowconf 85%

Cancelled reply message may expose internal error string

agent/src/session/service.py:481

In _format_result_message, the CANCELLED branch returns attempt.error as the reply content. attempt.error is set by mark_cancelled(reason=...) to the free-form reason string, which usually contains internal details like "cancelled by user". While normally harmless, any future internal usage (e.g., passing structured errors) might leak internals to the user-facing reply. More importantly, if the reason parameter ever contains exceptions or tracebacks, the user will see them directly in the assistant message content, which could be an information leak. This is a low-severity data-safety concern.

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo