fix(model_fallbacks): use dotted slugs for codex subscription catalog - #4015
Closed
ColeMatthewBienek wants to merge 17 commits into
Closed
fix(model_fallbacks): use dotted slugs for codex subscription catalog#4015ColeMatthewBienek wants to merge 17 commits into
ColeMatthewBienek wants to merge 17 commits into
Conversation
Adds a nullable scheduled_tasks.project_id, plumbed through the store, REST routes, and sys_scheduled_task_* MCP tools. Create/update reject a project_id that doesn't exist or isn't owned by the caller; the fire path files the session after create but soft-fails (logs, keeps the run alive) if the project has since vanished.
Projects are owned by the RAW user_id (None in single-user/local mode), never RESERVED_USER_LOCAL — that folding is specific to the LEVEL_OWNER session grant. The fire path's project lookup was folding task.user_id to "local" before calling project_store.get, so every local-mode project looked owner-mismatched and was treated as vanished, leaving fired sessions unfiled even with a valid project. Also warns when set_conversation_project() reports no metadata row to file into, and hardens the fake ProjectStore in tests to enforce owner equality like the real store.
Round 2 fix: the previous round's raw-user_id lookup only worked for auth-disabled (owner None) and multi-user (real ids) — it broke the actual production posture, OMNIGENT_LOCAL_SINGLE_USER=1, where the auth provider resolves a headerless request to the literal "local" identity and project CRUD stores that raw value. Scheduled tasks normalize "the single local user" to user_id=None regardless of which of the two local modes produced it, so a bare None can't tell them apart at fire time. Adds AuthProvider.is_local_single_user() and a single shared omnigent.server.auth.resolve_project_owner(user_id, local_single_user) that reconstructs the correct project-owner scope from a normalized owner value. Both the scheduled-tasks route validation (create AND update, which share one helper) and the fire path call this same function, sourced from the same auth_provider instance at wiring time in app.py, so the two call sites cannot drift apart again.
…ing it at fire time Round 3 fix: the round-2 fix reconstructed a task's project-owner scope from the CURRENT server's auth mode every time it fired, but scheduled_tasks.user_id=None carries no provenance about which mode was active when the task was created. A restart that flips OMNIGENT_LOCAL_SINGLE_USER between a task's creation and a later fire made a still-valid project's owner scope disagree with what the project was actually created under, and the project would look vanished. Adds a nullable scheduled_tasks.project_owner column, written once at create/update validation time (create/update already resolved and confirmed the project under the true owner scope — this just persists that result instead of discarding it). The fire path reads it directly via a new decode_scheduled_task_project_owner, removing the unsafe re-resolution entirely for any task created under this column. A NULL project_owner (a row with project_id set from before this column existed) is the one accepted gap: decode falls back to the old current-mode resolution, explicitly documented as a one-time migration-window limitation, not silently masked. A resolved-to-None owner (auth fully disabled) is persisted as the "" sentinel so it is never confused with that NULL/legacy case. Also narrows UnifiedAuthProvider.is_local_single_user() to be source-aware — it no longer reports True for an oidc/accounts provider that happens to be constructed with local_single_user=True, since only header mode ever consults that flag.
…on-fire Round 4 fix: a legacy row (project_id set, project_owner NULL) only got project_owner written when a PATCH itself touched project_id. Any other PATCH (rename, RRULE change, ...) left it NULL forever, so the row stayed mode-dependent and could still soft-fail after an auth-mode switch indefinitely. Adds two deterministic healing paths so the live fallback is bounded to "until the row is next touched": - Heal-on-update: any successful PATCH on a task with project_id set and project_owner NULL resolves the owner under the current mode and persists it, regardless of which fields the PATCH actually changed. - Heal-on-fire: when a fire takes the legacy NULL fallback and the project lookup succeeds, the resolved owner is written back onto the row so the fallback (and its live re-resolution) happens at most once per legacy row. Also documents the cross-restart test's unit-boundary choice (exercises build_on_fire directly rather than a full two-app restart flow), per review request to fold in or document.
Round 5 fix: heal-on-update wrote the current-mode-resolved owner unconditionally on any unrelated PATCH, without confirming the project actually exists under that scope. A legacy project owned by None (created auth-disabled), touched by an unrelated rename while the server runs local-single-user, would get "local" written even though it doesn't match -- permanently converting a recoverable legacy row (project_owner NULL, still falls back) into a wrong non-legacy one (decode no longer falls back), breaking filing even after the mode reverted. Adds _heal_legacy_project_owner, mirroring heal-on-fire's existing discipline: resolve, then a scoped project_store.get, and persist project_owner ONLY on a confirmed hit. On a miss, the row is left untouched (project_owner stays NULL) so the live fallback remains available for a later heal or fire. Also documents (rather than guards) the inert heal-on-fire race with a concurrent project_id-clearing PATCH, per review's fold-in-if-cheap guidance -- a conditional/CAS update isn't worth it for a single stray column value with no observable effect. Regression tests: a route-level miss (project_owner stays NULL across the mismatched-mode PATCH) and its fire-level companion (mismatched- mode fire also leaves the row legacy, and a later fire in the correct mode still resolves and files).
Round 6 fix: _heal_legacy_project_owner's scoped project_store.get() was uncaught, so a transient project-store error during an unrelated PATCH of a legacy task (rename, RRULE change, ...) would 500 and block the user's own update -- the heal is optional and must never do that. Wraps the resolve+lookup in try/except, logging and treating any failure exactly like a miss (project_owner stays NULL, still recoverable). Mirrors heal-on-fire, which already never lets a healing failure break the run. Also corrects the heal-on-fire race comment: an unrelated PATCH does NOT self-correct a stale project_owner left by the race (heal-on-update only touches project_owner when it's NULL) -- only a PATCH that itself sets project_id again, or deleting the task, does. Regression test: project_store.get raising during an unrelated PATCH of a legacy row -> PATCH still succeeds (200, rename applied), project_owner stays NULL, the failure is logged.
…sessions into a project
Codex's ChatGPT-subscription backend only accepts dotted marketing slugs (gpt-5.6-sol, gpt-5.6-luna, gpt-5.6-terra, gpt-5.5), not the dashed vendor ids. The subscription/CLI-config static fallback catalog was pinning the dashed spelling, so overriding to one of these models under ChatGPT auth 400'd with codex_turn_error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
@ColeMatthewBienek This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder. These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:
Use |
13 tasks
Contributor
|
Closed. If you want to pick this back up, comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
Closes #
Summary
Codex sub-agents were failing with HTTP 400
codex_turn_error: "The 'gpt-5-6-terra' model is not supported when using Codex with a ChatGPT account." This started when Codex CLI auth switched to a ChatGPT-subscription login (auth_mode=chatgpt, noOPENAI_API_KEY).Root cause:
_CODEX_MODELSinomnigent/model_fallbacks.pyused the dashed vendor ids (gpt-5-6-sol,gpt-5-6-luna,gpt-5-6-terra,gpt-5-5). This tuple backs the(SUBSCRIPTION_KIND, "codex")and(CLI_CONFIG_KIND, "codex")static fallbacks — the ChatGPT-subscription path. Codex's ChatGPT-subscription backend only accepts the dotted marketing slugs (gpt-5.6-sol,gpt-5.6-luna,gpt-5.6-terra,gpt-5.5), which is what a user's~/.codex/config.toml/~/.codex/models_cache.jsonactually use under ChatGPT auth. When an omnigent codex launch applied a model override from this catalog,_pin_codex_config_modelwrote the dashed id into the sessionconfig.tomland ChatGPT-auth Codex 400'd._CODEX_MODELSto the dotted slugs.model_catalog.resolve_catalog_model("databricks", ...)and_databricks_*code) untouched — dashed ids are correct there._MECHANICAL_VENDOR_ID_REinomnigent/model_override.pyalready permits.in the tail, sonormalize_model_for_provider/canonical_model_spellingpass the dotted ids through unchanged for vendor-direct/subscription kinds — no change needed there.tests/test_model_catalog.py).Test Plan
uv run python -m pytest tests/test_model_catalog.py tests/dev/lint/test_lint_no_hardcoded_models.py -q→ 75 passeduv run python -m pytest tests/test_model_override.py tests/repl/test_repl_pending_model_override.py tests/runtime/test_model_override.py tests/server/integration/test_sessions_model_override.py tests/server/test_smart_routing.py tests/test_codex_native_app_server.py -q→ 218 passeduv run pre-commit run --files omnigent/model_fallbacks.py tests/test_model_catalog.py→ ruff format/check pass; pre-existingpyreflyandno-hardcoded-modelsfailures reproduce identically onmain(unrelated files:cursor_executor.py,kubernetes.py,examples/polly/*.yaml) — confirmed viagit stashdiff, not introduced by this changeuv run pyrefly check 2>&1 | grep -i "model_fallbacks\|test_model_catalog"→ no output (no errors touching either changed file)Demo
N/A — non-visual, backend model-id fix.
Type of change
Test coverage
Coverage notes
N/A — updated the existing unit test asserting the codex subscription/CLI-config static catalog.
Changelog
Fixed Codex sub-agents 400'ing with
codex_turn_errorunder ChatGPT-subscription auth by using the dotted model slugs (gpt-5.6-sol, etc.) Codex-on-ChatGPT expects