fix/context-explicit-lineage - #233
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughPrompt execution now serializes session operations and tracks active prompt lineage explicitly. Context construction requires explicit branch endpoints, compaction rebuilds context from adopted lineage entries, assistant persistence uses the active parent, and terminal compaction snapshots session parameters before asynchronous execution. ChangesPrompt lineage coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Prompt
participant OperationCoordinator
participant Lineage
participant ContextBuilder
participant Provider
participant SessionRepository
Prompt->>OperationCoordinator: acquire session operation
Prompt->>Lineage: create active parent
Prompt->>ContextBuilder: build branch context
ContextBuilder->>Provider: submit completion request
Provider-->>Prompt: response or overflow
Prompt->>SessionRepository: compact from active parent
SessionRepository-->>Lineage: return compaction entry
Lineage->>ContextBuilder: rebuild from adopted parent
Prompt->>OperationCoordinator: release session operation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
==========================================
+ Coverage 84.71% 84.76% +0.05%
==========================================
Files 314 315 +1
Lines 29106 29196 +90
==========================================
+ Hits 24656 24748 +92
+ Misses 3058 3053 -5
- Partials 1392 1395 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
internal/assistant/context_overflow_compaction.go (1)
26-32: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInput validation doesn't guard the new
lineagefield, butrecoverProviderContextOverflowdereferences it unconditionally.
input.preparation.auth == nilis checked here, butinput.preparation.lineageis not, even thoughrecoverProviderContextOverflow(line 60) doesinput.preparation.lineage.activeParentEntryIDas a direct field read (not through the nil-safepromptLineage.adopt()method). Before this PR, the equivalent field wasuserEntryID string, whose zero value could never panic; now that it's*promptLineage, a nil value here will panic when the context-window-error recovery path is taken.Today's only caller (
modelResponseinruntime_model.go) always supplies a non-nil lineage, so this isn't reachable in the current call graph, but the validation function's explicit purpose is to guard against exactly this nil-deref class for every required field —lineageshould be included.🛡️ Proposed fix
- if input == nil || input.build == nil || input.preparation == nil || input.preparation.auth == nil { + if input == nil || input.build == nil || input.preparation == nil || + input.preparation.auth == nil || input.preparation.lineage == 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 `@internal/assistant/context_overflow_compaction.go` around lines 26 - 32, Extend the nil-input validation for recoverProviderContextOverflow to also reject a nil input.preparation.lineage, alongside the existing build, preparation, and auth checks. Keep the current invalid-input error and return behavior unchanged so recoverProviderContextOverflow cannot dereference lineage before validation.
🧹 Nitpick comments (1)
internal/assistant/session_operations_internal_test.go (1)
60-79: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise the mid-wait cancellation branch.
ctxis already canceled beforecoordinator.acquire(ctx, "session")is called, so execution hits the upfrontctx.Err()check insession_operations.go(lines 30-32) rather than theselect's<-ctx.Done()branch (lines 55-62) that exercisesreleaseReferencecleanup for a goroutine that was genuinely blocked. The name "CancelsWaiter" implies the latter, but the latter is untested.Consider adding a case where a second goroutine actually blocks on
acquirefor a held session, then cancel its context mid-wait (similar to the synchronization pattern inTestSessionOperationCoordinatorSerializesSameSession), to cover the refcount cleanup for the in-flight cancellation path.🤖 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 `@internal/assistant/session_operations_internal_test.go` around lines 60 - 79, The test TestSessionOperationCoordinatorCancelsWaiter currently cancels its context before acquire, covering only the upfront cancellation check. Modify it so a second goroutine calls acquire while the session is held, then cancel the context after confirming it is blocked and assert context.Canceled with a nil release function; finally release the owner and verify a subsequent acquire succeeds, covering the in-flight waiter cleanup path.
🤖 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.
Outside diff comments:
In `@internal/assistant/context_overflow_compaction.go`:
- Around line 26-32: Extend the nil-input validation for
recoverProviderContextOverflow to also reject a nil input.preparation.lineage,
alongside the existing build, preparation, and auth checks. Keep the current
invalid-input error and return behavior unchanged so
recoverProviderContextOverflow cannot dereference lineage before validation.
---
Nitpick comments:
In `@internal/assistant/session_operations_internal_test.go`:
- Around line 60-79: The test TestSessionOperationCoordinatorCancelsWaiter
currently cancels its context before acquire, covering only the upfront
cancellation check. Modify it so a second goroutine calls acquire while the
session is held, then cancel the context after confirming it is blocked and
assert context.Canceled with a nil release function; finally release the owner
and verify a subsequent acquire succeeds, covering the in-flight waiter cleanup
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5498ed25-6848-4592-8e7f-25d60b4cafb2
📒 Files selected for processing (23)
internal/assistant/agent_runtime_wrappers_internal_test.gointernal/assistant/context_auto_compaction.gointernal/assistant/context_auto_compaction_internal_extra_test.gointernal/assistant/context_auto_compaction_internal_test.gointernal/assistant/context_build.gointernal/assistant/context_build_test.gointernal/assistant/context_compaction.gointernal/assistant/context_overflow_compaction.gointernal/assistant/context_overflow_compaction_test.gointernal/assistant/export_test.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_context.gointernal/assistant/runtime_context_internal_test.gointernal/assistant/runtime_model.gointernal/assistant/runtime_persist.gointernal/assistant/session_operations.gointernal/assistant/session_operations_internal_test.gointernal/assistant/tool_schema_cache_internal_test.gointernal/database/session_repository_test.gointernal/database/session_store.gointernal/terminal/compact_commands.gointernal/terminal/compact_commands_internal_test.go
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 `@internal/assistant/export_test.go`:
- Around line 93-116: Update ProviderOverflowRecoveryNilLineageForTest to
initialize CompletionRequest and contextwindow.Budget with explicit zero values
for every field instead of using empty struct literals, satisfying exhaustruct
while preserving the existing test behavior.
🪄 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 Plus
Run ID: 19f96ee0-77e7-4744-861b-86a9abde63a7
📒 Files selected for processing (9)
internal/assistant/context_build_test.gointernal/assistant/context_overflow_compaction.gointernal/assistant/context_overflow_compaction_test.gointernal/assistant/context_post_response_auto_compaction_test.gointernal/assistant/export_test.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/runtime_context_internal_test.gointernal/assistant/session_operations_internal_test.gointernal/database/session_repository_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/assistant/context_overflow_compaction.go
- internal/assistant/runtime_context_internal_test.go
- internal/assistant/context_build_test.go
1a7f399 to
1b895c0
Compare
|
1 participant
No description provided.