Fix relative settings file resolution when merging sandbox config#663
Fix relative settings file resolution when merging sandbox config#663shuofengzhang wants to merge 2 commits intoanthropics:mainfrom
Conversation
|
@claude review |
There was a problem hiding this comment.
LGTM — straightforward bug fix with good test coverage.
Extended reasoning...
Overview
This PR adds a _resolve_settings_path() helper to SubprocessCLITransport that resolves relative settings file paths against options.cwd instead of the process working directory. Two call sites in _build_settings_value() are updated to use it, and a regression test verifies the fix.
Security risks
None. The change only affects how an already-user-supplied path is resolved — no new file access patterns, no auth changes, no external input handling.
Level of scrutiny
Low scrutiny is appropriate. This is a small, focused bug fix (~15 lines of logic + ~25 lines of test) that follows existing patterns in the codebase. The helper is simple: expanduser(), check is_absolute(), prepend cwd if relative. The test uses tmp_path to create a real file and verify end-to-end correctness.
Other factors
The fix is logically correct — when the SDK needs to read a settings file on behalf of the CLI (to merge sandbox config), it should resolve relative paths the same way the CLI would, which is relative to cwd. The test directly exercises the bug scenario described in the PR description. No outstanding reviewer comments.
What changed
settingsfile paths againstcwdwhen sandbox settings are merged inSubprocessCLITransport._resolve_settings_path()helper to centralize settings path resolution.settings="settings.json"+cwd=<dir>+sandbox=...correctly loads and merges the settings file.Why
When
sandboxis provided, the SDK parses settings files itself before passing merged JSON to the CLI. Previously, relativesettingspaths were resolved from the process working directory instead ofcwd, which could cause the SDK to miss valid settings files and drop existing settings unexpectedly.Testing
pytest -q