fix(core): preserve config.toml symlinks on write#9437
fix(core): preserve config.toml symlinks on write#9437ryoppippi wants to merge 1 commit intoopenai:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Thanks for the PR. Our contribution guidelines indicate that we're not currently accepting PRs for new features. The linked issue is a feature request, so I would normally close this PR. However, one could argue that this is a bug fix because Codex does follow symlinks for |
|
I've reviewed your PR. I think it's directionally correct, but it has a lot of shortcomings:
I've prepared a separate PR that addresses these issues. I welcome you to review and comment on it. I'm going to close this PR. Thanks again for taking the time to post it. |
We already support reading from `config.toml` through a symlink, but the code was not properly handling updates to a symlinked config file. This PR generalizes safe symlink-chain resolution and atomic writes into path_utils, updating all config write paths to use the shared logic (including set_default_oss_provider, which previously didn't use the common path), and adds tests for symlink chains and cycles. This resolves #6646. Notes: * Symlink cycles or resolution failures replace the top-level symlink with a real file. * Shared config write path now handles symlinks consistently across edits, defaults, and empty-user-layer creation. This PR was inspired by #9437, which was contributed by @ryoppippi
We already support reading from `config.toml` through a symlink, but the code was not properly handling updates to a symlinked config file. This PR generalizes safe symlink-chain resolution and atomic writes into path_utils, updating all config write paths to use the shared logic (including set_default_oss_provider, which previously didn't use the common path), and adds tests for symlink chains and cycles. This resolves openai#6646. Notes: * Symlink cycles or resolution failures replace the top-level symlink with a real file. * Shared config write path now handles symlinks consistently across edits, defaults, and empty-user-layer creation. This PR was inspired by openai#9437, which was contributed by @ryoppippi
We already support reading from `config.toml` through a symlink, but the code was not properly handling updates to a symlinked config file. This PR generalizes safe symlink-chain resolution and atomic writes into path_utils, updating all config write paths to use the shared logic (including set_default_oss_provider, which previously didn't use the common path), and adds tests for symlink chains and cycles. This resolves openai#6646. Notes: * Symlink cycles or resolution failures replace the top-level symlink with a real file. * Shared config write path now handles symlinks consistently across edits, defaults, and empty-user-layer creation. This PR was inspired by openai#9437, which was contributed by @ryoppippi
Fixes #6646
What
When Codex writes to
~/.codex/config.toml, we persist via a temporary file + rename. Ifconfig.tomlis a symlink (e.g. managed externally), renaming over it replaces the symlink with a regular file.This PR follows the symlink chain and atomically writes to the final target file instead, preserving the symlink.
Changes
config.tomlsymlink chain and write to the final target pathTesting
nix develop -c cargo test -p codex-core