Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
45 changes: 45 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 = await self._resolve_resume_model_provider_override(transport, request, persisted)
if model_provider:
resume_params["modelProvider"] = model_provider
resp = await transport.send_request(
"thread/resume",
resume_params,
Expand All @@ -585,6 +589,47 @@ async def _start_or_resume_thread(

return await self._start_thread(transport, request)

async def _resolve_resume_model_provider_override(
self,
transport: CodexTransport,
request: AgentRequest,
thread_id: str,
) -> Optional[str]:
"""Return a provider override only when a persisted thread is stale.

Codex preserves a thread's latest model / reasoning effort on resume
unless the client sends a model/provider override. Vibe Remote only
needs to override the provider after the user changes Codex auth mode
or project provider settings, so inspect the stored thread first and
leave normal resumes on Codex's persisted fallback path.
"""
current_provider = read_active_model_provider(cwd=getattr(request, "working_path", None))
if not current_provider:
return None

try:
resp = await transport.send_request(
"thread/read",
{
"threadId": thread_id,
"includeTurns": False,
},
)
except Exception as exc:
logger.warning("Failed to read Codex thread %s provider before resume: %s", thread_id, exc)
return None

thread_obj = resp.get("thread") if isinstance(resp, dict) else None
if not isinstance(thread_obj, dict) and isinstance(resp, dict) and resp.get("id") == thread_id:
thread_obj = resp
stored_provider = thread_obj.get("modelProvider") if isinstance(thread_obj, dict) else None
if not isinstance(stored_provider, str) or not stored_provider.strip():
return None

if stored_provider.strip() == current_provider:
return None
return current_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 Keep persisted provider when resuming cross-provider threads

Only overriding modelProvider based on current config when it differs from the stored thread provider can break valid cross-provider resumes: this path now forces thread/resume onto the current provider for any mismatch, even when the persisted thread was created under a different provider and must continue there. In those cases, resume can fail or misroute history because the provider affinity encoded in the stored thread is discarded at resume time.

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 db97d6c. The provider override is now limited to transitions between Vibe Remote-managed Codex provider IDs (openai and openai-managed). Other mismatches are treated as intentional cross-provider sessions and resume without modelProvider, preserving Codex persisted provider affinity. Added a regression test for an unmanaged anthropic thread while the current config points at openai-managed.


def _build_thread_developer_instructions(self, request: AgentRequest) -> Optional[str]:
"""Build Codex thread-level developer instructions for start/resume.

Expand Down
137 changes: 134 additions & 3 deletions tests/test_codex_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,14 @@ async def test_resume_thread_refreshes_developer_instructions_without_appending(
subagent_model=None,
subagent_reasoning_effort=None,
)
transport = SimpleNamespace(send_request=AsyncMock(return_value={"thread": {"id": "thread-existing"}}))
transport = SimpleNamespace(
send_request=AsyncMock(
side_effect=[
{"thread": {"id": "thread-existing", "modelProvider": "openai"}},
{"thread": {"id": "thread-existing"}},
]
)
)

with patch.object(
_MODULE,
Expand All @@ -899,8 +906,8 @@ async def test_resume_thread_refreshes_developer_instructions_without_appending(
thread_id = await agent._start_or_resume_thread(transport, request)

self.assertEqual(thread_id, "thread-existing")
transport.send_request.assert_awaited_once()
method, params = transport.send_request.await_args.args
self.assertEqual(transport.send_request.await_count, 2)
method, params = transport.send_request.await_args_list[1].args
self.assertEqual(method, "thread/resume")
self.assertEqual(params["threadId"], "thread-existing")
developer_instructions = params["developerInstructions"]
Expand All @@ -913,6 +920,130 @@ async def test_resume_thread_refreshes_developer_instructions_without_appending(
)
self.assertNotIn("Channel-level session key:", developer_instructions)

async def test_resume_thread_does_not_force_model_provider_when_thread_matches_config(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(
side_effect=[
{"thread": {"id": "thread-existing", "modelProvider": "openai-managed"}},
{"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")
self.assertEqual(transport.send_request.await_args_list[0].args[0], "thread/read")
method, params = transport.send_request.await_args_list[1].args
self.assertEqual(method, "thread/resume")
self.assertNotIn("modelProvider", params)

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(
side_effect=[
{"thread": {"id": "thread-existing", "modelProvider": "openai"}},
{"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")
self.assertEqual(transport.send_request.await_args_list[0].args[0], "thread/read")
method, params = transport.send_request.await_args_list[1].args
self.assertEqual(method, "thread/resume")
self.assertEqual(params["modelProvider"], "openai-managed")

async def test_resume_thread_omits_model_provider_when_provider_read_fails(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(
side_effect=[
RuntimeError("thread/read unavailable"),
{"thread": {"id": "thread-existing"}},
]
)
)

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

self.assertEqual(thread_id, "thread-existing")
method, params = transport.send_request.await_args_list[1].args
self.assertEqual(method, "thread/resume")
self.assertNotIn("modelProvider", params)

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