Skip to content

feat: allow session/set_mode to switch collaboration presets (plan/default)#168

Open
z-x-yang wants to merge 2 commits intozed-industries:mainfrom
z-x-yang:feat/acp-set-mode-collaboration
Open

feat: allow session/set_mode to switch collaboration presets (plan/default)#168
z-x-yang wants to merge 2 commits intozed-industries:mainfrom
z-x-yang:feat/acp-set-mode-collaboration

Conversation

@z-x-yang
Copy link

Why

acpx documents set-mode plan, but with codex-acp the ACP call fails with Invalid params because set_session_mode currently only accepts approval presets (read-only/auto/full-access).

In practice this means ACP clients can set approval mode, but cannot switch collaboration presets (especially plan) via session/set_mode.

What this PR changes

  • Extends mode handling in src/thread.rs so session/set_mode supports both:
    1. existing approval presets (read-only, auto, full-access)
    2. collaboration presets from models_manager.list_collaboration_modes() (including plan and default)
  • Exposes collaboration presets in SessionModeState.availableModes so ACP clients can discover them.
  • Tracks active_mode_id so load/config mode state reflects the latest selected mode.
  • Keeps existing approval-preset behavior unchanged.

Validation

  • cargo check
  • cargo test test_set_mode_plan_routes_to_collaboration_mode

Behavior notes

  • set_mode(plan) now routes through Op::OverrideTurnContext { collaboration_mode: Some(...) }.
  • set_mode(auto/read-only/full-access) still updates approval/sandbox policies exactly as before.

If maintainers prefer, I can split this into:

  1. mode discovery/state changes, and
  2. set_mode routing changes.

@z-x-yang z-x-yang marked this pull request as ready for review February 26, 2026 07:50
@benbrandt
Copy link
Member

We can also expose this as a config option instead? and allow for setting both?

@z-x-yang
Copy link
Author

@benbrandt great point — yes, this should support both paths.

In codex-acp, session/set_config_option with config_id="mode" already routes to handle_set_mode(...), so this change effectively enables both:

  • session/set_mode (e.g. set-mode plan)
  • session/set_config_option(mode=...) (e.g. set mode plan)

I verified both flows locally via acpx against this branch. If helpful, I can add an explicit test for the set_config_option("mode", "plan") path as a follow-up in this PR.

@z-x-yang
Copy link
Author

z-x-yang commented Mar 4, 2026

Follow-up update pushed to this PR branch (7099e35):

  • Added session mode persistence store at $CODEX_HOME/acp/session-mode-overrides.v1.json
  • Persist mode override after successful:
    • session/set_mode
    • session/set_config_option when config_id == "mode"
  • Replay saved mode on new_session / load_session after thread init
  • Corruption-safe behavior: warn + continue
  • Added unit tests for store persistence and corrupt-file tolerance

Related tracking issue: #172.

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.

2 participants