Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughPrompts plugin and related server/transport code were updated: prompt resolver signatures simplified, prompt-stream request flag removed, new context keys for selected prompt metadata added, prompt headers renamed to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Apr 7, 2026
Contributor
Author
18 tasks
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
7e6de90 to
ea62825
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
9917544 to
5f923f9
Compare
|
|
18 tasks
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
5f923f9 to
face6b2
Compare
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
2 times, most recently
from
869bf55 to
e65dccc
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
face6b2 to
44734e7
Compare
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
e65dccc to
633d32f
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
44734e7 to
369f154
Compare
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
633d32f to
66cc058
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
369f154 to
56330d2
Compare
18 tasks
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
66cc058 to
f37eed7
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
3 times, most recently
from
4f1cda0 to
5c75f3d
Compare
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
f37eed7 to
4416453
Compare
Contributor
Confidence Score: 4/5Not safe to merge as-is: the UI build will fail due to missing destructuring in settingsPanel.tsx. One P0 build-breaking issue in the UI component prevents the PR from shipping. The Go backend changes are clean and well-tested; the single fix needed is adding ui/components/prompts/fragments/settingsPanel.tsx — missing Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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: 1
🧹 Nitpick comments (1)
plugins/prompts/main.go (1)
214-217: Docstring contract forPreLLMHookis stale versus implementation.The comment says resolution/missing prompt/version returns an error, but the code logs warnings and returns unchanged request with
nilerror.Also applies to: 224-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 214 - 217, The docstring for PreLLMHook (and the similar block around lines 224-240) claims resolution/missing prompt/version returns an error, but the implementation currently logs a warning and returns the original *schemas.BifrostRequest with a nil error; update the documentation to match the code or change the code to return an error—pick one approach. Concretely, either (A) edit the comment for PreLLMHook and the related docblock to state that resolution failures or invalid/empty templates will be logged and result in returning the unchanged request with nil error, or (B) modify PreLLMHook to return an error on resolution/missing prompt/version and adjust callers accordingly; reference PreLLMHook to locate the implementation and update the comments at the top of the file to keep the contract consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/prompts/main.go`:
- Around line 61-62: The PromptVersionKey is stored as a string in the write
path but read with bifrost.GetIntFromContext (which only type-asserts to int),
causing the version header to be ignored; fix by converting the stored string to
an int when reading: fetch the raw value from context (PromptVersionKey) as a
string (or use bifrost.GetStringFromContext if available), parse it to an int
(e.g., strconv.Atoi) and return that as versionNumber in the function that
returns promptID, versionNumber, nil; alternatively, ensure the write path
stores an int under PromptVersionKey so bifrost.GetIntFromContext
succeeds—update either the setter call that writes PromptVersionKey or the
getter path where versionNumber is obtained accordingly.
---
Nitpick comments:
In `@plugins/prompts/main.go`:
- Around line 214-217: The docstring for PreLLMHook (and the similar block
around lines 224-240) claims resolution/missing prompt/version returns an error,
but the implementation currently logs a warning and returns the original
*schemas.BifrostRequest with a nil error; update the documentation to match the
code or change the code to return an error—pick one approach. Concretely, either
(A) edit the comment for PreLLMHook and the related docblock to state that
resolution failures or invalid/empty templates will be logged and result in
returning the unchanged request with nil error, or (B) modify PreLLMHook to
return an error on resolution/missing prompt/version and adjust callers
accordingly; reference PreLLMHook to locate the implementation and update the
comments at the top of the file to keep the contract consistent.
🪄 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: 304abd90-784a-402a-8405-c62650f3793f
📒 Files selected for processing (10)
core/schemas/bifrost.gocore/utils.goplugins/prompts/helpers_test.goplugins/prompts/main.goplugins/prompts/plugin_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.goui/components/prompts/fragments/settingsPanel.tsxui/lib/store/apis/baseApi.ts
✅ Files skipped from review due to trivial changes (2)
- core/utils.go
- ui/lib/store/apis/baseApi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/components/prompts/fragments/settingsPanel.tsx
- plugins/prompts/helpers_test.go
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
272618a to
53dd746
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/prompts/main.go (2)
246-250:⚠️ Potential issue | 🟡 MinorSet the selected prompt ID independently of the display name.
SelectedPromptIDis currently gated byprompt.Name != "", so a prompt with a blank name loses its stable identifier even though a version was resolved and applied. The ID should be written wheneverprompt != nil; only the name needs the extra guard.Suggested fix
- if prompt != nil && prompt.Name != "" { - ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) - ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) - } + if prompt != nil { + ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) + if prompt.Name != "" { + ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) + } + } ctx.SetValue(schemas.BifrostContextKeySelectedPromptVersion, strconv.Itoa(version.VersionNumber))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 246 - 250, The code incorrectly only sets SelectedPromptID when prompt.Name != "" so prompts with empty names lose their ID; change the logic so that ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) is executed whenever prompt != nil, while keeping the guard for setting schemas.BifrostContextKeySelectedPromptName so the name is only written if prompt.Name != ""; ensure ctx.SetValue(schemas.BifrostContextKeySelectedPromptVersion, strconv.Itoa(version.VersionNumber)) remains unchanged and still uses version.
446-490:⚠️ Potential issue | 🟡 MinorNegative prompt versions still resolve to “latest”.
A malformed
x-bf-prompt-version: -1— or a buggy customPromptResolverreturning-1— falls through theversionNumber > 0check and silently selectsLatestVersion. That should fail closed instead of behaving like “latest”.Suggested fix
func (p *Plugin) resolveVersion(promptID string, versionNumber int) ( *configstoreTables.TablePrompt, *configstoreTables.TablePromptVersion, bool, ) { p.mu.RLock() defer p.mu.RUnlock() prompt, ok := p.promptsByID[promptID] if !ok || prompt == nil { return nil, nil, false } + if versionNumber < 0 { + return prompt, nil, false + } if versionNumber > 0 { byNumber, ok := p.versionsByPromptAndNumber[promptID] if !ok { return nil, nil, false } @@ n, err := strconv.ParseInt(s, 10, 64) if err != nil { return 0, err } + if n < 0 { + return 0, fmt.Errorf("%s must be non-negative", key) + } return int(n), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 446 - 490, The resolveVersion function currently treats any non-positive versionNumber as "no explicit version" and returns LatestVersion, which allows negative values like -1 to silently resolve to latest; update resolveVersion (function name: resolveVersion) to explicitly reject negative version numbers by adding a check that if versionNumber < 0 it returns nil, nil, false (failing closed), while leaving versionNumber == 0 behavior unchanged; optionally you can also validate in parseNumberFromContext if you prefer to surface a parsing error early, but the minimal fix is the negative check inside resolveVersion before the existing versionNumber > 0 block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/server/server.go`:
- Around line 1082-1083: The lookup using
getPromptsPluginName()/schemas.BifrostContextKeyPromptsPluginName is dead
because nothing ever writes that context key; either write the key during server
initialization or remove the fallback lookup. Fix by setting the prompts plugin
name into the server context with SetValue(...) at startup (where server is
built/initialized) so getPromptsPluginName() can read it, or if the feature
isn't ready, remove the lib.FindPluginAs[handlers.PromptCacheReloader] branch
that relies on getPromptsPluginName() and keep using prompts.PluginName only;
adjust references to handlers.PromptCacheReloader and lib.FindPluginAs
accordingly.
---
Duplicate comments:
In `@plugins/prompts/main.go`:
- Around line 246-250: The code incorrectly only sets SelectedPromptID when
prompt.Name != "" so prompts with empty names lose their ID; change the logic so
that ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) is
executed whenever prompt != nil, while keeping the guard for setting
schemas.BifrostContextKeySelectedPromptName so the name is only written if
prompt.Name != ""; ensure
ctx.SetValue(schemas.BifrostContextKeySelectedPromptVersion,
strconv.Itoa(version.VersionNumber)) remains unchanged and still uses version.
- Around line 446-490: The resolveVersion function currently treats any
non-positive versionNumber as "no explicit version" and returns LatestVersion,
which allows negative values like -1 to silently resolve to latest; update
resolveVersion (function name: resolveVersion) to explicitly reject negative
version numbers by adding a check that if versionNumber < 0 it returns nil, nil,
false (failing closed), while leaving versionNumber == 0 behavior unchanged;
optionally you can also validate in parseNumberFromContext if you prefer to
surface a parsing error early, but the minimal fix is the negative check inside
resolveVersion before the existing versionNumber > 0 block.
🪄 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: cf7c6158-a0b4-47d8-9281-a3cc76383d2d
📒 Files selected for processing (10)
core/schemas/bifrost.gocore/utils.goplugins/prompts/helpers_test.goplugins/prompts/main.goplugins/prompts/plugin_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.goui/components/prompts/fragments/settingsPanel.tsxui/lib/store/apis/baseApi.ts
✅ Files skipped from review due to trivial changes (4)
- ui/components/prompts/fragments/settingsPanel.tsx
- ui/lib/store/apis/baseApi.ts
- core/utils.go
- plugins/prompts/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/prompts/plugin_test.go
- transports/bifrost-http/server/plugins.go
- transports/bifrost-http/handlers/inference.go
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
d54ccca to
dc92eb1
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
53dd746 to
29a6458
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
transports/bifrost-http/server/server.go (1)
288-294:⚠️ Potential issue | 🟠 MajorVerify the prompts-plugin name is populated before this lookup runs.
This only changes behavior if
schemas.BifrostContextKeyPromptsPluginNameis written on the server/bootstrap path beforeRegisterAPIRoutes. Otherwise the new enterprise routing silently falls back toprompts.PluginName, so the added branch is effectively inactive.Run this read-only check against the current stacked branch:
#!/bin/bash set -euo pipefail echo "== Reads ==" rg -n --type go -C2 'BifrostContextKeyPromptsPluginName' echo echo "== Writes via BifrostContext.SetValue ==" rg -n --type go -C2 'SetValue\s*\(\s*(schemas\.)?BifrostContextKeyPromptsPluginName\b' echo echo "== Writes via context.WithValue ==" rg -n --type go -C2 'WithValue\s*\([^)]*BifrostContextKeyPromptsPluginName' echo echo "== Bootstrap and route registration ==" rg -n --type go -C3 'Bootstrap\(|RegisterAPIRoutes\('Expected result: at least one reachable write before
RegisterAPIRoutes(...)/getPromptsPluginName()on the enterprise startup path.Based on learnings: Bifrost context key/value assignments are done with
(*BifrostContext).SetValue(key, value), not withcontext.WithValue.Also applies to: 1082-1084
plugins/prompts/main.go (2)
246-249:⚠️ Potential issue | 🟠 Major
SelectedPromptIDshould not depend on prompt name presence.If
prompt.Nameis empty, the code currently skips settingBifrostContextKeySelectedPromptID, which drops stable prompt identity metadata even though resolution succeeded.Suggested fix
- if prompt != nil && prompt.Name != "" { - ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) - ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) - } + if prompt != nil { + ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) + if prompt.Name != "" { + ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 246 - 249, The current conditional checks prompt.Name before setting BifrostContextKeySelectedPromptID which causes the selected prompt ID to be omitted when the prompt has an empty name; change the logic in the prompt resolution block so that ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) is executed whenever prompt != nil (independent of prompt.Name), and only conditionally set ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) when prompt.Name != "" to preserve stable identity while still avoiding empty name metadata.
486-490:⚠️ Potential issue | 🟡 MinorReject negative prompt versions instead of silently treating them as “latest.”
strconv.ParseIntaccepts-1, andresolveVersioninterprets non-positive as latest. That makes invalid negative header values behave like valid “latest” selection.Suggested fix
n, err := strconv.ParseInt(s, 10, 64) if err != nil { return 0, err } + if n < 0 { + return 0, fmt.Errorf("invalid %s: must be non-negative", key) + } return int(n), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 486 - 490, The parse block in resolveVersion currently allows negative numbers (strconv.ParseInt on s) which later get treated as "latest"; change the logic to explicitly reject negative versions: after parsing s into n (the strconv.ParseInt result), if n < 0 return a descriptive error (e.g., "invalid negative version") instead of returning 0, nil; keep handling of zero/positive versions unchanged so only negative values are rejected. Ensure the check references the parsed variable (n) and the resolveVersion function so behavior is clear.
🧹 Nitpick comments (2)
plugins/prompts/plugin_test.go (1)
612-616: Remove obsoletespecifiedexpectation field from version parsing tests.
specifiedis no longer asserted after the resolver contract simplification, so keeping it now adds noise.Also applies to: 623-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/plugin_test.go` around lines 612 - 616, Remove the obsolete specified expectation from the version parsing tests by deleting the specified field from the want struct (currently declared with fields num, specified, wantErr) and removing all expectations and assertions that reference want.specified in the test cases and checks (also remove the corresponding specified entries in per-case literal structs around the other occurrences noted). Update any test case literals and assertion logic in plugin_test.go (the version parsing tests) so they only use num and wantErr.plugins/prompts/main.go (1)
216-220: PreLLMHook docstring no longer matches behavior.The comment says missing prompt/version returns an error, but implementation warns and returns
(req, nil, nil). Please update the doc to avoid misleading integrators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 216 - 220, Update the PreLLMHook docstring to reflect actual behavior: state that missing prompt or version does NOT return an error but logs a warning and returns (req, nil, nil); likewise invalid or empty templates return the original request with a nil error. Mention the function's return tuple semantics: first value is the possibly-modified *schemas.BifrostRequest, second is always nil *schemas.LLMPluginShortCircuit, and third is an error only for true resolution failures (not for missing prompt/version or empty templates).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/prompts/main.go`:
- Around line 246-249: The current conditional checks prompt.Name before setting
BifrostContextKeySelectedPromptID which causes the selected prompt ID to be
omitted when the prompt has an empty name; change the logic in the prompt
resolution block so that ctx.SetValue(schemas.BifrostContextKeySelectedPromptID,
prompt.ID) is executed whenever prompt != nil (independent of prompt.Name), and
only conditionally set ctx.SetValue(schemas.BifrostContextKeySelectedPromptName,
prompt.Name) when prompt.Name != "" to preserve stable identity while still
avoiding empty name metadata.
- Around line 486-490: The parse block in resolveVersion currently allows
negative numbers (strconv.ParseInt on s) which later get treated as "latest";
change the logic to explicitly reject negative versions: after parsing s into n
(the strconv.ParseInt result), if n < 0 return a descriptive error (e.g.,
"invalid negative version") instead of returning 0, nil; keep handling of
zero/positive versions unchanged so only negative values are rejected. Ensure
the check references the parsed variable (n) and the resolveVersion function so
behavior is clear.
---
Nitpick comments:
In `@plugins/prompts/main.go`:
- Around line 216-220: Update the PreLLMHook docstring to reflect actual
behavior: state that missing prompt or version does NOT return an error but logs
a warning and returns (req, nil, nil); likewise invalid or empty templates
return the original request with a nil error. Mention the function's return
tuple semantics: first value is the possibly-modified *schemas.BifrostRequest,
second is always nil *schemas.LLMPluginShortCircuit, and third is an error only
for true resolution failures (not for missing prompt/version or empty
templates).
In `@plugins/prompts/plugin_test.go`:
- Around line 612-616: Remove the obsolete specified expectation from the
version parsing tests by deleting the specified field from the want struct
(currently declared with fields num, specified, wantErr) and removing all
expectations and assertions that reference want.specified in the test cases and
checks (also remove the corresponding specified entries in per-case literal
structs around the other occurrences noted). Update any test case literals and
assertion logic in plugin_test.go (the version parsing tests) so they only use
num and wantErr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 259896eb-7a72-4b6a-a515-75cd5ffea10c
📒 Files selected for processing (10)
core/schemas/bifrost.gocore/utils.goplugins/prompts/helpers_test.goplugins/prompts/main.goplugins/prompts/plugin_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.goui/components/prompts/fragments/settingsPanel.tsxui/lib/store/apis/baseApi.ts
✅ Files skipped from review due to trivial changes (3)
- core/utils.go
- ui/components/prompts/fragments/settingsPanel.tsx
- ui/lib/store/apis/baseApi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/server/plugins.go
- plugins/prompts/helpers_test.go
18 tasks
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
29a6458 to
9cd0a3e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/prompts/main.go (1)
477-490:⚠️ Potential issue | 🟡 MinorReject negative prompt versions during parsing.
strconv.ParseIntaccepts-1, andresolveVersiontreats non-positive values as “latest”, so negative headers silently bypass explicit-version intent.Suggested fix
func parseNumberFromContext(ctx *schemas.BifrostContext, key schemas.BifrostContextKey) (num int, err error) { s, ok := ctx.Value(key).(string) if !ok { return 0, nil } s = strings.TrimSpace(s) if s == "" { return 0, nil } n, err := strconv.ParseInt(s, 10, 64) if err != nil { return 0, err } + if n < 0 { + return 0, fmt.Errorf("%s must be non-negative", key) + } return int(n), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 477 - 490, The parseNumberFromContext function currently allows negative integers (because strconv.ParseInt accepts "-1"), which lets negative prompt-version headers bypass explicit-version handling in resolveVersion; update parseNumberFromContext to validate the parsed int (in function parseNumberFromContext) and return a non-nil error when n < 0 (include a clear error message referencing the invalid negative value and the provided context key) so callers like resolveVersion cannot treat negative values as valid versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/prompts/main.go`:
- Around line 217-220: Update the PreLLMHook comment to match actual behavior:
state that when a prompt/version is missing or resolution fails (or when a
template is invalid/empty), the function logs warnings and returns (req, nil,
nil) rather than returning an error; also update the adjacent doc block covering
the related cases (the comment span around lines referencing the same behavior)
so both descriptions consistently describe the nil-error, logged-warning
behavior for PreLLMHook.
---
Duplicate comments:
In `@plugins/prompts/main.go`:
- Around line 477-490: The parseNumberFromContext function currently allows
negative integers (because strconv.ParseInt accepts "-1"), which lets negative
prompt-version headers bypass explicit-version handling in resolveVersion;
update parseNumberFromContext to validate the parsed int (in function
parseNumberFromContext) and return a non-nil error when n < 0 (include a clear
error message referencing the invalid negative value and the provided context
key) so callers like resolveVersion cannot treat negative values as valid
versions.
🪄 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: 3f7fcc4c-20d6-4091-b88d-4bdf407014a7
📒 Files selected for processing (10)
core/schemas/bifrost.gocore/utils.goplugins/prompts/helpers_test.goplugins/prompts/main.goplugins/prompts/plugin_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.goui/components/prompts/fragments/settingsPanel.tsxui/lib/store/apis/baseApi.ts
✅ Files skipped from review due to trivial changes (3)
- ui/components/prompts/fragments/settingsPanel.tsx
- core/utils.go
- ui/lib/store/apis/baseApi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/server/plugins.go
- plugins/prompts/plugin_test.go
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
dc92eb1 to
6328594
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
9cd0a3e to
423ec88
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/prompts/main.go (1)
246-267:⚠️ Potential issue | 🟠 MajorMove side effects after template validation to preserve no-op fallback behavior.
Line 252 mutates request params and Lines 246-250 mutate selected-prompt context before template conversion is validated. If conversion fails or yields empty template (Lines 260-267), the hook exits early but leaves partial state changes behind.
Proposed fix
- if prompt != nil && prompt.Name != "" { - ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) - ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) - } - ctx.SetValue(schemas.BifrostContextKeySelectedPromptVersion, strconv.Itoa(version.VersionNumber)) - - // Apply model params from the version (version params are defaults; request params win). - switch { - case req.ChatRequest != nil: - applyVersionParamsToChatRequest(version, req.ChatRequest, p.logger) - case req.ResponsesRequest != nil: - applyVersionParamsToResponsesRequest(version, req.ResponsesRequest, p.logger) - } - template, err := chatMessagesFromVersionMessages(version.Messages) if err != nil { p.logger.Warn("prompts plugin: failed to convert version messages to chat messages: %v", err) return req, nil, nil } if len(template) == 0 { p.logger.Warn("prompts plugin: no template messages found for prompt %s version %d", promptID, version.VersionNumber) return req, nil, nil } + + if prompt != nil { + ctx.SetValue(schemas.BifrostContextKeySelectedPromptID, prompt.ID) + if prompt.Name != "" { + ctx.SetValue(schemas.BifrostContextKeySelectedPromptName, prompt.Name) + } + } + ctx.SetValue(schemas.BifrostContextKeySelectedPromptVersion, strconv.Itoa(version.VersionNumber)) + + // Apply model params from the version (version params are defaults; request params win). + switch { + case req.ChatRequest != nil: + applyVersionParamsToChatRequest(version, req.ChatRequest, p.logger) + case req.ResponsesRequest != nil: + applyVersionParamsToResponsesRequest(version, req.ResponsesRequest, p.logger) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 246 - 267, The hook currently mutates request and context (calls to ctx.SetValue with schemas.BifrostContextKeySelectedPromptID/Name/Version and the calls applyVersionParamsToChatRequest/applyVersionParamsToResponsesRequest) before validating the template via chatMessagesFromVersionMessages and checking template length, leaving partial state on error/empty template; change the flow so you first call chatMessagesFromVersionMessages(version.Messages) and return early on error or empty template, and only after it succeeds assign ctx.SetValue for SelectedPromptID/Name/Version (keeping strconv.Itoa(version.VersionNumber)) and then applyVersionParamsToChatRequest/applyVersionParamsToResponsesRequest so no side effects occur when conversion fails or yields an empty template.
♻️ Duplicate comments (2)
plugins/prompts/main.go (2)
217-220:⚠️ Potential issue | 🟡 MinorAlign PreLLMHook return docs with actual warn-and-continue behavior.
Line 219 says resolution/missing prompt-version returns errors, but implementation returns
(req, nil, nil)with warnings (Lines 227-239 and 241-267). The comment should match runtime behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 217 - 220, Update the comment/documentation for PreLLMHook to reflect its actual warn-and-continue behavior: instead of stating that resolution failures or missing prompt/version return an error, document that in those cases the function logs warnings and returns (req, nil, nil) leaving the request unchanged; reference the PreLLMHook implementation paths around the warning blocks (the checks that log and return req,nil,nil) so the doc matches runtime behavior.
486-490:⚠️ Potential issue | 🟡 MinorReject negative prompt versions instead of treating them as “latest”.
ParseIntaccepts negatives, andresolveVersiontreats non-positive values as latest (Line 456 onward). Sox-bf-prompt-version: -1currently bypasses explicit version selection silently.Proposed fix
n, err := strconv.ParseInt(s, 10, 64) if err != nil { return 0, err } + if n < 0 { + return 0, fmt.Errorf("%q must be non-negative", string(key)) + } return int(n), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/main.go` around lines 486 - 490, The code in resolveVersion uses strconv.ParseInt to parse the x-bf-prompt-version header and currently treats negative values as "latest"; change the validation after parsing (the block with strconv.ParseInt and the return int(n)) to explicitly reject negative values by returning a clear error when n < 0 while keeping zero and positive values valid; update the error message to mention an invalid negative prompt version and reference the strconv.ParseInt call and the resolveVersion function when locating where to add this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/prompts/main.go`:
- Around line 246-267: The hook currently mutates request and context (calls to
ctx.SetValue with schemas.BifrostContextKeySelectedPromptID/Name/Version and the
calls applyVersionParamsToChatRequest/applyVersionParamsToResponsesRequest)
before validating the template via chatMessagesFromVersionMessages and checking
template length, leaving partial state on error/empty template; change the flow
so you first call chatMessagesFromVersionMessages(version.Messages) and return
early on error or empty template, and only after it succeeds assign ctx.SetValue
for SelectedPromptID/Name/Version (keeping strconv.Itoa(version.VersionNumber))
and then applyVersionParamsToChatRequest/applyVersionParamsToResponsesRequest so
no side effects occur when conversion fails or yields an empty template.
---
Duplicate comments:
In `@plugins/prompts/main.go`:
- Around line 217-220: Update the comment/documentation for PreLLMHook to
reflect its actual warn-and-continue behavior: instead of stating that
resolution failures or missing prompt/version return an error, document that in
those cases the function logs warnings and returns (req, nil, nil) leaving the
request unchanged; reference the PreLLMHook implementation paths around the
warning blocks (the checks that log and return req,nil,nil) so the doc matches
runtime behavior.
- Around line 486-490: The code in resolveVersion uses strconv.ParseInt to parse
the x-bf-prompt-version header and currently treats negative values as "latest";
change the validation after parsing (the block with strconv.ParseInt and the
return int(n)) to explicitly reject negative values by returning a clear error
when n < 0 while keeping zero and positive values valid; update the error
message to mention an invalid negative prompt version and reference the
strconv.ParseInt call and the resolveVersion function when locating where to add
this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47207cee-18ce-4b4e-a021-9414c3ce08ea
📒 Files selected for processing (9)
core/schemas/bifrost.goplugins/prompts/helpers_test.goplugins/prompts/main.goplugins/prompts/plugin_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.goui/components/prompts/fragments/settingsPanel.tsxui/lib/store/apis/baseApi.ts
✅ Files skipped from review due to trivial changes (2)
- ui/components/prompts/fragments/settingsPanel.tsx
- ui/lib/store/apis/baseApi.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- transports/bifrost-http/server/plugins.go
- plugins/prompts/helpers_test.go
- core/schemas/bifrost.go
- plugins/prompts/plugin_test.go
roroghost17
force-pushed
the
04-07-refactor_extracts_routing_utilites_to_a_separate_package
branch
from
6328594 to
a9ceab4
Compare
roroghost17
force-pushed
the
04-07-feat_updates_prompts_plugin_flow
branch
from
423ec88 to
944dd72
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/prompts/plugin_test.go (1)
488-499: Add the same literal/case-insensitive coverage forx-bf-prompt-version.This test now protects the raw
x-bf-prompt-idheader rename, but the version-header path is still only exercised throughPromptVersionHeader. If that literal regresses, the current version test can still pass because it uses the same constant on both setup and lookup.🧪 Suggested test addition
func TestHTTPTransportPreHook_CaseInsensitiveHeaders(t *testing.T) { p := newTestPlugin(nil, nil, nil) ctx := bfCtx() // "X-Bf-Prompt-Id" is a title-case variant of the canonical "x-bf-prompt-id". req := &schemas.HTTPRequest{ Headers: map[string]string{"X-Bf-Prompt-Id": "upper-case"}, } _, _ = p.HTTPTransportPreHook(ctx, req) got, _ := ctx.Value(PromptIDKey).(string) assert.Equal(t, "upper-case", got) } + +func TestHTTPTransportPreHook_CaseInsensitiveVersionHeaders(t *testing.T) { + p := newTestPlugin(nil, nil, nil) + ctx := bfCtx() + req := &schemas.HTTPRequest{ + Headers: map[string]string{"X-Bf-Prompt-Version": "42"}, + } + + _, _ = p.HTTPTransportPreHook(ctx, req) + + got, _ := ctx.Value(PromptVersionKey).(string) + assert.Equal(t, "42", got) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/prompts/plugin_test.go` around lines 488 - 499, Add a case-insensitive check for the version header in TestHTTPTransportPreHook_CaseInsensitiveHeaders by including the title-case literal "X-Bf-Prompt-Version" in the req.Headers map (e.g., "X-Bf-Prompt-Version": "1.2") alongside "X-Bf-Prompt-Id", call p.HTTPTransportPreHook(ctx, req) as before, and assert the context value retrieved via ctx.Value(PromptVersionKey).(string) equals the expected version string; this ensures the code path that reads x-bf-prompt-version is covered independently of the PromptVersionHeader constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/prompts/plugin_test.go`:
- Around line 488-499: Add a case-insensitive check for the version header in
TestHTTPTransportPreHook_CaseInsensitiveHeaders by including the title-case
literal "X-Bf-Prompt-Version" in the req.Headers map (e.g.,
"X-Bf-Prompt-Version": "1.2") alongside "X-Bf-Prompt-Id", call
p.HTTPTransportPreHook(ctx, req) as before, and assert the context value
retrieved via ctx.Value(PromptVersionKey).(string) equals the expected version
string; this ensures the code path that reads x-bf-prompt-version is covered
independently of the PromptVersionHeader constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d10efb2f-ad9b-4d1d-9f2c-e6cef4b93cc7
📒 Files selected for processing (9)
core/schemas/bifrost.goplugins/prompts/helpers_test.goplugins/prompts/main.goplugins/prompts/plugin_test.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/server/plugins.gotransports/bifrost-http/server/server.goui/components/prompts/fragments/settingsPanel.tsxui/lib/store/apis/baseApi.ts
✅ Files skipped from review due to trivial changes (3)
- ui/components/prompts/fragments/settingsPanel.tsx
- ui/lib/store/apis/baseApi.ts
- plugins/prompts/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/bifrost.go
Contributor
Merge activity
|
akshaydeo
changed the base branch from
04-07-refactor_extracts_routing_utilites_to_a_separate_package
to
graphite-base/2545
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
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
5 participants
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.
Changes
Refactors the prompts plugin to improve error handling, simplify the resolver interface, and add support for enterprise prompts plugin routing. The changes prepare the plugin for better integration with enterprise features while maintaining backward compatibility.
versionSpecifiedboolean return value fromResolvemethod and simplified version resolution logicBifrostContextKeyPromptsPluginNamecontext key and routing logic to support enterprise prompts plugin selectionBifrostContextKeyPromptStreamRequestand related stream detection from model parameters in transport hookspromptStoreto exportedInMemoryStoreinterface for better extensibilityBifrostContextKeySelectedPromptName,BifrostContextKeySelectedPromptVersion, andBifrostContextKeySelectedPromptIDfor governance and observabilitybf-prompt-id/bf-prompt-versiontox-bf-prompt-id/x-bf-prompt-versionresolveVersionmethod to handle explicit version numbers vs latest version more clearlyType of change
Affected areas
How to test
Test prompt resolution with both header-based and custom resolvers. Verify enterprise prompts plugin routing works when
BifrostContextKeyPromptsPluginNameis set.Breaking changes
The
PromptResolverinterface signature changed fromResolve(ctx, req) (string, int, bool, error)toResolve(ctx, req) (string, int, error). Custom resolver implementations will need to be updated to remove theversionSpecifiedboolean return value. Additionally, prompt headers changed frombf-prompt-id/bf-prompt-versiontox-bf-prompt-id/x-bf-prompt-version.Security considerations
No security implications - this is an internal refactoring that maintains the same access patterns and validation logic.