Repository navigation
Malformed inline settings is silently discarded with sandbox; the file form raises a bare JSONDecodeError #1335
Description
Activity
I'd like to work on this. Two quick questions before I start: (1) do you accept external PRs on this repo? (2) For the fix, I'd parse the inline settings string strictly on malformed JSON, raise a clear error identifying it as the inline settings string (not a file), and make the file-form error name the file while keeping the documented missing-file degradation as-is. Is raising the behavior you'd want here, or would you prefer a fallback? Happy to implement whichever you choose, with regression tests covering the behavior matrix in the issue.
Thanks — and yes to your first question: external PRs are welcome here, and this one is a good candidate precisely because you have already read the issue.
I want to save you from duplicating work, so here is where I actually left it. I did not implement anything, as the issue says. What I did do was pin the reproduction to a specific commit and enumerate the behaviour matrix, which is the part that is slow to get right. At
37422c2, calling the real_build_settings_value():valid JSON + sandbox -> {"model": "sonnet", "sandbox": {"enabled": true}} MALFORMED + sandbox -> {"sandbox": {"enabled": true}} MALFORMED, no sandbox (pass-through) -> {"model":"sonnet",} path to MALFORMED file + sandbox -> RAISED JSONDecodeError: Illegal trailing comma before end of object nonexistent path + sandbox -> {"sandbox": {"enabled": true}}with
MALFORMEDbeing'{"model":"sonnet",}'. Two details in there are worth having before you write the test, because they are what make the bug a bug rather than a preference:The silent case is the loud-looking one. With
sandboxset, one trailing comma costs the entire settings object and the run proceeds looking healthy. Withoutsandbox, the same bytes pass through verbatim and the CLI rejects them outright. So the dangerous configuration is the quieter one — that inversion is the argument for raising rather than degrading.And the warning text at
subprocess_cli.py:507-509reports "treating as file path" while the "file path" is the JSON text itself. It never says the settings were dropped.On your question (2): your plan matches what I would choose, and I agree with the shape of it — raise for the inline case, keep the documented missing-file degradation, and make the file-form error name the file. The
_build_command()placement is the other half: it is called atsubprocess_cli.py:812, one line above thetry:that starts at:813, so aJSONDecodeErrorfrom reading the settings file escapes theClaudeSDKErrorhierarchy and never sets_exit_error. If you do both, note that the file-form path also needs the move, or the error shape stays inconsistent between the two forms you are deliberately distinguishing.I have no intention of opening a competing PR, so please go ahead. Two things that would help a reviewer on your side: if you can, say which commit you tested against, since this area has been refactoring; and the regression test is worth asserting on the warning as well as the raised error, because the warning naming a nonexistent file path is the part a reader would otherwise believe.
If it would be more useful I can also review your PR when it is up — I have no commit rights to this repo's branch protections but I can run the matrix. Otherwise, good luck.
Thanks for the detailed spec implemented exactly per your direction and the PR is up: #1352. Strict inline parse, file-form error names the file, missing-file degradation kept, both paths moved inside the try: so errors stay in the ClaudeSDKError hierarchy. Tests assert on the warning contents too, and I stated the tested commit in the description. Would appreciate your review when you have a moment.
Closing the loop on my side: I have reviewed #1352 at
755bd5a3and left the detail there — short version,tests/test_transport.pyis 209 passed, the wider suite is 1401 passed, ruff and mypy are clean, and reverting only the source file while keeping your tests gives 3 failures, so the regression tests genuinely pin the fix.Nothing blocking from me, and I have left the approval to whoever owns the repo.
I am marking this as done from my end so the issue does not sit open waiting on me. If the maintainers send it back with changes, or if you want a second pass on a follow-up commit, say so and I will pick it up.
Two things go wrong with
options.settings, in opposite directions, and neither matches the documented contract.ClaudeAgentOptions.settingsis documented as "Path to an additional settings JSON file to load, or an inline JSON string" (types.py:2105-2116). It goes on to describe the degradation for a missing file: "a missing file is logged and only the sandbox settings are passed". It says nothing about an inline string that fails to parse — and that is the case that loses your settings silently.1. A malformed inline string is discarded, and the warning names a file that does not exist.
Calling the real
_build_settings_value()at37422c2:where
MALFORMEDis'{"model":"sonnet",}'.With
sandboxalso set, one trailing comma costs you the entire settings object — model, permissions, env, hooks — and the CLI starts with only the sandbox block. The run proceeds and looks fine. The only trace is this, atsubprocess_cli.py:507-509:It reports a file path, and the "file path" is the JSON text itself. It never says the settings were dropped, and it is the only warning on a path that is about to lose data.
Note the contrast two lines down in the same output: with no
sandbox, the malformed text is passed through verbatim and the CLI rejects it. So the more dangerous configuration is the quieter one.2. The same bytes as a file raise a bare
json.JSONDecodeError, outside the wrapper._build_command()is called atsubprocess_cli.py:812— one line above thetry:that begins at:813. So aJSONDecodeErrorfrom reading the settings file is not converted toCLIConnectionErrorandself._exit_erroris never set. Callers get ajsonexception from what is otherwise aClaudeSDKErrorhierarchy, naming a column offset and no file.What I would change. Raise instead of degrading, for the inline-string case specifically, since it is not a documented degradation and it loses user configuration:
_build_command()inside thetry, or wrap the file read, so the failure comes out asCLIConnectionError.The one judgement call I would want your view on: whether "malformed inline string" should raise or should be added to the documented degradation list alongside the missing file. Raising is safer, but it is a behaviour change for anyone whose settings happen to contain a trailing comma today and who has not noticed that they are running with no settings at all.
I have not implemented any of this. Say which you want and I will open a PR.