Skip to content

fix(acp): return global sessions in unstable_listSessions#18743

Closed
Haohao-end wants to merge 2 commits intoanomalyco:devfrom
Haohao-end:fix/acp-list-sessions-global-18575
Closed

fix(acp): return global sessions in unstable_listSessions#18743
Haohao-end wants to merge 2 commits intoanomalyco:devfrom
Haohao-end:fix/acp-list-sessions-global-18575

Conversation

@Haohao-end
Copy link

Issue for this PR

Closes #18575

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

ACP.unstable_listSessions was using sdk.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_listSessions to use Session.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.
  • It removes the accidental project-level filtering caused by passing params.cwd as directory.
  • It preserves the existing ACP response shape and pagination behavior:
    • still returns root sessions only
    • still uses the same SessionInfo field mapping
    • still computes nextCursor from the last returned session's time.updated

This 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:

  • tracing the old implementation in packages/opencode/src/acp/agent.ts
  • confirming that sdk.session.list(...) was project-scoped because it used directory: params.cwd
  • checking the existing global enumeration implementation in packages/opencode/src/session/index.ts
  • comparing the usage pattern with packages/opencode/src/server/routes/experimental.ts

I also attempted to run local checks, but my local environment is missing required tooling/dependencies:

  • bun typecheck failed because tsgo is not installed in the current environment
  • bun test test/server/global-session-list.test.ts failed because local dependencies such as zod and drizzle-orm/bun-sqlite/migrator were not available

So 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

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions github-actions bot added the needs:compliance This means the issue will auto-close after 2 hours. label Mar 23, 2026
@github-actions
Copy link
Contributor

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • Not all checklist items are checked. Please confirm you have tested locally and have not included unrelated changes.

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.

@github-actions
Copy link
Contributor

The following comment was made by an LLM, it may be inaccurate:

I found related PRs that address the same issue:

Potential Duplicates:

  1. PR fix(acp): use global session list for unstable_listSessions #18645 - fix(acp): use global session list for unstable_listSessions

  2. PR fix(acp): use global session list in unstable_listSessions #18576 - fix(acp): use global session list in unstable_listSessions

Both PRs are addressing the exact same issue - changing unstable_listSessions to use global session enumeration instead of project-scoped listing. You should check the status of PRs #18576 and #18645 to understand how they relate to PR #18743.

Copy link

@atharvau atharvau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@atharvau atharvau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.listGlobal implementation
  • 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 + 1 pattern

Minor Suggestions

  1. The import should follow existing import grouping conventions
  2. Consider extracting the magic number 100 to a named constant

Overall: Clean, focused bug fix that improves correctness without introducing risk.

@Haohao-end
Copy link
Author

Thanks for the reviews.

I updated the PR description to satisfy the template, synced the branch with dev, and all required checks are now passing.

Regarding the duplicate note: this PR is a minimal fix for #18575 only. It switches ACP unstable_listSessions from the project-scoped listing path to Session.listGlobal(...) while preserving the existing response shape and pagination behavior.

Copy link

@atharvau atharvau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@atharvau atharvau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Mar 23, 2026
@github-actions github-actions bot closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ACP unstable_listSessions only returns sessions for the current project

2 participants