fix: guard scoped context deadlines#3177
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughNormalizes delegated/scoped parents when creating new BifrostContext and hardens BifrostContext.Deadline to tolerate nil/cleared parent chains, avoiding nil pointer dereferences. Adds tests covering deadline behavior across plugin scope release and a changelog entry documenting the fix. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Confidence Score: 5/5Safe to merge — fix is correct and well-tested, no regressions identified. The unwrap loop eliminates the hazard of holding a pooled scoped context as a child parent, the nil-parent guard in Deadline() closes the crash path, and tests cover nil receivers, cleared parents, released parents, and nested scoped parents. No P0 or P1 findings. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "[test]: core schemas cover nil context d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
core/schemas/context_test.go (1)
250-273: ⚡ Quick winAdd one nested-scope regression test (
root -> scopedA -> scopedB -> child)Current tests validate one scoped hop. Adding a nested-scope case would lock in behavior if
WithPluginScopeis ever called on a scoped context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/context_test.go` around lines 250 - 273, Add a nested-scope regression test mirroring TestNewBifrostContext_ScopedParentSurvivesScopeRelease but with two scoped hops: create root := NewBifrostContext(..., expectedDeadline) and set BifrostContextKeyTraceID, then scopedA := root.WithPluginScope(&nameA), scopedB := scopedA.WithPluginScope(&nameB), child := NewBifrostContext(scopedB, NoDeadline) (defer Cancel where appropriate), call scopedB.ReleasePluginScope() and scopedA.ReleasePluginScope(), and assert child.Deadline() still matches expectedDeadline and child.Value(BifrostContextKeyTraceID) equals the root value; name the test e.g. TestNewBifrostContext_NestedScopedParentSurvivesScopeRelease and reuse the same helper symbols (NewBifrostContext, WithPluginScope, ReleasePluginScope, BifrostContextKeyTraceID, NoDeadline).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/context.go`:
- Around line 80-82: The assignment only unwraps one level of
BifrostContext.valueDelegate; update the logic in the context creation code so
it fully unwraps chained scoped parents by looping while parent is a
*BifrostContext with a non-nil valueDelegate (e.g., replace the single if check
with a for/for-like loop that repeatedly casts parent to *BifrostContext and
sets parent = parentCtx.valueDelegate until no further valueDelegate exists),
referencing the parent variable, the BifrostContext type, and its valueDelegate
field.
---
Nitpick comments:
In `@core/schemas/context_test.go`:
- Around line 250-273: Add a nested-scope regression test mirroring
TestNewBifrostContext_ScopedParentSurvivesScopeRelease but with two scoped hops:
create root := NewBifrostContext(..., expectedDeadline) and set
BifrostContextKeyTraceID, then scopedA := root.WithPluginScope(&nameA), scopedB
:= scopedA.WithPluginScope(&nameB), child := NewBifrostContext(scopedB,
NoDeadline) (defer Cancel where appropriate), call scopedB.ReleasePluginScope()
and scopedA.ReleasePluginScope(), and assert child.Deadline() still matches
expectedDeadline and child.Value(BifrostContextKeyTraceID) equals the root
value; name the test e.g.
TestNewBifrostContext_NestedScopedParentSurvivesScopeRelease and reuse the same
helper symbols (NewBifrostContext, WithPluginScope, ReleasePluginScope,
BifrostContextKeyTraceID, NoDeadline).
🪄 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: 09f92bde-e711-449a-a6c0-f72e934f5a87
📒 Files selected for processing (2)
core/schemas/context.gocore/schemas/context_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/schemas/context_test.go (1)
173-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a typed key for this context test.
SetValue/Valuewith raw strings violates the repository's no-raw-context-keys rule and can collide with other scoped values. Switch this test to a dedicated key type.Suggested fix
func TestNewBifrostContext_SetAndGetValue(t *testing.T) { ctx := NewBifrostContext(context.Background(), NoDeadline) + type testContextKey string + const key1 testContextKey = "key1" + const nonexistentKey testContextKey = "nonexistent" + // Set a value - ctx.SetValue("key1", "value1") + ctx.SetValue(key1, "value1") // Get the value - if v := ctx.Value("key1"); v != "value1" { + if v := ctx.Value(key1); v != "value1" { t.Errorf("Expected value1, got %v", v) } // Get non-existent key - if v := ctx.Value("nonexistent"); v != nil { + if v := ctx.Value(nonexistentKey); v != nil { t.Errorf("Expected nil for non-existent key, got %v", v) }As per coding guidelines, Rule: No raw context keys in Go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/context_test.go` around lines 173 - 187, Replace raw string context keys in TestNewBifrostContext_SetAndGetValue with a package-local typed key to avoid raw-context-key collisions: declare a small key type (e.g., type testCtxKey string) inside the test and use testCtxKey("key1") when calling ctx.SetValue and ctx.Value (and testCtxKey("nonexistent") for the non-existent lookup); keep using the same NewBifrostContext and NoDeadline setup and the same assertions but reference the typed key instead of raw strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/context_test.go`:
- Around line 211-216: Add a test that covers the nil-receiver path by calling
Deadline() on a nil *BifrostContext and asserting it returns ok==false (and does
not panic); either extend TestBifrostContext_DeadlineWithClearedParent or add a
new TestBifrostContext_DeadlineWithNilReceiver that does: var bc
*BifrostContext; deadline, ok := bc.Deadline(); if ok { t.Errorf(...) } to
ensure the nil pointer path for the Deadline() method is exercised.
---
Outside diff comments:
In `@core/schemas/context_test.go`:
- Around line 173-187: Replace raw string context keys in
TestNewBifrostContext_SetAndGetValue with a package-local typed key to avoid
raw-context-key collisions: declare a small key type (e.g., type testCtxKey
string) inside the test and use testCtxKey("key1") when calling ctx.SetValue and
ctx.Value (and testCtxKey("nonexistent") for the non-existent lookup); keep
using the same NewBifrostContext and NoDeadline setup and the same assertions
but reference the typed key instead of raw strings.
🪄 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: 3e200d73-c39a-40e3-ab90-2140f435dac6
📒 Files selected for processing (2)
core/schemas/context.gocore/schemas/context_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/context.go
|
Addressed the review feedback in the latest commits:
Verified locally from go test ./schemas -run 'TestBifrostContext_DeadlineWithClearedParent|TestBifrostContext_DeadlineWithClearedParentAndOwnDeadline|TestNewBifrostContext_ReleasedPluginScopeParent|TestNewBifrostContext_ScopedParentSurvivesScopeRelease|TestNewBifrostContext_NestedScopedParentSurvivesScopeRelease' -count=1
go test ./schemas -count=1
go test -race ./schemas -count=1 |
ragokan
force-pushed
the
okan/harden-bifrost-context-deadline
branch
from
1304a1b to
d4ae169
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/schemas/context_test.go (1)
239-306: ⚡ Quick winPrefer
bifrost.Ptr()here, and drop the released-parent case.This block still passes
&name/&nameA/&nameBintoWithPluginScope, and the repo convention is to usebifrost.Ptr(...)for those pointers. It also testsNewBifrostContext(released, ...), which leaves a pooled wrapper attached to the child instead of covering the actual child-before-release regression. The scoped-parent tests below already exercise the supported path. Based on learnings: prefer usingbifrost.Ptr()to create pointers instead of the address operator (&) even when & would be valid syntactically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/context_test.go` around lines 239 - 306, Replace uses of the address operator when creating plugin scope names with the repo helper by calling bifrost.Ptr("...") instead of &name/&nameA/&nameB in the tests that call WithPluginScope (e.g., change scoped := root.WithPluginScope(&name) to scoped := root.WithPluginScope(bifrost.Ptr(name)) and similarly for scopedA/scopedB), and remove the obsolete TestNewBifrostContext_ReleasedPluginScopeParent test entirely so we only keep the supported scoped-parent and nested-scoped-parent tests (TestNewBifrostContext_ScopedParentSurvivesScopeRelease and TestNewBifrostContext_NestedScopedParentSurvivesScopeRelease) which exercise the intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/schemas/context_test.go`:
- Around line 239-306: Replace uses of the address operator when creating plugin
scope names with the repo helper by calling bifrost.Ptr("...") instead of
&name/&nameA/&nameB in the tests that call WithPluginScope (e.g., change scoped
:= root.WithPluginScope(&name) to scoped :=
root.WithPluginScope(bifrost.Ptr(name)) and similarly for scopedA/scopedB), and
remove the obsolete TestNewBifrostContext_ReleasedPluginScopeParent test
entirely so we only keep the supported scoped-parent and nested-scoped-parent
tests (TestNewBifrostContext_ScopedParentSurvivesScopeRelease and
TestNewBifrostContext_NestedScopedParentSurvivesScopeRelease) which exercise the
intended behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae8f6b5c-817b-414a-b7fe-abf6f66aef78
📒 Files selected for processing (3)
core/changelog.mdcore/schemas/context.gocore/schemas/context_test.go
✅ Files skipped from review due to trivial changes (1)
- core/changelog.md
1 participant
Summary
Fix a
BifrostContextpanic that can occur when a child context is created from a scoped plugin context and that scoped context is later released back to the pool.Fixes #3178.
Changes
NewBifrostContextnow unwraps scopedBifrostContextparents to their root context, so child contexts do not retain pooled scoped contexts afterReleasePluginScope().(*BifrostContext).Deadline()now safely handles nil receivers and cleared parents while preserving its own configured deadline.core/changelog.mdentry for the fix.Design trade-off: this keeps the existing scoped-context pool behavior, but avoids storing scoped pooled contexts as child parents.
Type of change
Affected areas
How to test
From
core/:Expected outcome: all commands pass.
No new configs or environment variables were added.
Screenshots/Recordings
Not applicable; this is a Go core fix with no UI changes.
Breaking changes
Related issues
Fixes #3178.
Security considerations
No new auth, secret, PII, sandboxing, or network behavior is introduced. The change only hardens context parent/deadline handling.
Checklist
docs/contributing/raising-a-pr.mdx,docs/contributing/code-conventions.mdx);docs/contributing/README.mdwas not present onmaincore/changelog.md; no docs page needed)core/schemas