fix(acp): return global sessions in unstable_listSessions#18743
fix(acp): return global sessions in unstable_listSessions#18743Haohao-end wants to merge 2 commits intoanomalyco:devfrom
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
The following comment was made by an LLM, it may be inaccurate: I found related PRs that address the same issue: Potential Duplicates:
Both PRs are addressing the exact same issue - changing |
atharvau
left a comment
There was a problem hiding this comment.
ACP Sessions Fix Review ✅
This PR fixes session listing in ACP mode to use global sessions instead of directory-scoped sessions.
✅ Functional Fix
- Root Cause: ACP was using directory-scoped session listing (lines 674-683 removed)
- Solution: Switch to Session.listGlobal for global session access (line 674)
- Pagination: Improved cursor-based pagination logic (lines 675-677)
✅ Implementation Quality
- Cleaner Logic: Reduces complex SDK call chain to simpler global list access
- Better Performance: Eliminates unnecessary API round-trip for session fetching
- Proper Pagination: Fixed more/cursor logic for correct pagination behavior
✅ Code Simplification
- Removed 10 lines of complex nested API calls
- More direct data access pattern
- Clearer intent with Session.listGlobal
📝 Technical Notes
The change makes sense for ACP mode which should have global session visibility rather than being constrained to specific directories.
Recommendation: MERGE - Good bug fix that simplifies code and improves functionality.
atharvau
left a comment
There was a problem hiding this comment.
Code Review
✅ Approved - Good Bug Fix
This is a well-targeted bug fix that correctly addresses the scoping issue in unstable_listSessions.
What's Fixed
- Bug: The old implementation incorrectly scoped session listing to the current project directory by passing
directory: params.cwd, when it should return global sessions - Solution: Properly uses
Session.listGlobal()which queries all sessions without directory filtering
Code Quality
- Simplification: Reduces complexity by removing manual sorting/filtering logic in favor of the existing
Session.listGlobalimplementation - Consistency: Aligns with existing patterns used elsewhere in the codebase
- Maintains API: Preserves existing response structure and pagination behavior
Security & Performance
- No security issues: Uses existing validated query patterns
- Performance improvement: Leverages database-level ordering instead of in-memory sorting
- Proper pagination: Correctly implements cursor-based pagination with
limit + 1pattern
Minor Suggestions
- The import should follow existing import grouping conventions
- Consider extracting the magic number
100to a named constant
Overall: Clean, focused bug fix that improves correctness without introducing risk.
|
Thanks for the reviews. I updated the PR description to satisfy the template, synced the branch with Regarding the duplicate note: this PR is a minimal fix for #18575 only. It switches ACP |
atharvau
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall Assessment: APPROVED ✅
What This PR Does
Optimizes ACP unstable_listSessions to use Session.listGlobal instead of making SDK calls, improving performance and consistency.
✅ Positives
- Performance improvement: Eliminates unnecessary SDK roundtrip
- Simpler logic: Replaces complex sorting/filtering with built-in pagination
- Consistent API: Uses the same Session.listGlobal used elsewhere
- Proper pagination: Correctly handles cursor-based pagination with more detection
🔍 Analysis
- No bugs detected: Logic is sound and handles edge cases
- No security issues: No security implications
- Performance gain: Direct access vs SDK call overhead
- Style compliance: Follows existing patterns
Code Quality
The refactored logic is cleaner:
- Uses Session.listGlobal with proper limit handling
- Correctly detects if there are more results (limit + 1 technique)
- Proper slicing when more results exist
- Maintains same API contract
Behavior Validation
- ✅ Cursor-based pagination preserved
- ✅ Sorting by updated time maintained (built into Session.listGlobal)
- ✅ Same response format
- ✅ Proper handling of edge cases (empty results, no cursor)
This is an excellent optimization that reduces complexity while improving performance.
atharvau
left a comment
There was a problem hiding this comment.
Code Review for PR #18743
Overall Assessment
Good bug fix that correctly addresses the scope issue in ACP session listing. The solution properly uses the existing global session enumeration pattern.
Code Analysis
Issue Understanding:
- unstable_listSessions was incorrectly scoped to current project directory via params.cwd
- Should return global sessions across all projects, not just current one
- Bug caused ACP to only show sessions for active project
Solution Quality:
- Correct API Usage: Switches to Session.listGlobal which is the established pattern
- Preserved Functionality: Maintains pagination logic and response structure
- Minimal Change: Targeted fix without unnecessary refactoring
- Consistent Pattern: Follows same approach used in experimental.ts
Technical Implementation
Pagination Logic - Excellent:
The new implementation properly handles pagination:
- Uses limit + 1 to detect if more results exist
- Correctly calculates nextCursor from last item
- Maintains the same SessionInfo response structure
Edge Case Handling:
- Properly handles empty results
- Cursor-based pagination preserved
- Root sessions filtering maintained via roots: true parameter
Performance Considerations
Potential Concern:
Global session listing might be slower than project-scoped listing for users with many projects/sessions. Consider if there should be any performance monitoring or limits.
Positive:
- Uses existing optimized Session.listGlobal implementation
- Maintains cursor-based pagination to avoid loading all sessions at once
Testing Gap
Issue:
Author couldn't run local tests due to missing dependencies (tsgo, zod, drizzle-orm). While the code review shows the fix is correct, this highlights a potential CI/testing gap.
Recommendation:
Ensure this PR passes full CI test suite before merging, especially tests related to ACP session listing functionality.
Code Quality
Strengths:
- Clean, readable implementation
- Proper use of existing APIs
- Good variable naming (list, more, page)
- Maintains backward compatibility
Verdict
APPROVE with Minor Monitoring - The fix is technically correct and addresses the reported issue properly. However, ensure CI tests pass and consider monitoring performance impact of global session enumeration.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
Issue for this PR
Closes #18575
Type of change
What does this PR do?
ACP.unstable_listSessionswas usingsdk.session.list({ directory: params.cwd, roots: true }), which scopes results to the current project directory. That means ACP only returned sessions for the active project instead of returning the global session list described in the issue.This PR changes
unstable_listSessionsto useSession.listGlobal({ roots: true, cursor, limit: limit + 1 })instead.Why this works:
Session.listGlobal(...)is the existing global session enumeration path already used elsewhere in the codebase.params.cwdasdirectory.SessionInfofield mappingnextCursorfrom the last returned session'stime.updatedThis change is intentionally minimal and only touches
packages/opencode/src/acp/agent.ts.How did you verify your code works?
I verified the fix by:
packages/opencode/src/acp/agent.tssdk.session.list(...)was project-scoped because it useddirectory: params.cwdpackages/opencode/src/session/index.tspackages/opencode/src/server/routes/experimental.tsI also attempted to run local checks, but my local environment is missing required tooling/dependencies:
bun typecheckfailed becausetsgois not installed in the current environmentbun test test/server/global-session-list.test.tsfailed because local dependencies such aszodanddrizzle-orm/bun-sqlite/migratorwere not availableSo I was able to verify the code path and the fix logically in the repository, but I could not complete a full local automated test run in the current machine state.
Screenshots / recordings
N/A
Checklist