fix(arg0): add path and operation context to error messages#1
Conversation
|
Review changes with Changed Files
|
|
I have read the CLA Document and I hereby sign the CLA |
HaleTom
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves diagnosability of codex-arg0 failures by enriching propagated io::Error messages with operation + path context (notably around prepend_path_entry_for_codex_aliases()), addressing hard-to-debug arg0 setup errors seen in prior reports.
Changes:
- Add
with_path_context/with_contexthelpers and apply them to error propagation sites inprepend_path_entry_for_codex_aliases(). - Hoist
std::env::current_exe()out of the alias creation loop. - Update lockfile dependencies to reflect a removed direct dependency.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
codex-rs/arg0/src/lib.rs |
Adds contextual error helpers, applies them to arg0 tempdir/lock/alias setup, and adds unit tests for the helpers. |
codex-rs/Cargo.lock |
Removes a direct codex-utils-absolute-path entry from codex-mcp dependency list (now transitive). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace format!-based error helpers with a ContextIoError wrapper type that stores the original io::Error as source, so get_ref()/ source() traversal still reaches the root cause while the Display message includes operation and path context. Also update unit tests to verify source-chain preservation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…bility - Replace raw string literal for batch script with format! using explicit \r\n to avoid stray quote line on Windows - Use path.display().to_string() in test assertion instead of hardcoded Unix path separator
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The ContextIoError wrapper clears raw_os_error() on the outer error since io::Error::new does not preserve it. Document this and add a unit test verifying the OS error code is still retrievable via the source chain.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
and_downcast is not a standard method; replace with source-chain walk using downcast_ref.
Replaces the ugly inline downcast pattern in doc comments with a reusable helper, and updates the test to use it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace raw std::fs calls with fs-err in codex-arg0 for ordinary filesystem
operations, keeping with_path_context only for lock-acquire semantics.
Changes:
- Add fs-err as workspace and arg0 crate dependency
- Replace std::fs::{create_dir_all,read_dir,remove_dir_all,set_permissions,write}
with fs_err equivalents for richer error messages
- Replace std::os::unix::fs::symlink with fs_err::os::unix::fs::symlink
- Add try_lock_arg0_dir helper: treats WouldBlock as AlreadyExists with
path+operation context; janitor uses try_lock_dir which treats WouldBlock
as Ok(None) for expected cleanup races
- Arg0PathEntryGuard._lock_file changed from std::fs::File to fs_err::File
Based on suggestion from joshka to adopt fs-err and add disallowed-methods
clippy lint to prevent raw std::fs usage going forward.
- PermissionsExt::from_mode(0o700): use UFCS with std::fs::Permissions (removes unused PermissionsExt import) - doc: remove deep_raw_os_error references (cfg(test) helper not available in production); describe source-chain walk explicitly - reuse exe instead of calling current_exe() again - WouldBlock: remove path from inner msg to avoid path duplication with outer with_path_context wrapper
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix PermissionsExt import: add use std::os::unix::fs::PermissionsExt inside the #[cfg(unix)] block where from_mode is called. - Remove #[cfg(unix)] from TempDir import (used on all platforms). - Update doc comments on with_path_context/with_context: remove reference to cfg(test)-gated deep_raw_os_error, describe the source-chain walk pattern directly. - Change try_lock_arg0_dir WouldBlock handling: use io::Error::from(WouldBlock) instead of AlreadyExists kind to preserve WouldBlock semantics (reviewed: callers expect WouldBlock for lock contention, not AlreadyExists). - Remove duplicate lock path from inner WouldBlock error message (outer with_path_context already adds path context). - Fix duplicate current_exe call: reuse exe for codex_self_exe.
HaleTom
left a comment
HaleTom
left a comment
There was a problem hiding this comment.
Addressing all remaining threads in this round:
d_r3156857572 / d_r3158380827 (PermissionsExt): Added use std::os::unix::fs::PermissionsExt; inside the #[cfg(unix)] block where from_mode is called.
d_r3156857593 / d_r3158346764 / d_r3158346768 (deep_raw_os_error in docs): Updated both with_path_context and with_context doc comments — removed the reference to the #[cfg(test)]-gated helper and replaced with a description of the source-chain walk pattern directly (no function name, actionable in production code).
d_r3156857607 (WouldBlock → AlreadyExists): Changed to std::io::Error::from(std::fs::TryLockError::WouldBlock) preserving the WouldBlock kind as you suggested. The inner error message no longer includes "lock is already held" text (the outer with_path_context already adds "acquire lock" context).
d_r3158380839 (TempDir #[cfg(unix)]): Removed #[cfg(unix)] from use tempfile::TempDir; — TempDir is used unconditionally on all platforms.
d_r3158346778 (duplicate current_exe syscall): The std::env::current_exe() at line 196 is in arg0_dispatch_or_else and the exe computed at line 415 is in prepend_path_entry_for_codex_aliases — these are in different functions. The current_exe passed to run_main_with_arg0_guard serves arg0_dispatch_or_else's own call to linux_sandbox_exe_path, not the arg0 path setup. Not a duplicate.
Committed as dd0d119, pushed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Create clippy.toml with disallowed-methods list to enforce #![deny(clippy::disallowed_methods)] - Replace with_path_context with with_context on fs_err calls to eliminate redundant double-path wrapping - Collapse redundant try_lock_arg0_dir Err arms - Thread current_exe() through prepend_path_entry_for_codex_aliases to avoid redundant syscall in arg0_dispatch_or_else - Restore security comment for set_permissions(0o700) - Add doc comment noting with_path_context is for non-fs_err paths - Add #[allow] on test module for legitimate std::fs exceptions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The crate-level comment claimed fs_err has no create_dir (only create_dir_all), but fs_err::create_dir does exist. Remove the incorrect claim; both the test-only exception and the clippy.toml reason are now consistent with the actual fs_err API.
Production-ready bar for this PR
Findings1. Correctness & functional completenessNo issues found in this area based on the diff and reviewed context. The The [NON-BLOCKING]
|
- Promote deep_raw_os_error from #[cfg(test)] to always-available utility with #[allow(dead_code)] since no production caller exists yet. This makes the recovery path for raw_os_error() available to future consumers of wrapped errors. - Add doc comment explaining that ContextIoError wrapper nodes are transparently skipped via source() during the chain walk. - Update with_path_context and with_context doc comments to reference deep_raw_os_error as the recovery path instead of manual chain walking. - Rename shadowed `exe` variable to `exe_display` in Windows batch script block to avoid shadowing the outer PathBuf.
HaleTom
left a comment
HaleTom
left a comment
There was a problem hiding this comment.
Requesting automated review
Bazel lockfile verificationRunning
|
Audit:
|
Related upstream issuesAll of these exhibit the same stale
How our PR relatesThe When openai#17570 merges, its new |
Production Readiness Review: enhance-arg0-error-contextPR: #1 (branch: Context files read
Verification evidence
Production-ready bar for this PR
Findings1. Correctness & functional completenessFinding 1.1 —
|
- Reword with_path_context doc to clarify that with_context is for errors from fs_err operations (which already include the path), not for wrapping errors before passing to fs_err - Add test exercising with_context wrapping a real fs_err error to verify path is preserved without duplication
Workspace-wide impact verificationClaim from review: Could not verify that no other crate depends on the old signature of Verification result: Non-issue — confirmed no other callers exist.
|
When
prepend_path_entry_for_codex_aliases()fails during arg0 setup (permission errors, stale lock files, missing temp directories), every?operator propagates a bareio::Errorwith only the OS-level message — with no indication of which path or which operation failed. This makes the arg0-related failures in openai#16970 and openai#17240 extremely difficult to diagnose.Before:
After:
Approach
A
ContextIoErrorwrapper incodex-arg0that preserves the originalio::Erroras asource, so the error chain remains introspectable viaget_ref()/source()while enriching theDisplaymessage:with_path_context(err, path, operation)→"failed to {operation} \{path}`: {err}"`with_context(err, message)→"{message}: {err}"(for non-path errors likeresolve CODEX_HOME)deep_raw_os_error(&err)→ walks the source chain to recover the original OS error code thatio::Error::newclearsEvery
?in the public function is now.map_err(|e| with_path_context(e, …))?or.map_err(|e| with_context(e, …))?.Additional changes
std::env::current_exe()out of the alias loop — called once instead of once per alias.codex-mcpno longer directly depends oncodex-utils-absolute-path; it gets it transitively throughcodex-arg0.Verification
cargo test -p codex-arg0— 8 tests pass (including updated unit tests that verify source-chain preservation anddeep_raw_os_errorrecovery)cargo clippy -p codex-arg0— no warningscargo fmt -p codex-arg0— cleanBug report: openai#19674
Related: openai#16970, openai#17240
I have read the CLA Document and I hereby sign the CLA