refactor(assistant): split runtime concerns - #63
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR modularizes the assistant runtime by extracting session resolution, context loading, model orchestration (with caching and retry), streaming partial-progress persistence, slash-command dispatch, and skill read tooling into dedicated files. ChangesAssistant Runtime Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
==========================================
- Coverage 62.32% 62.31% -0.01%
==========================================
Files 178 184 +6
Lines 17824 17818 -6
==========================================
- Hits 11108 11103 -5
+ Misses 5619 5618 -1
Partials 1097 1097
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/assistant/runtime_model.go (1)
118-146: ⚖️ Poor tradeoffConsider bundling
modelCompletionRequestparameters.SonarCloud flags this constructor as having 9 parameters (limit 7). Several of these already travel together (e.g.
selectedModel,auth,usage,messages,systemPromptcould be passed via the existingcontextResult/a small params struct), which would clear the gate and reduce positional-arg mistakes. Optional given this is a behavior-preserving refactor.🤖 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/runtime_model.go` around lines 118 - 146, Replace the long parameter list of Runtime.modelCompletionRequest by introducing a small params struct (e.g., ModelCompletionParams or reuse the existing contextResult) that bundles related values like SelectedModel *model.Model, Auth model.RequestAuth, Messages []database.MessageEntity, SystemPrompt string, Usage model.TokenUsage, SessionID string, CWD string, Registry *tool.Registry and OnEvent func(StreamEvent); change modelCompletionRequest to accept that single params struct (plus the runtime receiver), map the struct fields to the CompletionRequest as before, and update all call sites to construct and pass the new params struct so behavior remains identical while reducing the parameter count and preventing positional-arg mistakes.internal/assistant/runtime_skills.go (1)
44-46: 💤 Low valueConsider making this a package constant.
The function always returns the same value. Unless there are plans to make this configurable, consider defining it as a constant for clarity.
♻️ Optional refactor
+const maxActiveSkillReadLines = 2000 + -func maxActiveSkillReadLines() int { - return 2000 -}Then update the caller on line 20:
- limit := maxActiveSkillReadLines() + limit := maxActiveSkillReadLines🤖 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/runtime_skills.go` around lines 44 - 46, Replace the trivial function maxActiveSkillReadLines() with a package-level constant (e.g., MaxActiveSkillReadLines = 2000) and update all call sites (including the caller referencing maxActiveSkillReadLines) to use that constant instead of calling the function; keep the constant name exported or unexported to match intended visibility and adjust any tests or references to maxActiveSkillReadLines accordingly.
🤖 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/runtime_slash.go`:
- Around line 24-26: Replace the plain fmt.Errorf in the empty slash command
check with a samber/oops error: locate the if block that checks commandName ==
"" in runtime_slash.go and return an oops error created via
oops.In("assistant").Code("empty_slash_command").New("assistant: empty slash
command") (or use .Wrapf(err, "...") if you are wrapping an existing error),
ensuring the package consistently uses oops for contextual errors.
- Around line 86-88: Replace the plain fmt.Errorf return with an oops error:
change the return that currently does fmt.Errorf("assistant: skill %q not
found", name) to use samber/oops, e.g.
oops.In("assistant").Code("skill_not_found").Newf("skill %q not found", name)
(or Wrapf(nil, ...) if your codebase prefers Wrapf); update the import to
include "github.com/samber/oops" if not already present and ensure the returned
error uses this oops instance instead of fmt.Errorf.
---
Nitpick comments:
In `@internal/assistant/runtime_model.go`:
- Around line 118-146: Replace the long parameter list of
Runtime.modelCompletionRequest by introducing a small params struct (e.g.,
ModelCompletionParams or reuse the existing contextResult) that bundles related
values like SelectedModel *model.Model, Auth model.RequestAuth, Messages
[]database.MessageEntity, SystemPrompt string, Usage model.TokenUsage, SessionID
string, CWD string, Registry *tool.Registry and OnEvent func(StreamEvent);
change modelCompletionRequest to accept that single params struct (plus the
runtime receiver), map the struct fields to the CompletionRequest as before, and
update all call sites to construct and pass the new params struct so behavior
remains identical while reducing the parameter count and preventing
positional-arg mistakes.
In `@internal/assistant/runtime_skills.go`:
- Around line 44-46: Replace the trivial function maxActiveSkillReadLines() with
a package-level constant (e.g., MaxActiveSkillReadLines = 2000) and update all
call sites (including the caller referencing maxActiveSkillReadLines) to use
that constant instead of calling the function; keep the constant name exported
or unexported to match intended visibility and adjust any tests or references to
maxActiveSkillReadLines accordingly.
🪄 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: b6faa6b3-ab85-43d7-8a01-97aaaa5f0d92
📒 Files selected for processing (7)
internal/assistant/runtime.gointernal/assistant/runtime_context.gointernal/assistant/runtime_model.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_session.gointernal/assistant/runtime_skills.gointernal/assistant/runtime_slash.go
💤 Files with no reviewable changes (1)
- internal/assistant/runtime.go
|
This was referenced May 30, 2026
This was referenced Jun 9, 2026
Merged
Merged
Merged
Merged
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
2 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.
Summary
Validation