feat: add /btw background session command#18635
feat: add /btw background session command#18635Svtter wants to merge 4 commits intoanomalyco:devfrom
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Some providers (e.g. xAI) enforce undocumented tool count limits. Add limit.tools to model schema and cap activeTools in streamText when the total exceeds the provider limit, prioritizing built-in tools over MCP tools.
|
The following comment was made by an LLM, it may be inaccurate: Related PR Found:
No other duplicate PRs found. PR #18635 is explicitly intended to replace #17198 with the same feature rebased for current compatibility. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
atharvau
left a comment
There was a problem hiding this comment.
This PR adds a comprehensive background task command system (/btw) with todo management and child session support, plus tool count limiting and synthetic message filtering improvements.
Strengths ✅
- Comprehensive test coverage: Excellent test suite with realistic scenarios
- Well-architected: Clean separation between ask, todo, and status functionality
- Good error handling: Proper async error catching and reporting
- Type safety: Strong TypeScript usage throughout
Concerns 🚨
1. Large Feature Scope
- 7 files modified - this is a substantial feature addition
- Multiple concerns mixed: Tool limiting, synthetic messages, background tasks
- Consider splitting: This could be broken into smaller, focused PRs
2. Tool Limiting Logic
const TOOL_LIMITS: Record<string, number> = { xai: 200 }- Hardcoded limits: Should be configurable or from provider metadata
- Silent truncation: Dropping tools without user awareness could cause confusion
- Missing documentation: Users won't know why their tools aren't available
3. Synthetic Message Filtering
- Breaking change potential: Changes how synthetic messages are handled
- Unclear rationale: Why filter synthetic assistant parts but not user parts?
- Missing tests: No specific tests for this behavior change
4. Session Management Complexity
- Parent-child relationships: Complex async coordination between sessions
- Memory usage: Background sessions persist indefinitely?
- Error propagation: What happens when child sessions fail?
Recommendations
- Split the PR: Separate tool limits, synthetic filtering, and /btw command
- Tool limits: Make configurable or document the reasoning
- Add documentation: Command needs user-facing docs
- Session cleanup: Consider lifecycle management for background sessions
- Test synthetic filtering: Add specific tests for message filtering changes
Recommendation: Consider splitting into focused PRs for safer integration.
atharvau
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall Assessment:
Substantial feature addition that adds background task capability. Well-implemented but has some areas for improvement.
✅ Strengths:
- Comprehensive Feature: Adds
ask,todo, andstatussubcommands - Good Architecture: Proper session management and child session creation
- Extensive Testing: 269 lines of thorough test coverage
- Tool Limiting: Smart tool count limiting for providers with restrictions
- Async Handling: Proper background task execution and result posting
🟡 Areas for Improvement:
Code Quality:
- Naming Convention Violation: Multiple camelCase variables (
assistantMessage,toolExecutor) should be single words per style guide - Function Length:
commandBtwfunction is quite long (~200 lines) - consider breaking into smaller functions - Duplication: Helper functions (
post,update) could be extracted to reusable utilities
Potential Bugs:
- Double Length Check: Line 759 has redundant
if (assistantMessage.parts.length > 0)check - Synthetic Text Filtering: The synthetic text filtering logic may be too broad and could affect other features
Security/Performance:
- Tool Limiting: Hard-coded
TOOL_LIMITSfor xai (200) - consider making this configurable - Memory Usage: Background tasks could accumulate without cleanup mechanism
Documentation:
- ✅ Good command documentation in
.opencode/command/btw.md - 🟡 Complex features could use more inline code comments
Breaking Changes:
- Message Filtering: Changes to synthetic text part handling in
toModelMessages - Tool Limits: New tool limiting may affect existing workflows
Recommendations:
- Refactor: Break down
commandBtwinto smaller, focused functions - Fix Naming: Use single-word variable names per style guide
- Remove Duplication: Extract common patterns
- Add Cleanup: Consider background task cleanup/timeout mechanisms
Overall: Good feature, ready to merge with follow-up refactoring
|
@atharvau Thanks for the review. I'm going to split this work into two smaller PRs instead of keeping the current scope bundled together. Planned split:
I've already reworked the code locally along those lines and will follow up by separating them into those two reviewable changes. |
|
Follow-up: the split PRs are now up.
I'll keep further review on those split PRs. |
|
Yeah, sorry about that. Good call.
…On Mar 24, 2026 at 3:24 AM -0700, 修昊 ***@***.***>, wrote:
Svtter left a comment (anomalyco/opencode#18635)
Follow-up: the split PRs are now up.
• #18917 /btw command work
• #18916 tool-limit work
I'll keep further review on those split PRs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Issue for this PR
Closes #16992
Supersedes #17198
Type of change
What does this PR do?
This rebases the original
/btwimplementation from #17198 onto currentdev.I resolved the merge conflict in
packages/opencode/src/session/llm.tsand fixed/btwafter rebase by awaitingSessionStatus.get(), which is now async on currentdev.There are no intended behavior changes beyond making the branch mergeable and keeping the original
/btwbehavior working on currentdev.Credit to @brendandebeasi for the original implementation in #17198.
How did you verify your code works?
cd packages/opencode && bun typecheckcd packages/opencode && bun test test/session/btw.test.ts test/session/message-v2.test.tsScreenshots / recordings
N/A - backend/CLI command, no UI changes.
Checklist