Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions modules/agents/codex/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from modules.agents.codex.session import CodexSessionManager
from modules.agents.codex.transport import CodexTransport
from modules.agents.codex.turn_state import CodexTurnRegistry
from vibe.codex_config import read_active_model_provider

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -563,6 +564,9 @@ async def _start_or_resume_thread(
"threadId": persisted,
"developerInstructions": self._build_thread_developer_instructions(request),
}
model_provider = read_active_model_provider(cwd=getattr(request, "working_path", None))
if model_provider:
resume_params["modelProvider"] = model_provider
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Stop forcing modelProvider on every thread/resume

thread/resume is now always sent with modelProvider, but Codex app-server docs state that supplying modelProvider disables resume’s persisted model/reasoningEffort fallback (openai/codex codex-rs/app-server/README.md, around line 482). That means resuming a thread created with a non-default model will silently re-resolve model settings from current config instead of preserving the thread’s last persisted model, so users can unexpectedly switch models after resume (especially when turn/start omits an explicit model). This regression is introduced by adding resume_params["modelProvider"] unconditionally here.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Addressed in 00de2f8. Resume now reads the stored thread metadata first and only sends modelProvider when the persisted thread provider differs from the currently resolved Codex provider. Matching-provider resumes omit the override, so Codex keeps its persisted model / reasoning-effort fallback.

resp = await transport.send_request(
"thread/resume",
resume_params,
Expand Down
34 changes: 34 additions & 0 deletions tests/test_codex_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,40 @@ async def test_resume_thread_refreshes_developer_instructions_without_appending(
)
self.assertNotIn("Channel-level session key:", developer_instructions)

async def test_resume_thread_overrides_stale_session_model_provider(self):
agent = object.__new__(CodexAgent)
agent.controller = SimpleNamespace(config=SimpleNamespace(platform="slack", reply_enhancements=True))
agent.codex_config = SimpleNamespace(default_model=None)
agent._session_mgr = SimpleNamespace(set_thread_id=Mock())
agent.sessions = SimpleNamespace(
get_agent_session_id=Mock(return_value="thread-existing"),
)
request = SimpleNamespace(
working_path="/tmp/work",
context=SimpleNamespace(
platform="slack",
platform_specific={"is_dm": False},
user_id="U1",
channel_id="C1",
thread_id="171717.123",
),
base_session_id="session-1",
session_key="slack::channel::C1::thread::171717.123",
subagent_name=None,
subagent_model=None,
subagent_reasoning_effort=None,
)
transport = SimpleNamespace(send_request=AsyncMock(return_value={"thread": {"id": "thread-existing"}}))

with patch.object(_MODULE, "read_active_model_provider", return_value="openai-managed") as read_provider:
thread_id = await agent._start_or_resume_thread(transport, request)

self.assertEqual(thread_id, "thread-existing")
read_provider.assert_called_once_with(cwd="/tmp/work")
method, params = transport.send_request.await_args.args
self.assertEqual(method, "thread/resume")
self.assertEqual(params["modelProvider"], "openai-managed")

async def test_resume_thread_keeps_system_prompt_injection_when_quick_replies_are_disabled(self):
agent = object.__new__(CodexAgent)
agent.controller = SimpleNamespace(config=SimpleNamespace(platform="slack", reply_enhancements=False))
Expand Down
59 changes: 59 additions & 0 deletions tests/test_codex_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,65 @@ def test_apply_oauth_mode_clears_managed_base_url(tmp_path: Path) -> None:
assert "model_providers" not in parsed


def test_read_active_model_provider_falls_back_to_builtin_openai_after_oauth(tmp_path: Path) -> None:
"""OAuth mode clears Vibe Remote's managed provider pointer.

Resuming an old API-key thread must still override its persisted
``openai-managed`` provider back to Codex's built-in ``openai`` provider.
"""
home = tmp_path
codex_config.apply_codex_auth(
auth_mode="api_key",
api_key="sk-test",
base_url="https://relay.example/v1",
home=home,
)
codex_home = home / ".codex"
auth_path = codex_home / "auth.json"
auth_data = json.loads(auth_path.read_text(encoding="utf-8"))
auth_data["tokens"] = {"id_token": "abc"}
auth_path.write_text(json.dumps(auth_data), encoding="utf-8")

codex_config.apply_codex_auth(auth_mode="oauth", api_key=None, base_url=None, home=home)

parsed = tomllib.loads((codex_home / "config.toml").read_text(encoding="utf-8"))
assert "model_provider" not in parsed
assert codex_config.read_active_model_provider(home=home) == codex_config.DEFAULT_PROVIDER_ID


def test_read_active_model_provider_prefers_nearest_project_scope(tmp_path: Path) -> None:
home = tmp_path
codex_home = home / ".codex"
codex_home.mkdir()
project_root = tmp_path / "work"
nested_project = project_root / "nested"
cwd = nested_project / "src"
cwd.mkdir(parents=True)
seed = (
'model_provider = "top-level"\n'
"\n"
f'[projects."{project_root}"]\n'
'model_provider = "project-provider"\n'
"\n"
f'[projects."{nested_project}"]\n'
'model_provider = "nested-provider"\n'
)
(codex_home / "config.toml").write_text(seed, encoding="utf-8")

assert (
codex_config.read_active_model_provider(home=home, cwd=cwd)
== "nested-provider"
)
assert (
codex_config.read_active_model_provider(home=home, cwd=project_root / "other")
== "project-provider"
)
assert (
codex_config.read_active_model_provider(home=home, cwd=tmp_path / "elsewhere")
== "top-level"
)


def test_apply_oauth_clears_user_owned_relay_pointer_with_base_url(tmp_path: Path) -> None:
"""OAuth tokens don't validate against custom relays — clear the pointer.

Expand Down
57 changes: 57 additions & 0 deletions vibe/codex_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
# ``LEGACY_MANAGED_PROVIDER_IDS`` and the migration in ``apply_codex_auth``.
MANAGED_PROVIDER_ID = "openai-managed"
LEGACY_MANAGED_PROVIDER_IDS = ("openai",)
DEFAULT_PROVIDER_ID = "openai"

# Codex's top-level ``cli_auth_credentials_store`` controls where the CLI
# reads/writes cached credentials: ``file`` → ``~/.codex/auth.json``,
Expand Down Expand Up @@ -75,6 +76,62 @@ def get_codex_config_paths(home: Path | None = None) -> tuple[Path, Path]:
return codex_home / "config.toml", codex_home / "auth.json"


def read_active_model_provider(
home: Path | None = None,
cwd: str | Path | None = None,
) -> str:
"""Return the provider id Codex will use for a thread in ``cwd``.

Codex persists a thread's provider in session metadata and reuses it on
resume unless the caller passes an explicit ``modelProvider`` override.
Vibe Remote uses this helper to migrate resumed threads after the user
switches Codex auth/provider settings, for example OAuth -> API key relay.
Project-scoped settings take precedence over the top-level provider, which
matches Codex's normal ``thread/start`` resolution for a request ``cwd``.
If no top-level provider is configured, Codex falls back to its built-in
OpenAI provider, whose id is ``openai``.
"""
config_path, _ = get_codex_config_paths(home)
toml_data = _load_toml(config_path)
if cwd is not None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Respect profile/project config in provider resolution

read_active_model_provider() only reads $CODEX_HOME/config.toml and then derives model_provider from that single document, but Codex merges config in a higher-precedence order (global config, then project .codex/config.toml, then active profile overrides). Because _start_or_resume_thread() now uses this helper to decide whether to send modelProvider on thread/resume, a user with provider overrides in project config or [profiles.<name>] can get an incorrect override injected, which can route resumes to the wrong provider and break continuation for provider-bound threads.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Addressed in d0b05e9. The resume provider check no longer parses ~/.codex/config.toml in Vibe Remote. It asks the running Codex app-server via config/read with the request cwd, then compares that effective modelProvider with the stored thread metadata. That leaves Codex responsible for profile/project/config-layer precedence and avoids injecting a stale provider override.

project_provider = _project_model_provider(toml_data, Path(cwd).expanduser())
if project_provider:
return project_provider
model_provider = toml_data.get("model_provider")
if isinstance(model_provider, str) and model_provider.strip():
return model_provider.strip()
return DEFAULT_PROVIDER_ID


def _project_model_provider(toml_data: Dict[str, Any], cwd: Path) -> str | None:
"""Return the nearest project-scoped provider for ``cwd`` if configured."""
projects = toml_data.get("projects")
if not isinstance(projects, dict):
return None

try:
resolved_cwd = cwd.resolve(strict=False)
except OSError:
resolved_cwd = cwd.absolute()

best_match: tuple[int, str] | None = None
for raw_path, settings in projects.items():
if not isinstance(raw_path, str) or not isinstance(settings, dict):
continue
provider = settings.get("model_provider")
if not isinstance(provider, str) or not provider.strip():
continue
try:
project_path = Path(raw_path).expanduser().resolve(strict=False)
except OSError:
project_path = Path(raw_path).expanduser().absolute()
if resolved_cwd == project_path or project_path in resolved_cwd.parents:
score = len(project_path.parts)
if best_match is None or score > best_match[0]:
best_match = (score, provider.strip())
return best_match[1] if best_match else None


def _load_toml(path: Path) -> Dict[str, Any]:
if not path.exists():
return {}
Expand Down
Loading