mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
feat(providers): Codex CLI provider — OpenAI via ModelProvider.OPENAI (#659)
* feat(providers): Codex CLI provider — OpenAI via ModelProvider.OPENAI Mirrors the grok blueprint end to end: CodexCliProvider (RO ~/.codex mount, ANTHROPIC_* blanked), an orchestrator-side codex_auth.py refresher (JWT-exp staleness, atomic rewrite, lock-serialized single-use rotation, --check backstop; the CLI's own in-process refresh write no-ops on the RO mount by design — margins keep the orchestrator ahead of the CLI's 5-minute window), config.toml rendering with required=true gateway MCP servers, execpolicy deny rules (forbidden-only), per-role --sandbox (developer=workspace-write, review/doc roles read-only), codex exec --json with pinned ROBOCO_CODEX_CLI_MODEL (gpt-5.3-codex), usage summed from typed turn.completed events priced via the real 4-bucket split, dedicated image + entrypoint, registry/park/finalize/ compose/release wiring. V1 excludes interactive intake/secretary. Per adversarial review: migration 083 seeds the openai provider row enabled=True (without it every routing path 404'd — the whole feature was operationally dead code; grok needed the same seed in 039), the panel picker gained the OpenAI catalog group it silently lacked, and exit classification is structural — only stderr and error.message fields from error events are sniffed (word-boundaried patterns, exact auth phrases, bare 'login' dropped), so the model echoing on-topic words can never false-park the provider fleet-wide, proven by a benign-transcript test. Known open risk flagged, not claimed: whether codex's workspace-write OS sandbox excludes /app is unverified, and no hook mechanism exists to port the bash-guard defense-in-depth. * fix(providers): containment barrier on usage.json reads (code scanning) CodeQL flagged the codex usage read as path injection — correctly: os.path.basename does not neutralize '..', and the upstream segment validator isn't in CodeQL's taint model. The grok/codex reads collapse into one _read_usage_json_contained helper that resolves the built path and refuses anything outside the resolved usage root — a hostile id can never escape regardless of upstream drift. Traversal + containment regression tests added; a stray noqa in the test file replaced with a named constant per repo rule. * fix(providers): use realpath+startswith containment CodeQL recognizes The is_relative_to() guard was a real barrier but not in CodeQL's py/path-injection sanitizer model, so the alert persisted. Switch to the canonical os.path.realpath + startswith(root + os.sep) form, which CodeQL recognizes as a path-traversal barrier; behavior is identical (refuse any candidate resolving outside the usage root). * fix(providers): regexp-allowlist the usage-id segment (CodeQL barrier) Neither is_relative_to nor realpath+startswith was recognized by CodeQL's py/path-injection sanitizer model across the str->Path->open flow. Sanitize the tainted component at the source instead: the id must fullmatch a strict slug token ([A-Za-z0-9][A-Za-z0-9._-]*, no separators, no '..'), which CodeQL recognizes as a path-injection barrier; the realpath+startswith containment stays as defense-in-depth. * fix(providers): standalone regexp guard so CodeQL recognizes the barrier The sanitizer was one disjunct of a compound 'or' condition, which CodeQL's guard analysis does not trace as a barrier. Split the regexp fullmatch into its own single-condition guard (the redundant '..' check is dropped — the required alphanumeric first char already excludes it). --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -52,7 +52,16 @@ async def llm_setup(
|
||||
enabled=True,
|
||||
base_url="https://ollama.example.com",
|
||||
)
|
||||
db_session.add_all([anthropic, grok, ollama])
|
||||
# Mirrors migration 083_seed_openai_provider's contract: enabled=True at
|
||||
# seed time (no apply_mode="codex" write path exists to flip it later —
|
||||
# see that migration's docstring).
|
||||
openai = ProviderConfigTable(
|
||||
name="openai-test",
|
||||
type=ModelProvider.OPENAI,
|
||||
enabled=True,
|
||||
base_url="https://api.openai.com/v1",
|
||||
)
|
||||
db_session.add_all([anthropic, grok, ollama, openai])
|
||||
await db_session.flush()
|
||||
yield {"svc": ModelRoutingService(db_session)}
|
||||
|
||||
@@ -196,6 +205,18 @@ async def test_derive_mode_grok_when_only_grok_global(llm_setup: dict) -> None:
|
||||
assert await svc.derive_mode() == "grok"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_derive_mode_codex_when_only_openai_global(llm_setup: dict) -> None:
|
||||
"""A pure-OPENAI global assignment reports "codex", not the catch-all
|
||||
"mix" — the read-only branch derive_mode gained alongside the seed fix."""
|
||||
svc = llm_setup["svc"]
|
||||
codex_model = _first_model_for_type(ModelProvider.OPENAI)
|
||||
await svc.upsert_assignment(
|
||||
scope=AssignmentScope.GLOBAL, scope_value=None, model_name=codex_model
|
||||
)
|
||||
assert await svc.derive_mode() == "codex"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_derive_mode_mix_with_per_agent(llm_setup: dict) -> None:
|
||||
svc = llm_setup["svc"]
|
||||
@@ -393,6 +414,32 @@ async def test_resolve_for_agent_uses_global_assignment(
|
||||
assert route.model_name == model
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_upsert_and_resolve_openai_assignment_roundtrip(
|
||||
llm_setup: dict,
|
||||
) -> None:
|
||||
"""gpt-5.3-codex through upsert_assignment -> resolve_for_agent, against
|
||||
the seeded OPENAI row (migration 083). Before that seed existed,
|
||||
upsert_assignment's `_get_seeded_provider(ModelProvider.OPENAI)` lookup
|
||||
raised NotFoundError the moment anyone tried this — this is the round
|
||||
trip that would have caught it."""
|
||||
svc = llm_setup["svc"]
|
||||
codex_model = _first_model_for_type(ModelProvider.OPENAI)
|
||||
row = await svc.upsert_assignment(
|
||||
scope=AssignmentScope.AGENT_SLUG,
|
||||
scope_value="be-dev-1",
|
||||
model_name=codex_model,
|
||||
)
|
||||
assert row.model_name == codex_model
|
||||
|
||||
route = await svc.resolve_for_agent("be-dev-1")
|
||||
assert route.provider_type == ModelProvider.OPENAI
|
||||
assert route.model_name == codex_model
|
||||
# The seeded row carries no stored token — Codex authenticates via the
|
||||
# mounted ~/.codex subscription dir, not a decrypted provider token.
|
||||
assert route.auth_token is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_for_agent_uses_provider_token(llm_setup: dict) -> None:
|
||||
"""When provider has auth_token_encrypted, it's decrypted (lines 345-346)."""
|
||||
@@ -672,6 +719,23 @@ async def test_get_seeded_provider_unknown_raises(
|
||||
await svc._get_seeded_provider(ModelProvider.ANTHROPIC)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_upsert_openai_assignment_without_seed_raises_not_found(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""The exact pre-fix failure: assigning a catalog model whose provider
|
||||
type has no seeded `provider_configs` row raises NotFoundError out of
|
||||
`upsert_assignment`. This is what migration `083_seed_openai_provider`
|
||||
fixes — a bare session (no `llm_setup` fixture, so no OPENAI row) proves
|
||||
the seed is load-bearing, not incidental."""
|
||||
svc = ModelRoutingService(db_session)
|
||||
codex_model = _first_model_for_type(ModelProvider.OPENAI)
|
||||
with pytest.raises(NotFoundError):
|
||||
await svc.upsert_assignment(
|
||||
scope=AssignmentScope.GLOBAL, scope_value=None, model_name=codex_model
|
||||
)
|
||||
|
||||
|
||||
def test_get_model_routing_service_factory(db_session: AsyncSession) -> None:
|
||||
"""Factory wraps ModelRoutingService with the given session (line 369)."""
|
||||
|
||||
|
||||
@@ -0,0 +1,162 @@
|
||||
"""Migration 083 tests — seed_openai_provider.
|
||||
|
||||
Verifies the post-upgrade state and exercises the downgrade SQL ordering,
|
||||
mirroring ``test_migration_028_seed_self_hosted.py`` and
|
||||
``039_seed_grok_provider``'s own shape.
|
||||
|
||||
NOT a real alembic round-trip — the suite builds the test DB via
|
||||
Base.metadata.create_all (see conftest). Migration 083's upgrade()/downgrade()
|
||||
bodies are reviewed here; the tests guard the resulting DB-level contract —
|
||||
in particular ``enabled=True`` at seed time, the one detail that diverges
|
||||
from GROK's own seed (see the migration's docstring for why: there is no
|
||||
``apply_mode="codex"`` write path to flip it later).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.db.tables import ModelAssignmentTable, ProviderConfigTable
|
||||
from roboco.models.base import AssignmentScope, ModelProvider
|
||||
from sqlalchemy import text
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
_INSERT_SQL = text(
|
||||
"""
|
||||
INSERT INTO provider_configs
|
||||
(id, name, type, base_url, auth_token_encrypted, enabled, created_at)
|
||||
VALUES
|
||||
(
|
||||
gen_random_uuid(),
|
||||
'Codex (OpenAI)',
|
||||
'openai',
|
||||
'https://api.openai.com/v1',
|
||||
NULL,
|
||||
true,
|
||||
now()
|
||||
)
|
||||
ON CONFLICT (name) DO NOTHING
|
||||
"""
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migration_083_upgrade_insert_contract(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""The upgrade INSERT SQL seeds the Codex row ENABLED (unlike GROK's
|
||||
seed, which starts disabled) and is idempotent."""
|
||||
# --- First run: the row should be inserted.
|
||||
await db_session.execute(_INSERT_SQL)
|
||||
await db_session.flush()
|
||||
|
||||
result = await db_session.execute(
|
||||
text(
|
||||
"SELECT name, type, enabled, base_url "
|
||||
"FROM provider_configs "
|
||||
"WHERE name = 'Codex (OpenAI)'"
|
||||
)
|
||||
)
|
||||
rows = list(result)
|
||||
assert len(rows) == 1
|
||||
name, ptype, enabled, base_url = rows[0]
|
||||
assert name == "Codex (OpenAI)"
|
||||
assert ptype == "openai"
|
||||
# The load-bearing assertion: enabled=True at seed time. Seeding False
|
||||
# (GROK's convention) would leave resolve_for_agent silently falling back
|
||||
# to Anthropic forever, since no apply_mode="codex" write path exists to
|
||||
# flip it — the exact "unreachable" failure this migration fixes.
|
||||
assert enabled is True
|
||||
assert base_url == "https://api.openai.com/v1"
|
||||
|
||||
# --- Second run: ON CONFLICT DO NOTHING must not create a duplicate.
|
||||
await db_session.execute(_INSERT_SQL)
|
||||
await db_session.flush()
|
||||
|
||||
result = await db_session.execute(
|
||||
text("SELECT id FROM provider_configs WHERE name = 'Codex (OpenAI)'")
|
||||
)
|
||||
assert len(list(result)) == 1, (
|
||||
"Expected exactly one 'Codex (OpenAI)' row after two INSERT "
|
||||
"executions; ON CONFLICT DO NOTHING must prevent duplicates."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migration_083_downgrade_deletes_assignments_before_config(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""Downgrade SQL deletes model_assignments before provider_configs.
|
||||
|
||||
A FK RESTRICT constraint on model_assignments.provider_config_id means
|
||||
deleting provider_configs first would raise an IntegrityError.
|
||||
"""
|
||||
suffix = uuid4().hex[:8]
|
||||
openai = ProviderConfigTable(
|
||||
name=f"Codex (OpenAI)-test-{suffix}",
|
||||
type=ModelProvider.OPENAI,
|
||||
enabled=True,
|
||||
)
|
||||
db_session.add(openai)
|
||||
await db_session.flush()
|
||||
|
||||
assignment = ModelAssignmentTable(
|
||||
scope=AssignmentScope.AGENT_SLUG,
|
||||
scope_value=f"test-agent-{suffix}",
|
||||
provider_config_id=openai.id,
|
||||
model_name="gpt-5.3-codex",
|
||||
)
|
||||
db_session.add(assignment)
|
||||
await db_session.flush()
|
||||
|
||||
result = await db_session.execute(
|
||||
text("SELECT id FROM provider_configs WHERE name = :name").bindparams(
|
||||
name=openai.name
|
||||
)
|
||||
)
|
||||
assert result.scalar_one_or_none() is not None
|
||||
|
||||
result = await db_session.execute(
|
||||
text("SELECT id FROM model_assignments WHERE scope_value = :sv").bindparams(
|
||||
sv=assignment.scope_value
|
||||
)
|
||||
)
|
||||
assert result.scalar_one_or_none() is not None
|
||||
|
||||
# Step 1: delete referencing model_assignments first.
|
||||
await db_session.execute(
|
||||
text(
|
||||
"DELETE FROM model_assignments "
|
||||
"WHERE provider_config_id IN ("
|
||||
" SELECT id FROM provider_configs WHERE name = :name"
|
||||
")"
|
||||
).bindparams(name=openai.name)
|
||||
)
|
||||
# Step 2: now safe to delete the provider row.
|
||||
await db_session.execute(
|
||||
text("DELETE FROM provider_configs WHERE name = :name").bindparams(
|
||||
name=openai.name
|
||||
)
|
||||
)
|
||||
|
||||
result = await db_session.execute(
|
||||
text("SELECT id FROM provider_configs WHERE name = :name").bindparams(
|
||||
name=openai.name
|
||||
)
|
||||
)
|
||||
assert result.scalar_one_or_none() is None, (
|
||||
"provider_configs row should be deleted by downgrade"
|
||||
)
|
||||
|
||||
result = await db_session.execute(
|
||||
text("SELECT id FROM model_assignments WHERE scope_value = :sv").bindparams(
|
||||
sv=assignment.scope_value
|
||||
)
|
||||
)
|
||||
assert result.scalar_one_or_none() is None, (
|
||||
"model_assignments row should be deleted before provider_configs"
|
||||
)
|
||||
Reference in New Issue
Block a user