Repository navigation
[codex] Consume runtime approval decisions - #95
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (5)
📝 WalkthroughWalkthroughAdds session-local runtime-message handling for approval decisions: a typed consumer, deterministic decision publishing with first-write-wins semantics, a DuplicateRuntimeMessageError, Session wiring for a runtime bus and approval cursor, and tests plus docs describing the flow and payload schema. ChangesApproval-Decision Runtime Messaging
Sequence DiagramsequenceDiagram
actor User
participant SessionManager
participant SessionRuntimeBus as "Session Runtime Bus"
participant ApprovalDecisionConsumer as "ApprovalDecisionConsumer"
participant ApprovalCoordinator as "ApprovalCoordinator"
User ->> SessionManager: submit_approval_response(session_id, request_id, approved, feedback, scope)
SessionManager ->> SessionRuntimeBus: publish(APPROVAL_DECISION with deterministic message_id)
SessionManager ->> ApprovalDecisionConsumer: consume(bus, cursor)
ApprovalDecisionConsumer ->> SessionRuntimeBus: read messages after cursor
ApprovalDecisionConsumer ->> ApprovalDecisionConsumer: validate payload (request_id, approved, feedback?, scope, session match)
alt payload valid & session matches
ApprovalDecisionConsumer ->> ApprovalCoordinator: respond(ApprovalResponse)
ApprovalCoordinator -->> ApprovalDecisionConsumer: applied / acknowledged
ApprovalDecisionConsumer -->> SessionManager: ApprovalDecisionConsumptionResult(applied_request_ids=[...])
SessionManager ->> SessionManager: update cursor, pending_approval, set approval_event
else invalid / cross-session / coordinator rejects
ApprovalDecisionConsumer -->> SessionManager: ApprovalDecisionConsumptionResult(skipped_message_ids=[...])
SessionManager ->> SessionManager: advance cursor only
end
SessionManager ->> User: return ApprovalResponse or None (success if applied_request_ids contains request_id)
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 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. Review rate limit: 0/1 reviews remaining, refill in 27 minutes and 1 second.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/approval/runtime_messages.py`:
- Around line 116-119: The membership check for scope_value against
_APPROVAL_SCOPES can raise TypeError for unhashable values (e.g.,
typed_payload["scope"] == []); update the logic in the block that reads
typed_payload, scope_value and uses logger.warning/message_id so it first guards
the value (e.g., ensure scope_value is a simple hashable like str or wrap the
membership test in a try/except TypeError) and treat any TypeError or non-string
as invalid by logging via logger.warning("approval_decision %r has invalid
scope", message_id) and returning None; keep the same log/message_id and
_APPROVAL_SCOPES identifiers so only the membership check behavior changes.
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 1452-1465: The publish can raise ValueError on duplicate message
during retries; make the approval publish idempotent by catching ValueError
around the session.runtime_message_bus.publish(...) call (the RuntimeMessage
with message_id f"approval_decision:{session_id}:{request_id}") and treating
that specific duplicate-error as success (i.e., log/debug and continue) before
calling self._consume_approval_decisions_for_session(session); ensure you only
suppress the duplicate-existence error and re-raise other exceptions so the
read-after-write path remains intact.
🪄 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: f2d4794a-8989-4705-846f-7dfad7b62b97
📒 Files selected for processing (7)
coding-agent/docs/AGENTKIT-ARCHITECTURE.mdcoding-agent/docs/CODING-AGENT-ARCHITECTURE.mdcoding-agent/src/agentkit/runtime/messages.pycoding-agent/src/coding_agent/approval/__init__.pycoding-agent/src/coding_agent/approval/runtime_messages.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/coding_agent/test_approval_runtime_messages.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/src/coding_agent/ui/http_server.py (1)
748-762:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn 500 for runtime-message failures instead of echoing them as bad input.
submit_approval_response()can now fail with server-side runtime-message errors, but this blanketexcept Exceptionturns them into400responses and sends the internal message text back to the client. That misclassifies a backend inconsistency as a caller mistake and can leak internal session/request identifiers. Catch only expected validation errors here, and log unexpected failures as500s.Proposed fix
- except Exception as exc: - raise HTTPException(status_code=400, detail=str(exc)) from exc + except (ValueError, TypeError) as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + except Exception as exc: + logger.exception( + "Unexpected approval submission failure for session %s", + session_id, + ) + raise HTTPException(status_code=500, detail="Internal server error") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 748 - 762, The except block around the call to session_manager.submit_approval_response should stop turning all exceptions into 400s and avoid echoing internal error text; catch only expected validation/known client errors (e.g., whichever validation exception your codebase uses) and re-raise them as HTTPException(status_code=400), but for any other unexpected Exception from submit_approval_response or session_manager, log the exception and raise HTTPException(status_code=500, detail="Internal server error") instead; keep existing handling for SessionOwnershipConflictError and HTTPException, and reference submit_approval_response, session_manager, SessionOwnershipConflictError and approval_response when locating the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 1405-1414: The helper currently advances
session.approval_decision_cursor immediately from the
ApprovalDecisionConsumer.consume() result even when decisions were skipped
because their target request doesn't exist yet; change the logic so you only
advance the cursor and set session.pending_approval / session.approval_event
when the consumer actually applied a decision to an existing request.
Concretely: inspect the consume() outcome (e.g., result.applied_request_ids and
any per-decision status returned by ApprovalDecisionConsumer.consume()) and
distinguish "request not found yet" from "already applied"; if a decision was
skipped due to "request not found", do not update
session.approval_decision_cursor or return success—keep the cursor unchanged so
the decision remains replayable until ApprovalCoordinator.add_request(...)
attaches the request, and only then update session.approval_decision_cursor and
set session.pending_approval = session.approval_coordinator.projection() /
session.approval_event when a real application occurred; apply the same change
to the analogous block around the branch handling lines 1468-1482.
---
Outside diff comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 748-762: The except block around the call to
session_manager.submit_approval_response should stop turning all exceptions into
400s and avoid echoing internal error text; catch only expected validation/known
client errors (e.g., whichever validation exception your codebase uses) and
re-raise them as HTTPException(status_code=400), but for any other unexpected
Exception from submit_approval_response or session_manager, log the exception
and raise HTTPException(status_code=500, detail="Internal server error")
instead; keep existing handling for SessionOwnershipConflictError and
HTTPException, and reference submit_approval_response, session_manager,
SessionOwnershipConflictError and approval_response when locating the code to
change.
🪄 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: 35665b11-ec44-4d1f-94cf-e528c7a7076f
📒 Files selected for processing (5)
coding-agent/src/coding_agent/approval/__init__.pycoding-agent/src/coding_agent/approval/runtime_messages.pycoding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_http_server.py
✅ Files skipped from review due to trivial changes (1)
- coding-agent/src/coding_agent/approval/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/src/coding_agent/approval/runtime_messages.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
coding_agent.approvalruntime-message consumer for product-ownedapproval_decisionmessagesSessionManager.submit_approval()through publish/consume ofapproval_decisionbefore applying the decision toApprovalCoordinatorPipelineContextfor HTTP session turns while keeping agentkit from interpreting approval decisionsVerification
uv run pytest tests/coding_agent/test_approval_runtime_messages.py -q(red first: missingcoding_agent.approval.runtime_messages; green after implementation)uv run pytest tests/coding_agent/test_approval_runtime_messages.py tests/coding_agent/test_pipeline_adapter.py tests/coding_agent/plugins/test_approval.py tests/agentkit/runtime/test_pipeline.py tests/agentkit/runtime/test_runtime_messages.py -q(90 passed)uv run pytest tests/agentkit/runtime/test_pipeline.py tests/agentkit/runtime/test_runtime_messages.py -q(35 passed)uv run pytest tests/coding_agent/ -q -k "not TestGoldenFixtures and not test_http_server_legacy_session_shims_removed"(460 passed, 5 deselected)uv run basedpyright src/coding_agent/approval/runtime_messages.py src/coding_agent/approval/__init__.py src/coding_agent/ui/session_manager.py src/agentkit/runtime/messages.py tests/coding_agent/test_approval_runtime_messages.py --level error(0 errors)uv run python -m py_compile src/coding_agent/approval/runtime_messages.py src/coding_agent/approval/__init__.py src/coding_agent/ui/session_manager.py src/agentkit/runtime/messages.py tests/coding_agent/test_approval_runtime_messages.pygit diff --checkKnown Existing Failures
tests/coding_agent/ -qstill excludes the threeTestGoldenFixturescases becausecoding-agent/data/eval/golden/parent-child-subagent-001.{jsonl,yaml}is absent.tests/coding_agent/ -qstill excludestest_http_server_legacy_session_shims_removedbecause currenthttp_server.pystill contains legacy session shims outside this PR.Summary by CodeRabbit
Documentation
New Features
Refactor
Tests