fix(claudecode): refuse managed-only sandbox paths and aggregate the trust warnings - #2754
Merged
Merged
Conversation
…trust warnings Works through the five follow-ups deferred from #2720. 1. `sandbox.filesystem.allowManagedReadPathsOnly` and `sandbox.network.allowManagedDomainsOnly` are documented `Managed`, so neither file rulesync writes reads them. They are now dropped in both scopes with a warning, the same treatment the `Managed` top-level keys already get, instead of being committed as a lock that never applies. 2. The per-key trust warnings from the `permissions` fields, the `sandbox` paths and the top-level passthrough are collected and emitted as one summary line per file. The reasons are what differ between them; the "review this as you would a hook" advice only needs saying once. 3. `allowUnsandboxedCommands` and `autoAllowBashIfSandboxed` keep warning on their documented `true` default, now with the reason recorded: a project settings file outranks the user file, so an explicit `true` there re-opens what a user`s `false` closed. 4. Adds the six candidate keys, each checked against the settings reference: `claudeMdExcludes`, `modelOverrides`, `skipWebFetchPreflight`, `remoteControlAtStartup`, `prUrlTemplate` and `companyAnnouncements` are all scoped `Any file`, so all are written and warned about at the value that widens. `remoteControlAtStartup` is the exception: Claude Code honors a `false` from a project file but ignores a `true`, so a `true` is skipped at project scope and emitted only under --global. 5. Guards the `CLAUDECODE_TRUST_KEY_WIDENING_VALUES` lookup with `Object.hasOwn`, so its safety no longer depends on evaluation order.
…d warn on crossSessionInbound Review follow-ups on the trust-warning work. - stripCommandExecutingSandboxPaths, stripManagedOnlySandboxPaths and stripGlobalOnlySandboxPaths were byte-identical apart from the table they scanned and the message they logged. They are now one stripSandboxPaths that takes a list of SandboxPathRefusal records, so a fourth scope class adds a table rather than a fourth copy of the walk, and the override is cloned once instead of three times. - The widening predicates are named helpers (isNotFalse, isNotTrue, isNonEmptyList, isNonEmptyMap) shared across the table. Each states the restrictive value and reports everything else, so an off-type value in an authored file can no longer pass unreported: skipWebFetchPreflight and disableSkillShellExecution previously matched their widening value exactly. - remoteControlAtStartup's predicate is shared between the widening table and the project-scope drop table rather than written out twice. - crossSessionInbound is added to CLAUDECODE_TRUST_AFFECTING_KEYS. It is on the same documented stricter-value-wins list as remoteControlAtStartup, but its project value is honored only when it is stricter than the one above it, which no per-value predicate can decide; under --global a fetched "accept" is honored outright, so it is warned about instead of dropped. - Documented why import keeps the managed-only sandbox paths while it drops the command-executing ones. - The widening it.each now carries a 0-or-1 element list of quiet values, so "this key has no quiet value" cannot be confused with an undefined one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…table and pin the off-type cases Second review round, all low findings. - The named predicates now sit above both tables and the sandbox table uses them, so the six inline non-empty-list checks and the boolean checks beside them are one shared symbol each. The sandbox booleans move from an exact match on the widening value to "not the restrictive value", the same fail-safe direction the top-level table took. - Added the cases that pin that direction: an off-type skipWebFetchPreflight, disableSkillShellExecution and sandbox path are reported rather than passed over. An exact match on the widening value lets all of them through silently. - docs/reference/file-formats.md now states the quiet value rather than the warning one, matching what the predicates actually test. - The import-side comment said the sandbox subtree is stripped symmetrically with generate, which stopped being the whole story once the managed-only paths were refused on write but kept on import. Documented, and pinned with a round-trip test. - modelOverrides maps model IDs to provider-specific model IDs, so its reason now says which inference profile a call is routed to rather than which endpoint a prompt reaches. - Softened the claim that the shared remoteControlAtStartup predicate must never drift: the two tables ask different questions that agree today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dyoshikawa
force-pushed
the
resolve-issue-2721-claudecode-trust-warnings
branch
from
August 25, 2026 03:10
56e8b5f to
f0b44f4
Compare
…match The off-type cases added in the previous commit all landed on entries whose predicate had not changed, so reverting the six sandbox substitutions left the suite green. Two sandbox cases now cover entries that did move. ignoreViolations is composed from the shared predicates instead of spelling its own body: an empty map suppresses nothing, and isNonEmptyMap(false) is true, so the false that spells the key off has to be excluded as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reorder the it.each columns to [label, key, value] so the title placeholder picks up the unique label instead of the shared key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works through the five follow-ups deferred from the review of #2720.
1. Managed-scope sandbox restriction paths
sandbox.filesystem.allowManagedReadPathsOnlyandsandbox.network.allowManagedDomainsOnlyare both documented with aManagedscope in the settings reference, and the sandboxing guide says the same in prose ("SetallowManagedReadPathsOnlytotruein managed settings ..."). Neither file rulesync writes is the managed settings file, so both are now dropped in both scopes with a warning naming that file — thesandboxcounterpart of the treatmentCLAUDECODE_UNHONORED_KEY_SOURCESalready gives theManagedtop-level keys.They are dropped rather than warned about because they only ever narrow: a
sandboxblock that reads as though it locked the policy to managed values, while Claude Code ignores it, is the worse of the two failure modes. Import keeps them, unlike the command-executing paths: a value already hand-written in asettings.jsonwas written to be honored somewhere, so round-tripping it preserves that intent for the day it moves into a managed file, at the cost of a warning on every generate until it is removed. That reasoning is now in the table's JSDoc.2. Warning volume and aggregation
The trust warnings from the
permissionsfields, thesandboxpaths and the top-level passthrough are now collected into one summary line per file:The per-key reasons are what differ; the "review this as you would a hook" framing only needed saying once, and repeating it per key buried them. The refusal warnings (a key or path that is not written) stay one per key — each carries its own remediation instruction.
3.
value !== falseon documented defaultsKept, with the reason now recorded next to both predicates.
allowUnsandboxedCommandsandautoAllowBashIfSandboxeddo default totrue, but a project.claude/settings.jsonoutranks the user file, so an explicittruefrom a fetched override re-opens the escape hatch a user'sfalseclosed — and it does so without changing anything a diff of the effective policy would show.4. Future warn candidates
All six were checked against the settings reference. Every one is scoped
Any file, so none is skipped at project scope; all are added toCLAUDECODE_TRUST_AFFECTING_KEYS, four of them with a widening-value predicate so the restrictive value stays quiet:claudeMdExcludesmodelOverridesskipWebFetchPreflighttrue(turns off the WebFetch domain safety check)remoteControlAtStartuptrue, in the global scope onlyprUrlTemplatecompanyAnnouncementsremoteControlAtStartupturned out to be the one key whose scope depends on its value: Claude Code honors afalsefrom project or local settings but ignores atrue, so a checked-in file cannot turn Remote Control on for everyone who opens the repository. A project-scopetrueis therefore skipped with its own warning and emitted only under--global; afalseis written and says nothing.5. Defensive
Object.hasOwnCLAUDECODE_TRUST_KEY_WIDENING_VALUES[canonicalKey]is now guarded, so the lookup is safe on its own terms rather than because theCLAUDECODE_TRUST_AFFECTING_KEYScheck happens to short-circuit first. Note that no test can distinguish the guarded lookup from the unguarded one today — the two tables' key sets are disjoint, so the earlier check short-circuits before the lookup is reached. The accompanying test pins that short-circuit order, not the guard itself.Review follow-ups
Applied after the first review round:
strip*SandboxPathsfunctions were byte-identical apart from their table and message. They are now onestripSandboxPathsover a list ofSandboxPathRefusalrecords, which also drops the override from three nestedstructuredClones to one.isNotFalse,isNotTrue,isNonEmptyList,isNonEmptyMap) shared across the table, each stating the restrictive value and reporting everything else.skipWebFetchPreflightanddisableSkillShellExecutionpreviously matched their widening value exactly, so an off-type value such as1was written with no warning at all.remoteControlAtStartup's predicate is shared between the widening table and the project-scope drop table instead of written out twice.crossSessionInboundis added toCLAUDECODE_TRUST_AFFECTING_KEYS. It is on the same stricter-value-wins list, but its project value is honored only when it is stricter than the one above it, which no per-value predicate can decide without reading the user's settings. Under--globala fetched"accept"is honored outright, so it is warned about rather than dropped.it.eachnow carries a 0-or-1 element list of quiet values, so "no quiet value" (prUrlTemplate) cannot be confused with a quiet value that happens to beundefined.Verification
pnpm cicheckpasses.claudecode-permissions.test.tsgrows from 107 to 123 tests, covering the aggregated summary (count, singular/plural, silence when nothing widens), the managed-only drops in both scopes, each new key at both its widening and its quiet value, all threeremoteControlAtStartupcases, and a settings key named after anObject.prototypemember.Closes #2721