Conversation
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughWhen governance returns ChangesGovernance Decision Context Cleanup
sequenceDiagram
participant EvaluateGovernanceRequest
participant PostLLMHook
participant PostMCPHook
EvaluateGovernanceRequest->>EvaluateGovernanceRequest: clear governanceRejectedContextKey (on DecisionAllow)
EvaluateGovernanceRequest->>PostLLMHook: call PostLLMHook without rejected flag
EvaluateGovernanceRequest->>PostMCPHook: call PostMCPHook without rejected flag
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
danpiths
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Confidence Score: 5/5The change is a single, well-scoped addition inside a mutually exclusive branch; it cannot interfere with the rejection-setting path and uses a thread-safe, nil-guarded API call already established across the codebase. The fix clears a stale boolean flag in an Allow-only branch that could never conflict with the flag-setting logic above it. ClearValue is mutex-protected and sets the map entry to nil rather than deleting it, so the type-assertion check in PostLLMHook correctly falls through. No correctness, security, or data-loss risks are introduced. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix: clear `governanceRejectedContextKey..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
915f2d4 to
81a9b2a
Compare
…ance req so that fallbacks account for budgets/rate limits
81a9b2a to
268e30c
Compare
Merge activity
|
…ce req so that fallbacks account for budgets/rate limits (#3645) ## Summary When a primary LLM request fails and a fallback retry succeeds, the governance plugin was leaving a stale rejection flag in the context. This caused `PostLLMHook` to skip budget and rate-limit ID collection for the successful fallback attempt, as it incorrectly treated the request as rejected. ## Changes - Clear the `governanceRejectedContextKey` flag from the context whenever a `DecisionAllow` result is reached, ensuring that any rejection flag set during a prior failed attempt does not persist into a successful fallback retry. ## Type of change - [x] Bug fix ## Affected areas - [x] Plugins ## How to test Trigger a scenario where a primary provider attempt fails governance and a fallback attempt succeeds. Verify that `PostLLMHook` correctly collects budget and rate-limit IDs for the successful fallback response rather than skipping collection due to the stale rejection flag. ```sh go test ./plugins/governance/... ``` ## Breaking changes - [x] No ## Security considerations No security implications. This fix ensures accurate budget and rate-limit tracking, which could prevent unintended bypasses of usage accounting on fallback paths. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
3 participants
Summary
When a primary LLM request fails and a fallback retry succeeds, the governance
plugin was leaving a stale rejection flag in the context. This caused
PostLLMHookto skip budget and rate-limit ID collection for the successfulfallback attempt, as it incorrectly treated the request as rejected.
Changes
governanceRejectedContextKeyflag from the context whenever aDecisionAllowresult is reached, ensuring that any rejection flag set duringa prior failed attempt does not persist into a successful fallback retry.
Type of change
Affected areas
How to test
Trigger a scenario where a primary provider attempt fails governance and a
fallback attempt succeeds. Verify that
PostLLMHookcorrectly collects budgetand rate-limit IDs for the successful fallback response rather than skipping
collection due to the stale rejection flag.
go test ./plugins/governance/...Breaking changes
Security considerations
No security implications. This fix ensures accurate budget and rate-limit
tracking, which could prevent unintended bypasses of usage accounting on
fallback paths.
Checklist
docs/contributing/README.mdand followed the guidelines