Skip to content

fix(codex): ignore user config in subprocesses#488

Open
andrew-adamson wants to merge 1 commit intoQ00:mainfrom
andrew-adamson:fix/codex-ignore-user-config
Open

fix(codex): ignore user config in subprocesses#488
andrew-adamson wants to merge 1 commit intoQ00:mainfrom
andrew-adamson:fix/codex-ignore-user-config

Conversation

@andrew-adamson
Copy link
Copy Markdown

Summary

  • add --ignore-user-config to Codex subprocess invocations so runtime behavior does not inherit local user config unexpectedly
  • update the runtime and adapter coverage to match the new invocation shape
  • keep the tests portable across path semantics used in local environments

Testing

  • uv run pytest tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/providers/test_codex_cli_adapter.py

Copy link
Copy Markdown
Contributor

@ouroboros-agent ouroboros-agent Bot left a comment

Choose a reason for hiding this comment

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

Review — ouroboros-agent[bot]

Verdict: REQUEST_CHANGES

Reviewing commit 8127f13 for PR #488

Review record: baad0e3f-6896-40a8-97ef-b7c1ecf4dee6

Blocking Findings

| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.

---|-----------|----------|---------|
| 1 | src/ouroboros/orchestrator/codex_cli_runtime.py:555 | BLOCKING | Forcing codex exec --ignore-user-config breaks Ouroboros’ own Codex integration contract. setup.py explicitly registers the Ouroboros MCP/env hookup in ~/.codex/config.toml; skipping user config means runtime-launched Codex sessions will no longer load that registration, so tool-backed flows that depend on the configured MCP server stop working in production even though setup succeeded. There is no compensating path in this diff and no test covering execution with the configured Codex MCP hookup enabled. |
| 2 | src/ouroboros/providers/codex_cli_adapter.py:361 | BLOCKING | The adapter now also suppresses ~/.codex/config.toml unconditionally. That removes any Codex-side MCP/tool configuration the caller relies on, and for Ouroboros specifically it bypasses the setup-managed MCP/env hookup the project tells users to install there. Since this adapter allows tool-using completions (allowed_tools defaults to unrestricted), this is a behavior regression rather than a harmless isolation change, and the added tests only assert the flag is present instead of covering the configured-tool path that now breaks. |

Non-blocking Suggestions

None.

Design Notes

The change is mechanically small but crosses a real contract boundary: setup/uninstall treat ~/.codex/config.toml as the authoritative Codex integration point, while these command builders now opt out of reading it entirely. If isolation is needed, it should be selective rather than disabling the project’s own configured Codex environment.


Reviewed by ouroboros-agent[bot] via Codex deep analysis

@shaun0927
Copy link
Copy Markdown
Collaborator

I don't think we should merge this in its current form.

The isolation goal is understandable, but forcing codex exec --ignore-user-config crosses a real contract boundary for this repo. We explicitly treat ~/.codex/config.toml as the setup-managed Codex MCP/env hookup point. If we unconditionally ignore user config here, we are not just filtering unrelated local customization — we are also potentially disabling Ouroboros's own Codex-side integration path.

That makes this too broad.

If there is a specific inheritance problem we want to solve here, please narrow it first:

  • parent session/thread inheritance,
  • wrapper resolution drift,
  • or a particular user-config side effect.

Then solve that targeted problem directly instead of globally opting out of the configured Codex environment.

At minimum, any version of this change needs coverage showing that a setup-managed Codex MCP/env hookup still works end-to-end after the change. Right now the regression risk is larger than the isolation benefit.

@andrew-adamson
Copy link
Copy Markdown
Author

I agree that this is a bit of a blunt instrument. The intent was to be able to have different settings for codex invocations that are run by ouroboros vs those I've initiated interactively from the desktop or cli. For instance, when I'm working on small tasks interactively without ouroboros, I want the best model, fast mode, and I want particular notify actions (sounds), that I don't want when ouroboros is using codex. I also don't necessarily want certain mcp configs enabled for ouroboros workers.

Digging deeper, I think the best way to do this might be to use codex profiles, which are still in the config.toml file, and then modify ouroboros to take a codex_profile config setting that gets passed to codex exec --profile through the runtime command builder. Does that sound sane, and if so, do you want it as a new PR?

@andrew-adamson
Copy link
Copy Markdown
Author

The profile selection approach might also be a good way to implement the reasoning tier levels needed for #184

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