Skip to content

Fix relative settings file resolution when merging sandbox config#663

Open
shuofengzhang wants to merge 2 commits intoanthropics:mainfrom
shuofengzhang:fix/resolve-relative-settings-path
Open

Fix relative settings file resolution when merging sandbox config#663
shuofengzhang wants to merge 2 commits intoanthropics:mainfrom
shuofengzhang:fix/resolve-relative-settings-path

Conversation

@shuofengzhang
Copy link
Copy Markdown

What changed

  • Resolve relative settings file paths against cwd when sandbox settings are merged in SubprocessCLITransport.
  • Add _resolve_settings_path() helper to centralize settings path resolution.
  • Add a regression test to verify that settings="settings.json" + cwd=<dir> + sandbox=... correctly loads and merges the settings file.

Why

When sandbox is provided, the SDK parses settings files itself before passing merged JSON to the CLI. Previously, relative settings paths were resolved from the process working directory instead of cwd, which could cause the SDK to miss valid settings files and drop existing settings unexpectedly.

Testing

  • pytest -q

@km-anthropic
Copy link
Copy Markdown
Collaborator

@claude review

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

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.

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.

2 participants