fix(llm): wire provider_request_guard through both generation entry points - #395
Conversation
…oints #385 added `provider_request_guard` but connected it backwards on both sides: - `generate_chat_response_with_provenance` forwards the name at line 427 without declaring it, so every call raised `NameError: name 'provider_request_guard' is not defined`. - `generate_chat_response` declares the parameter but omits it from its delegation, so a caller's guard was accepted and silently dropped. The NameError is live in production. `run_tool_loop` calls the provenance entry point, so the tool loop dies, playbook extraction returns no structured output, and it surfaces as `ExtractorExecutionError: Playbook extraction failed without structured output` (Sentry PYTHON-FASTAPI-PW, -PV, -NQ). Ruff already flagged this on main — `F821 Undefined name` twice, plus `ARG002` for the unused declaration. Static analysis caught it; nothing gated on it. Adds a guard test for the class, not just these two sites: it asserts both entry points declare every forwarded option and that each forwarded name is a local rather than an accidental global. All 5 tests fail on unpatched main.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe chat response path now accepts and forwards ChangesProvider guard forwarding
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Review noted the guard listed the forwarded options in a hand-authored tuple, so catching a future drift still depended on someone remembering to update that list — the same manual sync step that produced the bug it guards. The options are now derived from the two signatures and compared for parity, so an option added to one entry point and forgotten in the other fails here with no one having to maintain a literal. The specific `provider_request_guard` regression stays pinned by its own test. Verified against origin/main content: 5 of 6 fail. The survivor is the locals check, which derives from the signature it inspects and is therefore vacuous when the parameter is absent entirely — parity is what covers that case, and it fails. Removing the parameter from only one signature (simulated drift) also fails the suite.
![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
🤖 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 `@tests/server/llm/test_provenance_forwarding.py`:
- Around line 39-65: Replace the signature-only tests
test_both_entry_points_declare_the_same_options and
test_forwarded_names_are_locals_not_globals with mock-based, parameterized
forwarding tests that iterate over _options(...) and pass a distinct non-None
sentinel for each option, asserting every value reaches the delegated
generate_chat_response_with_provenance call. Keep the existing
provider_request_guard regression test unchanged and ensure the tests validate
runtime forwarding rather than co_varnames or signature parity alone.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0be7d71d-bcd1-4db6-9ff7-89f4b03a6c36
📒 Files selected for processing (1)
tests/server/llm/test_provenance_forwarding.py
CodeRabbit was right on both counts. Signature parity still passes when both entry points declare an option and one drops it in the call — which is exactly what `generate_chat_response` did with `provider_request_guard`. And the `co_varnames` check was tautological once the options were derived from the signature: every declared parameter is a local by construction, so it asserted nothing and never inspected the forwarding code. Replaces it with two parameterized tests that pass a distinct sentinel for each derived option and assert it arrives — at the delegated call for the plain entry point, and at `_make_request` for the provenance one, which executes the real forwarding block. Proven against three states: 19 pass on this branch; 12 fail on origin/main content; and removing ONLY the delegation line, leaving the signature intact, fails `test_plain_entry_point_forwards_every_declared_option[provider_request_guard]` — the drift the previous version would have missed.
1 participant
Problem
provider_request_guardwas added by #385 and wired backwards on both sides:generate_chat_response_with_provenanceNameErroron every callgenerate_chat_responseThe NameError is live in production right now.
run_tool_loopcalls the provenanceentry point, so the tool loop dies mid-flight, playbook extraction returns no structured
output, and it surfaces as:
Sentry:
PYTHON-FASTAPI-PW(the NameError, root cause), withPYTHON-FASTAPI-PVandPYTHON-FASTAPI-NQas the downstream extraction failures.Ruff already caught this on
main—F821 Undefined name 'provider_request_guard'twice (lines 427, 428) plus
ARG002for the unused declaration at line 322. The bug wasstatically detectable before it shipped; nothing gated on the result.
Fix
Two lines: declare the parameter on the provenance entry point, and pass it through in
the plain entry point's delegation.
Verification
Reproduced the exact production error against
origin/main, then confirmed it resolved(run with the repo first on
PYTHONPATH— importing the installed editable copy insteadsilently tests the wrong file):
generate_chat_response_with_provenance(...)origin/mainNameError: name 'provider_request_guard' is not definedmain, pass here.tests/server/llm: 654 passed, 61 skipped.The test targets the class, not just these two sites: it asserts both entry points
declare every forwarded option, and that each forwarded name is a local rather than an
accidental global lookup. A future option pasted into one signature and not the other
fails here instead of in production.
Summary by CodeRabbit
Bug Fixes
Tests