fix(approval): classify resolved decide races#4242
Open
YOMXXX wants to merge 4 commits into
Open
Conversation
Contributor
|
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)
📝 WalkthroughWalkthroughAdds typed approval decision outcomes through store, gate, and RPC handling, classifies benign approval replay messages in observability, and updates localized documentation links. ChangesApproval Decision Outcome Classification
Documentation Link Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
added
bug
rust-core
![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
🧹 Nitpick comments (1)
tests/tool_registry_approval_raw_coverage_e2e.rs (1)
1295-1311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe expired RPC path still isn't pinned end-to-end.
This adds the unknown-id assertion, but the same E2E still never drives
openhuman.approval_decideagainst a request that became terminal via TTL expiry. The store unit test covers that branch; the JSON-RPC contract introduced in this PR does not. Add an expired-request assertion here too so duplicate, expired, and unknown stay distinct at the RPC layer.🤖 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 `@tests/tool_registry_approval_raw_coverage_e2e.rs` around lines 1295 - 1311, The approval_decide E2E coverage in the tool registry test still misses the TTL-expired terminal case, so add an assertion that drives openhuman.approval_decide against a request that has expired and verifies its RPC error is distinct from both “approval request not found” and “no pending approval”. Reuse the existing rpc and error_message helpers in the test around the approval_decide flow, and add the expired-request setup/assertion near the current unknown-id check so duplicate, expired, and unknown paths are all pinned end-to-end.
🤖 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/core/observability.rs`:
- Around line 557-559: The replay matcher in
is_approval_already_resolved_message is too broad because it only checks for the
generic “no pending approval found for request_id” text. Tighten the helper to
require the replay-specific “already decided as” suffix as well, so only the
intended replay path is matched. Also add a negative test around the
observability matching logic to ensure a prefix-only message does not get
classified as already resolved.
In `@src/openhuman/approval/gate.rs`:
- Around line 719-740: The non-`Decided` branch in `approval::gate` only logs,
leaving any parked waiter unresolved until TTL. Update the `AlreadyTerminal` and
`NotFound` handling in the decision path so the local waiter is released
immediately: in `AlreadyTerminal`, forward the persisted decision to the parked
future, and in `NotFound`, drop the sender so the existing cleanup path can run.
Use the existing `ApprovalDecisionOutcome` match in `gate.rs` to keep the
behavior consistent with the `Decided` flow.
---
Nitpick comments:
In `@tests/tool_registry_approval_raw_coverage_e2e.rs`:
- Around line 1295-1311: The approval_decide E2E coverage in the tool registry
test still misses the TTL-expired terminal case, so add an assertion that drives
openhuman.approval_decide against a request that has expired and verifies its
RPC error is distinct from both “approval request not found” and “no pending
approval”. Reuse the existing rpc and error_message helpers in the test around
the approval_decide flow, and add the expired-request setup/assertion near the
current unknown-id check so duplicate, expired, and unknown paths are all pinned
end-to-end.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60ccf6f7-dcd2-4250-a3ed-fb09b692206f
📒 Files selected for processing (7)
src/core/observability.rssrc/openhuman/approval/gate.rssrc/openhuman/approval/mod.rssrc/openhuman/approval/rpc.rssrc/openhuman/approval/store.rssrc/openhuman/approval/types.rstests/tool_registry_approval_raw_coverage_e2e.rs
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
1 participant
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
approval_decidecalls as benign expected errors while preserving Sentry signal for genuine missing registrations.ApprovalAlreadyResolvedobservability bucket for the knownno pending approval foundrace shape.upstream/main.Problem
no pending approval found for request_id '<uuid>'.store::decidereturnsOk(None)for three different states: already decided, lazily expired, or unknown id.Solution
ApprovalDecisionOutcomeanddecide_with_statusso the store/gate can classifyDecided,AlreadyTerminal, andNotFoundwithout breaking the existingdecidewrapper.approval_decideto return classifiableno pending approval found ... (already decided as ...)only for terminal replays, and a distinctapproval request not found ...error for never-persisted ids.ExpectedErrorKind::ApprovalAlreadyResolvedfor the benign replay string only.Submission Checklist
## Related— N/A: no coverage-matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md) — N/A: approval RPC error classification only.Closes #NNNin the## RelatedsectionImpact
approval_decidefailures are demoted as expected; never-persisted request ids remain reportable under a different message.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4176-approval-no-pending8af9e22d8d75483a8af7c9b38ba15fb3d9f8be12Validation Run
pnpm --filter openhuman-app format:check— N/A: no app TS/CSS changes.pnpm typecheck— N/A: no frontend TypeScript changes.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib decide_with_status_distinguishes_terminal_expired_and_missing_rowsfailed before implementation on missingdecide_with_status/ApprovalDecisionOutcome.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib classifies_resolved_approval_decide_race_as_expectedfailed before implementation on missingApprovalAlreadyResolved.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib classifies_resolved_approval_decide_race_as_expectedfailed before review fix because prefix-only messages were over-classified.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib decide_with_status_releases_waiter_for_already_terminal_rowfailed before review fix because the parked waiter timed out.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib decide_with_status_distinguishes_terminal_expired_and_missing_rowsGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib classifies_resolved_approval_decide_race_as_expectedGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib decide_with_status_releases_waiter_for_already_terminal_rowGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --test tool_registry_approval_raw_coverage_e2e approval_rpc_decision_paths_persist_always_allow_and_recent_auditcargo fmt --manifest-path Cargo.toml --checkgit diff --checkGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
approval request not found.Parity Contract
ApprovalDecided, resolve waiters, and persistApproveAlwaysForTool; the existingdecidewrapper still returnsOption<PendingApproval>for existing call sites.Duplicate / Superseded PR Handling
Summary by CodeRabbit