Skip to content

feat: Add support for Codex - #19

Merged
thomass-dev merged 8 commits into
probabl-ai:mainfrom
glemaitre:feat/codex-harness
Aug 25, 2026
Merged

feat: Add support for Codex#19
thomass-dev merged 8 commits into
probabl-ai:mainfrom
glemaitre:feat/codex-harness

Conversation

@glemaitre

@glemaitre glemaitre commented Aug 7, 2026

Copy link
Copy Markdown
Member

Add support for the codex harness

requires https://github.com/probabl-ai/skore-hub/pull/1754

@glemaitre
glemaitre marked this pull request as draft August 7, 2026 21:40
@rouk1
rouk1 marked this pull request as ready for review August 20, 2026 14:56
@rouk1

rouk1 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Now that response api is merge this is ready !

@rouk1

rouk1 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

I updated this PR to ensure existing codex configuration is not override by the command.

@thomass-dev may I ask for a review ?

@thomass-dev

thomass-dev commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Cursor review

Blocker: merge still hijacks the global default

The latest commit says existing Codex config is not fully overwritten. Other providers are kept, but every configure/launch still sets user-level:

  • model = "skore-agent"
  • model_provider = "skore"
    The CLI even prints that plain codex now defaults to Skore in every project. That is a global default change, not a merge of a provider. Anyone who uses Codex with OpenAI/OpenRouter elsewhere will get Skore until they edit
    ~/.codex/config.toml again — and the next skore agent will put it back.
    If the product intent is “add Skore, leave my default alone”, stop writing those two root keys (or restore them after the session). If the intent is “this command makes Codex use Skore”, keep the write but make the warning much
    louder; the commit message currently oversells what merge does.

Other issues

TOML is string-patched, not parsed. _CODEX_PROVIDER_SECTION and _set_toml_root_string work for the files this code writes. They will mis-handle nested tables under [model_providers.skore], quoted keys, or an API key
containing ". Copilot went the parse-and-rewrite route; Codex should too if you keep mutating user config.
_launch_codex is less defensive than Copilot. Missing project file is handled; corrupt project TOML raises tomllib.TOMLDecodeError instead of a RuntimeError with a “run skore agent first” hint.
Launch ignores project model. It re-syncs with the model_id argument, not .codex/skore-provider.toml. Fine today (skore-agent everywhere), brittle if those ever diverge.
Secrets live in ~/.codex/config.toml. Justified by Codex’s env_key behavior, and the project copy is gitignored. Still a durable hub key in a user file that is not removed on exit.
History. Two _ commits plus a Copilot upsert in the same Codex PR. Squash (or split Copilot) before merge.
Repo .gitignore. Adds .skore, opencode.json, Copilot/Pi/Codex byproducts. Useful for dogfooding this repo, not required for the feature.

Verdict

Feature direction matches Copilot’s constraints. I would not merge until you decide whether model / model_provider should be rewritten globally — that is the real behavior change, and it contradicts the “do not override” commit. After that, parse TOML instead of regex, and squash the _ commits.

@thomass-dev thomass-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Check my previous comment

rouk1 added 2 commits August 21, 2026 11:00
Write the Skore provider to a dedicated ~/.codex/skore.config.toml
profile and launch `codex --profile skore` instead of upserting root
model/model_provider keys into the user's config.toml. Plain codex now
keeps the user's default model; the user config file is never read,
parsed or rewritten. Also: corrupt project TOML raises a RuntimeError
with a 'run skore agent' hint, launch prefers the model recorded in the
project provider file, all written values are TOML-escaped, the profile
respects CODEX_HOME, and the dogfooding .gitignore entries are dropped.

Note for testers of earlier revisions: remove the root model and
model_provider keys (and the [model_providers.skore] table) that the
old code wrote to ~/.codex/config.toml — this revision no longer
rewrites that file. The profile mechanism requires Codex >= 0.134.
Drop the ~/.codex/skore.config.toml profile: it is still a single
user-level file, so two worktrees would fight over it. The project
file .codex/skore-provider.toml is now the only thing skore writes,
and _launch_codex passes the provider to codex as runtime --config
overrides (model_provider + model_providers.skore.*) since Codex
denies model_providers in project config files. The hub key travels
in the process environment and is referenced via env_http_headers,
so it never appears on the command line; plain codex keeps the
user's default model and ~/.codex is never read or written.
@rouk1

rouk1 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

New design: fully project-local
The ~/.codex/skore.config.toml profile is gone (it was still one user-level file shared across worktrees). Now:

  • Configure writes only .codex/skore-provider.toml in the workspace (gitignored) — nothing outside the project is ever read or written, so every worktree carries its own model/URL/key.
  • Launch passes the provider to Codex as runtime --config overrides (src/skore_cli/agent/_harnesses.py:275):
codex --model <project-model> \
  --config model_provider="skore" \
  --config model_providers.skore.name="Skore Agent" \
  --config model_providers.skore.base_url="" \
  --config model_providers.skore.wire_api="responses" \
  --config 'model_providers.skore.env_http_headers={ "X-API-Key" = "SKORE_AGENT_API_KEY" }'

with SKORE_AGENT_API_KEY set only in the exec'd process env.
Why this works (verified in the Codex Rust source):

  • Project files can't declare model_providers (PROJECT_LOCAL_CONFIG_DENYLIST), but runtime --config layers sit above user config and can (ArgAction::Append, dotted paths, TOML values — utils/cli/src/config_override.rs).
  • env_http_headers maps header → env var read at request time (model-provider-info/src/lib.rs:274) — the key never lands on the command line (ps-safe).
  • Overrides insert leaf keys, so a user's own providers in config.toml survive untouched (config/src/overrides.rs).
    Named SKORE_AGENT_API_KEY to avoid colliding with the CLI's existing SKORE_HUB_API_KEY (src/skore_cli/_hub_auth.py:12). Tests assert the overrides parse as valid TOML, the raw key is absent from argv, and no ~/.codex path is ever created.

@rouk1
rouk1 requested a review from thomass-dev August 24, 2026 09:29

@thomass-dev thomass-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you please fix the merge-conflict?

@rouk1

rouk1 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Could you please fix the merge-conflict?

Done!

@thomass-dev
thomass-dev merged commit 2be473d into probabl-ai:main Aug 25, 2026
15 checks passed
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.

3 participants