docs: document PreRequestHook routing phase, per-request vs per-attempt semantics, and updated plugin sequencing - #3935
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR documents PreRequestHook as a once-per-request routing-phase hook and updates architecture diagrams, plugin guides, Go examples, and provider-routing docs to show routing ordering, per-attempt semantics, fallback behavior, and validation after routing. ChangesPreRequestHook Introduction and Clarification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 unit tests (beta)
Comment |
|
|
Confidence Score: 5/5Documentation-only change; all behavioral claims verified against the Go implementation and safe to merge. Every documented API signature, constant, plugin order value, and behavioral claim was cross-checked against the live Go source. The two findings are cosmetic: a log message example with a spurious line break, and a routing example that reuses the governance plugin's reserved engine name for a custom plugin without explanation. Neither affects runtime behavior. docs/plugins/writing-go-plugin.mdx and docs/providers/provider-routing.mdx have minor inaccuracies worth a second look before publishing. Important Files Changed
Reviews (13): Last reviewed commit: "docs: update docs for perrequesthook" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
e8352f5 to
856c996
Compare
63d908a to
50e8ded
Compare
50e8ded to
7ad668e
Compare
ceeb945 to
a55b42c
Compare
53da3d4 to
fff3d21
Compare
a55b42c to
da3318a
Compare
05c2bc9 to
d3f5a32
Compare
2fd7c8c to
4edbac5
Compare
d3f5a32 to
3010c2b
Compare
![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
♻️ Duplicate comments (2)
docs/plugins/getting-started.mdx (1)
87-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLine 87 still collapses per-request and per-attempt hook semantics.
Please split this sentence so it explicitly says
PreRequestHook()runs once per top-level request, whilePreLLMHook()andPostLLMHook()run per provider attempt (including fallbacks).As per coding guidelines, “PreRequestHook is once per request; PreLLMHook / PostLLMHook remain per provider attempt.”
🤖 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 `@docs/plugins/getting-started.mdx` at line 87, Update the sentence describing hook execution to clearly separate per-request vs per-attempt semantics: state that PreRequestHook() runs once for the top-level request, while PreLLMHook() and PostLLMHook() run for each provider attempt (including fallbacks); locate the sentence mentioning "Hook Execution" and replace the combined line with two short clauses that reference PreRequestHook(), PreLLMHook(), and PostLLMHook() by name and explicitly note "once per request" for PreRequestHook() and "per provider attempt (including fallbacks)" for PreLLMHook() and PostLLMHook().Source: Coding guidelines
docs/plugins/writing-go-plugin.mdx (1)
155-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreRequestHook appears as part of v1.5.x+/v1.4.x skeleton API despite being v1.6.x+.
These skeleton blocks should either remove
PreRequestHookfrom older-version tabs or clearly annotate it as optional/v1.6.x+ only in-code (not just elsewhere on the page), to avoid copy/paste confusion.As per coding guidelines, “Provide a Go plugin skeleton and hook reference that correctly states PreRequestHook’s contract for v1.6.x+.”
Also applies to: 281-286
🤖 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 `@docs/plugins/writing-go-plugin.mdx` around lines 155 - 163, Update the Go plugin skeleton so PreRequestHook is not presented as part of older-version tabs: either remove the PreRequestHook function block from v1.4.x/v1.5.x skeletons or add an in-code annotation on the PreRequestHook declaration (the func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error) stating it is optional and only available in v1.6.x+; apply the same change to the other instance referenced (lines around the second occurrence) and ensure the comment explicitly marks the API contract as v1.6.x+ to prevent copy/paste confusion.Source: Coding guidelines
🤖 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 `@docs/architecture/core/plugins.mdx`:
- Around line 74-75: The diagram currently shows PreRequestHook producing a
blocking ShortCircuitError; update the diagram so PreRequestHook error outcomes
are non-blocking warnings that rejoin the normal flow (remove or repoint the
PreRequestHookCall --> ShortCircuitError transition), change the label from
“Return Error” to “Log Warning” (or similar), and route the error path back into
the subsequent normal node (e.g., the ValidateProvider/next pipeline node) so
execution continues per the PreRequestHook contract.
---
Duplicate comments:
In `@docs/plugins/getting-started.mdx`:
- Line 87: Update the sentence describing hook execution to clearly separate
per-request vs per-attempt semantics: state that PreRequestHook() runs once for
the top-level request, while PreLLMHook() and PostLLMHook() run for each
provider attempt (including fallbacks); locate the sentence mentioning "Hook
Execution" and replace the combined line with two short clauses that reference
PreRequestHook(), PreLLMHook(), and PostLLMHook() by name and explicitly note
"once per request" for PreRequestHook() and "per provider attempt (including
fallbacks)" for PreLLMHook() and PostLLMHook().
In `@docs/plugins/writing-go-plugin.mdx`:
- Around line 155-163: Update the Go plugin skeleton so PreRequestHook is not
presented as part of older-version tabs: either remove the PreRequestHook
function block from v1.4.x/v1.5.x skeletons or add an in-code annotation on the
PreRequestHook declaration (the func PreRequestHook(ctx *schemas.BifrostContext,
req *schemas.BifrostRequest) error) stating it is optional and only available in
v1.6.x+; apply the same change to the other instance referenced (lines around
the second occurrence) and ensure the comment explicitly marks the API contract
as v1.6.x+ to prevent copy/paste confusion.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: eafa93f0-dec4-4a1c-9eab-1df87fc209ae
📒 Files selected for processing (5)
docs/architecture/core/plugins.mdxdocs/plugins/getting-started.mdxdocs/plugins/sequencing.mdxdocs/plugins/writing-go-plugin.mdxdocs/providers/provider-routing.mdx
4edbac5 to
9f40efe
Compare
3010c2b to
5ec0e17
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/architecture/core/plugins.mdx (1)
74-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreRequestHook error path contradicts the non-blocking error contract.
The state diagram shows
PreRequestHookCall --> ShortCircuitError: Return Error, but PreRequestHook errors are non-blocking per the coding guidelines and schema definition. Errors should be logged as warnings and the pipeline should continue to the next plugin/validation stage, not short-circuit. Update the diagram to route error outcomes back into the normal flow (e.g.,RouteDecidedor a validation node) instead of branching toShortCircuitError.🤖 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 `@docs/architecture/core/plugins.mdx` around lines 74 - 75, The diagram incorrectly shows PreRequestHookCall leading to ShortCircuitError; update the flow so PreRequestHookCall error outcomes route back into the normal pipeline (for example to RouteDecided or the validation node) instead of ShortCircuitError, and annotate that PreRequestHook errors are non-blocking and should be logged as warnings (per the schema/guidelines) rather than short-circuiting the request.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@docs/architecture/core/plugins.mdx`:
- Around line 74-75: The diagram incorrectly shows PreRequestHookCall leading to
ShortCircuitError; update the flow so PreRequestHookCall error outcomes route
back into the normal pipeline (for example to RouteDecided or the validation
node) instead of ShortCircuitError, and annotate that PreRequestHook errors are
non-blocking and should be logged as warnings (per the schema/guidelines) rather
than short-circuiting the request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 082bf81f-550f-47d0-b1e8-d9248aa8e4b0
📒 Files selected for processing (5)
docs/architecture/core/plugins.mdxdocs/plugins/getting-started.mdxdocs/plugins/sequencing.mdxdocs/plugins/writing-go-plugin.mdxdocs/providers/provider-routing.mdx
9f40efe to
9b1c18b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/plugins/writing-go-plugin.mdx (1)
281-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t present
PreRequestHookas part of the v1.4.x skeleton.This v1.4.x sample shows
PreRequestHookas a standard hook, while the doc itself scopes it to v1.6.x+. In the v1.4.x tab, either remove it or mark it clearly as optional v1.6.x+ only.Suggested edit
-// PreRequestHook is called once per top-level request (routing phase) -// Mutations to req.Provider/req.Model/req.Fallbacks commit across fallbacks -func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error { - fmt.Println("PreRequestHook called") - return nil -} +// Optional in v1.6.x+ only: +// func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error { +// fmt.Println("PreRequestHook called") +// return nil +// }🤖 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 `@docs/plugins/writing-go-plugin.mdx` around lines 281 - 286, The v1.4.x example incorrectly shows PreRequestHook as a standard hook; update the v1.4.x skeleton to either remove the PreRequestHook function or explicitly mark it as optional and only available in v1.6.x+ (reference symbol: PreRequestHook, types: schemas.BifrostContext and schemas.BifrostRequest). Ensure the v1.4.x tab content and any surrounding text clearly state that PreRequestHook is a v1.6.x+ feature so readers won’t assume it exists in v1.4.x.Source: Coding guidelines
docs/plugins/getting-started.mdx (1)
87-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify hook lifecycle wording at this step.
Line 87 currently implies
PreRequestHook,PreLLMHook, andPostLLMHookall run “for each request,” which conflicts with the per-attempt semantics documented below. Make this line explicit:PreRequestHookruns once per top-level request;PreLLMHook/PostLLMHookrun per provider attempt (including fallbacks).Suggested edit
-3. **Hook Execution** - Calls `PreRequestHook()`, `PreLLMHook()` and `PostLLMHook()` for each request +3. **Hook Execution** - Calls `PreRequestHook()` once per top-level request, then `PreLLMHook()`/`PostLLMHook()` per provider attempt (including fallbacks)🤖 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 `@docs/plugins/getting-started.mdx` at line 87, Update the wording in the list item so it explicitly distinguishes hook lifecycles: state that PreRequestHook() is invoked once per top-level request, while PreLLMHook() and PostLLMHook() are invoked for each provider attempt (including fallbacks), and replace the phrase "for each request" with this clarified sentence referencing PreRequestHook, PreLLMHook, and PostLLMHook.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@docs/plugins/getting-started.mdx`:
- Line 87: Update the wording in the list item so it explicitly distinguishes
hook lifecycles: state that PreRequestHook() is invoked once per top-level
request, while PreLLMHook() and PostLLMHook() are invoked for each provider
attempt (including fallbacks), and replace the phrase "for each request" with
this clarified sentence referencing PreRequestHook, PreLLMHook, and PostLLMHook.
In `@docs/plugins/writing-go-plugin.mdx`:
- Around line 281-286: The v1.4.x example incorrectly shows PreRequestHook as a
standard hook; update the v1.4.x skeleton to either remove the PreRequestHook
function or explicitly mark it as optional and only available in v1.6.x+
(reference symbol: PreRequestHook, types: schemas.BifrostContext and
schemas.BifrostRequest). Ensure the v1.4.x tab content and any surrounding text
clearly state that PreRequestHook is a v1.6.x+ feature so readers won’t assume
it exists in v1.4.x.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fad54266-27f0-40fb-b46e-46d5d34a8512
📒 Files selected for processing (5)
docs/architecture/core/plugins.mdxdocs/plugins/getting-started.mdxdocs/plugins/sequencing.mdxdocs/plugins/writing-go-plugin.mdxdocs/providers/provider-routing.mdx
5ec0e17 to
13daf7a
Compare
0404470 to
252041c
Compare
13daf7a to
24abc35
Compare
24abc35 to
13daf7a
Compare
252041c to
0404470
Compare
13daf7a to
d803fb5
Compare
0404470 to
2aef19b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/plugins/getting-started.mdx (1)
87-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix lifecycle step wording to avoid semantic contradiction.
Line 87 currently says all three hooks run “for each request,” but
PreLLMHook()/PostLLMHook()are per-attempt (including fallbacks), whilePreRequestHook()is once per top-level request.Suggested edit
-3. **Hook Execution** - Calls `PreRequestHook()`, `PreLLMHook()` and `PostLLMHook()` for each request +3. **Hook Execution** - Calls `PreRequestHook()` once per top-level request, then `PreLLMHook()`/`PostLLMHook()` for each provider attempt (including fallbacks)As per coding guidelines,
PreRequestHookis once-per-request, whilePreLLMHook/PostLLMHookexecute per-attempt and re-run on fallbacks.🤖 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 `@docs/plugins/getting-started.mdx` at line 87, Update the lifecycle step wording to remove the contradictory "for each request" phrase: state that PreRequestHook() runs once per top-level request, while PreLLMHook() and PostLLMHook() run per attempt (and will re-run on fallbacks); edit the sentence referencing PreRequestHook, PreLLMHook, and PostLLMHook to reflect this exact distinction so readers know which hooks are once-per-request versus per-attempt.Sources: Coding guidelines, Learnings
docs/plugins/writing-go-plugin.mdx (1)
155-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClarify that these stubs are v1.6.x+ only (don’t present as active v1.5/v1.4 hooks).
These two skeleton tabs currently present
PreRequestHookas if it applies to v1.5.x+/v1.4.x directly, which conflicts with the versioned hook contract and can mislead implementers.Suggested edit
-// PreRequestHook is called once per top-level request (NOT per fallback attempt). -// This is the routing phase — use it for provider/model/fallback decisions. -// Mutations to req.Provider/req.Model/req.Fallbacks commit and propagate to every attempt. -// Errors are non-blocking (logged + skipped). -func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error { - ctx.Log(schemas.LogLevelInfo, "PreRequestHook called") - // Plugins that don't participate in routing should just return nil - return nil -} +// Optional in v1.6.x+ only: +// func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error { +// ctx.Log(schemas.LogLevelInfo, "PreRequestHook called") +// return nil +// }-// PreRequestHook is called once per top-level request (routing phase) -// Mutations to req.Provider/req.Model/req.Fallbacks commit across fallbacks -func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error { - fmt.Println("PreRequestHook called") - return nil -} +// Optional in v1.6.x+ only: +// func PreRequestHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) error { +// fmt.Println("PreRequestHook called") +// return nil +// }As per coding guidelines, “PreRequestHook added for v1.6.x+; older versions use the prior hook set.”
Also applies to: 281-286
🤖 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 `@docs/plugins/writing-go-plugin.mdx` around lines 155 - 163, Update the PreRequestHook stub to explicitly state it is available only in v1.6.x and later: annotate the comment above the function to say "PreRequestHook (v1.6.x+)" and add a short note that older versions (v1.5.x/v1.4.x) use the prior hook set; make the same change for the other skeleton at lines referenced, ensuring the comment near the function signature for PreRequestHook (and parameters schemas.BifrostContext, schemas.BifrostRequest) clearly indicates the version constraint so implementers aren’t misled.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@docs/plugins/getting-started.mdx`:
- Line 87: Update the lifecycle step wording to remove the contradictory "for
each request" phrase: state that PreRequestHook() runs once per top-level
request, while PreLLMHook() and PostLLMHook() run per attempt (and will re-run
on fallbacks); edit the sentence referencing PreRequestHook, PreLLMHook, and
PostLLMHook to reflect this exact distinction so readers know which hooks are
once-per-request versus per-attempt.
In `@docs/plugins/writing-go-plugin.mdx`:
- Around line 155-163: Update the PreRequestHook stub to explicitly state it is
available only in v1.6.x and later: annotate the comment above the function to
say "PreRequestHook (v1.6.x+)" and add a short note that older versions
(v1.5.x/v1.4.x) use the prior hook set; make the same change for the other
skeleton at lines referenced, ensuring the comment near the function signature
for PreRequestHook (and parameters schemas.BifrostContext,
schemas.BifrostRequest) clearly indicates the version constraint so implementers
aren’t misled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d5e7d85-3261-496c-b708-35a9edbcaac5
📒 Files selected for processing (5)
docs/architecture/core/plugins.mdxdocs/plugins/getting-started.mdxdocs/plugins/sequencing.mdxdocs/plugins/writing-go-plugin.mdxdocs/providers/provider-routing.mdx
Merge activity
|
3 participants
Summary
Documents the
PreRequestHookplugin lifecycle phase introduced in v1.6.x. This hook runs exactly once per top-level request — before any provider call and beforePreLLMHook— and is the designated place for routing decisions (provider/model/fallback resolution). Previously, routing documentation was scattered and the distinction between per-request and per-attempt hooks was not clearly articulated.Changes
PreRequestHookto the plugin lifecycle state diagram, sequence diagrams, and execution order descriptions across the architecture and getting-started docs, clarifying that it runs once per request whilePreLLMHook/PostLLMHookrun once per provider attempt (including fallbacks)<Info>callout explaining when to usePreRequestHookvsPreLLMHookvsPostLLMHooksequencing.mdxdocumenting the built-in plugin execution order withinPreRequestHook: governance (order 4) → enterprise load balancer → model-catalog-resolver (order 9, final fallback)PreRequestHookfunction stubs and a full routing example (withAppendRoutingEngineLogandAppendToContextList) to the Go plugin writing guidePreRequestHookphase rather than across separate middleware stages, and revised the flowcharts and execution order tables accordinglyType of change
Affected areas
How to test
Review the rendered documentation pages for:
docs/architecture/core/plugins.mdxdocs/plugins/getting-started.mdxdocs/plugins/sequencing.mdxdocs/plugins/writing-go-plugin.mdxdocs/providers/provider-routing.mdxVerify that the state diagram, sequence diagrams, and flowcharts render correctly and that the
PreRequestHookphase is consistently described across all pages.Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit