feat(claude-agent-sdk): opt-in setting_sources for target-repo skills - #502
Conversation
@microsoft-github-policy-service agree company="Too Good To Go" |
Jason Robert (jrob5756)
left a comment
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Five blocking findings here, all tied to the same root cause: setting_sources was added to the schema and to the claude-agent-sdk provider without the enforcement, serialization, and per-agent scoping that every other structured provider field already has. None of them are style nits — b1/b2 mean the field can be silently accepted and then silently dropped, and b3/b4 mean the skill filtering it interacts with doesn't do what its own comments claim. Until those are fixed this shouldn't merge as-is.
Blocking findings, by location:
src/conductor/config/schema.py:2693—setting_sourcesis documented claude-agent-sdk-only but accepted (and silently ignored) on every other provider.src/conductor/config/schema.py:2692—has_structured_config()doesn't know about the field, so it's erased by serialization and dropped by--provideroverrides with no warning.src/conductor/providers/claude_agent_sdk.py:1016— a per-agentskills: []opt-out gets silently upgraded toskills="all"once a settings tier is enabled.src/conductor/providers/claude_agent_sdk.py:1012— the comment justifying the"all"branch describes SDK behavior that isn't what the SDK actually does, and it contradicts an unchanged comment nine lines above it.
One more blocking finding has no single line to anchor to, so it's here in full:
BLOCKING — the invariant this PR overturns is still documented as unconditional in AGENTS.md and two user-facing docs, and there's no CHANGELOG entry.
The diff touches four files and no documentation. These statements are now false:
| Location | Now-false text |
|---|---|
AGENTS.md:434 |
"setting_sources=[] unconditionally, for the same reason strict_mcp_config=True is unconditional" |
AGENTS.md:431 |
"the unconditional setting_sources=[] ... stops the CLI loading CLAUDE.md, project settings, and hooks from it" |
AGENTS.md:217 |
"keeps enable_config_discovery off on Copilot ... and setting_sources=[] on claude-agent-sdk" |
AGENTS.md:435 |
"the SDK auto-allows it via Skill(<name>) in allowed_tools" — on the new "all" path there's no <name>; the SDK appends the bare Skill, a broader auto-approve |
docs/providers/comparison.md:164 |
"Conductor also pins the SDK's setting_sources to an empty list on every run", followed by a flat list of things not inherited (skills, CLAUDE.md, settings.json, hooks) |
docs/providers/experimental.md:104 |
"setting_sources is pinned empty as of #352 so ambient instructions, settings, hooks, and skills are not inherited" |
src/conductor/skills/discovery.py:20 |
still asserts setting_sources=[] as an unconditional invariant |
AGENTS.md is loaded as agent instructions in this repo, so a stale invariant there won't just sit unread — it'll get asserted as fact to whoever touches this file next, human or agent. comparison.md and experimental.md are worse for users specifically, since both currently promise an unconditional safety guarantee about hooks not being inherited, and this PR makes that guarantee conditional without saying so. The field's own docstring already warns to enable it only for repos trusted as much as the workflow itself — the docs should carry that same caveat, not the stronger claim.
Also missing: the ## [Unreleased] section of CHANGELOG.md is empty, docs/configuration.md's field-compatibility table has no row for this field, docs/workflow-syntax.md never mentions the new key, and no example workflow exercises it (so make validate-examples never touches this path).
Suggested fix: rewrite the AGENTS.md, comparison.md, experimental.md, and skills/discovery.py statements as "empty by default, opt-in per workflow via runtime.provider.setting_sources" and carry the trust caveat forward with them. Add the CHANGELOG entry, the workflow-syntax doc, the compatibility-table row, and an example.
Findings that could not be anchored inline
These name a line outside this pull request's diff, so GitHub cannot attach them to a specific line.
tests/test_providers/test_claude_agent_sdk.py:1
RECOMMENDED
The ten new tests cover provider wiring on the happy path and the _resolve_skill_filter table well. What's missing maps one-to-one onto the blocking findings above — each would have been caught by a three-line test:
- No schema test.
grep -rn setting_sources tests/matches one file. Nothing asserts the field is rejected on a non-claude-agent-sdkprovider, nothing rejects an invalid tier string, nothing round-tripsmodel_dump/model_validate. - No argv assertions, even though
TestSkillsWiring's own docstring sets the standard for this subsystem: "The provider's contract is ultimately theclaudeCLI command line, so these assert the argv the SDK builds from our options rather than stopping at the options object." That matters here specifically because the"all"branch produces a structurally different--allowedToolsvalue (bareSkill) from the declared branch (Skill(name)), and on thetools: []pathpermission_mode is None, so--allowedToolsis the only thing granting the tool. If the bare-Skillinjection regressed,test_declared_sources_grant_the_skill_toolwould still pass while every skill call got refused — the exact failure this PR says it exists to prevent. - The combined path never runs through
execute.setting_sourcesplus workflow-declared skills is only covered at the pure-function level; a refactor that letsetting_sourcesclobber the declared allowlist would leave the unit test green. - Only the
projecttier is exercised.user,local, and multi-element lists (which comma-join into argv) are untested. tools:omitted +setting_sources(theclaude_codepreset path) is untested.
Three of the new tests are also strictly weaker duplicates of pre-existing argv-level ones: test_default_sends_an_explicit_empty_list vs test_setting_sources_isolated_unconditionally, test_no_sources_no_skills_withholds_the_skill_tool vs test_explicit_no_tools_without_skills_stays_empty, and test_default_still_enables_no_skills vs test_no_skills_suppresses_ambient_discovery.
Separately, test_setting_sources_isolated_unconditionally (line 2884, not modified by this PR) is now misnamed — its docstring reads "No ambient skills, CLAUDE.md, settings.json, or hooks — ever." It still passes, but the name and "ever" assert a guarantee the code no longer makes, and a future reader will trust it when reasoning about the security boundary.
Suggested fix: add a parametrized schema-rejection test for non-claude-agent-sdk providers, a model_dump round-trip test, an argv assertion that the "all" path emits bare Skill in --allowedTools, a skills: [] + setting_sources test, and parametrize the tier over [["project"], ["user"], ["local"], ["user", "project", "local"]]. Reuse TestSkillsWiring._capture_options/_argv (add a setting_sources= parameter) instead of hand-rolling fake_query per test, and rename test_setting_sources_isolated_unconditionally to ..._by_default.
| """Extra HTTP headers to send with every request. Copilot-only.""" | ||
|
|
||
| setting_sources: list[Literal["user", "project", "local"]] | None = None | ||
| """Claude Code settings tiers the session may load. claude-agent-sdk-only. |
There was a problem hiding this comment.
BLOCKING
The docstring says "claude-agent-sdk-only," but _check_field_compatibility (schema.py:2850-2919) never enforces it. Every other provider-scoped field on this model does — the seven copilot_only_fields, auth_token via claude_only_fields, the seven aca_only_fields, four individual hermes_* guards. setting_sources is in none of them.
Confirmed empirically, all of these construct without error:
ProviderSettings(name='copilot', setting_sources=['project']) -> ACCEPTED
ProviderSettings(name='openai', setting_sources=['project']) -> ACCEPTED
ProviderSettings(name='claude', setting_sources=['user']) -> ACCEPTED
ProviderSettings(name='hermes', setting_sources=['local']) -> ACCEPTED
ProviderSettings(name='aca', ..., setting_sources=['project']) -> ACCEPTED
Only the case "claude-agent-sdk" arm at factory.py:264 reads the field, so on every other provider this is a pure no-op. aca is the sharpest case: set inner_provider: claude-agent-sdk alongside setting_sources: [project], the schema accepts it, and the runner's four-key inner_provider_settings allowlist means it's never forwarded to the sandbox.
This one matters more than a typical no-op because the field is security-relevant — the user is explicitly asking to load ambient hooks — and there's no feedback that the request was dropped. It also breaks the promise in docs/configuration.md:236: "The schema rejects the following misconfigurations at config load time so they cannot silently produce a no-op SDK call."
conductor validate doesn't cover this either: validate_workflow_config is only imported by cli/validate.py, and conductor run never calls it. The model_validator is the only gate on the run path.
| """Claude Code settings tiers the session may load. claude-agent-sdk-only. | |
| @model_validator(mode="after") | |
| def _check_field_compatibility(self) -> "ProviderSettings": | |
| if self.setting_sources is not None and self.name != "claude-agent-sdk": | |
| raise ValueError( | |
| "'setting_sources' is only supported when name='claude-agent-sdk' " | |
| f"(got name={self.name!r}). It selects Claude Code settings tiers, " | |
| "which no other provider reads." | |
| ) |
Match this to the existing hermes_home shape at schema.py:2909, and add a parametrized rejection test alongside the existing per-provider scoping tests.
There was a problem hiding this comment.
Fixed in 858f795 — took your patch as written, matching the hermes_home shape.
You're right that aca + inner_provider: claude-agent-sdk is the sharpest case, and worth naming that this rejects that combination rather than forwarding it. I looked for a precedent for a claude-agent-sdk-scoped field that survives into the sandbox and found none — the four-key inner_provider_settings allowlist has no room for it — so a hard reject is the honest answer today: the field cannot work there, and accepting it would be exactly the silent no-op this finding is about. If forwarding it is wanted later, that's an allowlist change plus a runner change, not a schema relaxation.
Test: TestSettingSourcesScoping::test_rejected_on_other_providers, parametrized over copilot / openai / claude / hermes / aca, plus test_unknown_tier_rejected for the typo case you flagged separately.
| headers: dict[str, str] | None = None | ||
| """Extra HTTP headers to send with every request. Copilot-only.""" | ||
|
|
||
| setting_sources: list[Literal["user", "project", "local"]] | None = None |
There was a problem hiding this comment.
BLOCKING
has_structured_config() (schema.py:3085) is has_custom_routing() or has_external_runtime() or has_aca_config(). None of the three looks at setting_sources, so a settings object that sets only this field returns False. The @model_serializer at schema.py:3089 then collapses it to a bare string:
p = ProviderSettings(name='claude-agent-sdk', setting_sources=['project'])
p.has_structured_config() # False
p.model_dump() # 'claude-agent-sdk' <- the opt-in is gone
ProviderSettings.model_validate(p.model_dump()).setting_sources # None
AGENTS.md states the contract this breaks directly: "has_structured_config() keeps either mode from collapsing to bare-string serialization." This PR adds a third mode and doesn't extend the guard.
Two consequences are already live in the tree:
cli/run.py:241—_apply_provider_overrideonly warns "Provider override discards structured runtime.provider settings" whenhad_structuredis true.--provider claude-agent-sdk(orconductor resume --provider ..., which AGENTS.md documents as routine) against a workflow withsetting_sources: [project]discards the setting at line 249 with no warning — every other structured field gets one.cli/run.py:206—_describe_providershort-circuits on the same predicate and returns the bare name, so even-vnever shows that ambient settings tiers are active. For a toggle that enables arbitrary hook execution, that's the wrong thing to stay quiet about.
Resume happens to be safe today only because engine/checkpoint.py doesn't persist runtime.provider and resume re-reads the YAML — that's luck, not design.
| setting_sources: list[Literal["user", "project", "local"]] | None = None | |
| def has_structured_config(self) -> bool: | |
| """Return True when the provider has any non-default structured settings.""" | |
| return ( | |
| self.has_custom_routing() | |
| or self.has_external_runtime() | |
| or self.has_aca_config() | |
| or self.setting_sources is not None | |
| ) |
Also add a setting_sources=[...] part to cli/run.py::_describe_provider, plus a round-trip regression test: ProviderSettings.model_validate(p.model_dump()) == p.
There was a problem hiding this comment.
Fixed in 858f795 — took your patch, and both consequences you traced are now covered by tests rather than by luck:
cli/run.py:241—TestSettingSourcesScoping::test_override_warns_before_discarding_itasserts_apply_provider_overrideemits the discard warning for a workflow withsetting_sources: [project].cli/run.py:206—_describe_providernow appendssetting_sources=[...], asserted bytest_described_for_verbose_output(which also pins the bare-name case, so the serializer collapse for an unset field stays covered).
Plus the round-trip you asked for: test_round_trips_through_model_dump (model_validate(p.model_dump()) == p) and test_counts_as_structured_config.
Noting the resume point rather than acting on it: engine/checkpoint.py still doesn't persist runtime.provider, so resume re-reads the YAML. Agreed that's luck and not design, but making it design is a checkpoint-schema change outside this PR — happy to open a follow-up issue if you want it tracked.
| # back "not in this session's skills allowlist". `"all"` widens the | ||
| # filter to exactly what the enabled tiers discovered, which is the | ||
| # set the workflow asked for by enabling them. | ||
| skills=_resolve_skill_filter(skill_names, self._setting_sources), |
There was a problem hiding this comment.
BLOCKING
skill_names comes from _resolve_skill_plugins(skill_directories) at line 904. An agent that declares skills: [] — the documented explicit opt-out — resolves to no skill directories, so skill_names == []. With workflow-level setting_sources: [project], _resolve_skill_filter([], ['project']) returns "all", so the agent that explicitly asked for no skills gets every skill the target repo's .claude/skills ships.
Line 945 makes it worse: skills_enabled=bool(skill_names) or bool(self._setting_sources) grants the Skill tool back to an agent that declared both tools: [] and skills: [].
This contradicts two documented invariants — AGENTS.md's "skills: [] remains the one opt-out," and docs/providers/comparison.md:164, which describes pinning setting_sources empty as exactly what stopped skills: [] from being a no-op on this provider. This PR reintroduces that no-op at the per-agent level.
The asymmetry is structural: setting_sources lives on the workflow-global runtime.provider, while skills: is per-agent, so there's currently no way to say "this workflow loads the target repo's skills, but this one agent gets none."
| skills=_resolve_skill_filter(skill_names, self._setting_sources), | |
| effective_sources = [] if agent.skills == [] else self._setting_sources | |
| sdk_tools, permission_mode = self._resolve_tool_config( | |
| tools, | |
| agent, | |
| skills_enabled=bool(skill_names) or bool(effective_sources), | |
| agents_enabled=bool(custom_agents), | |
| ) |
_resolve_tool_config already reads the raw tri-state (agent.tools is None) for exactly this reason — do the same with agent.skills, and pass effective_sources into _resolve_skill_filter(skill_names, effective_sources) as well. Add a test for skills: [] + setting_sources: [project].
There was a problem hiding this comment.
Fixed in 858f795 — took your patch and threaded effective_sources into _resolve_skill_filter as well, so skills: [] opts the agent out of the tier for all three consequences, not just the tool grant:
effective_sources: list[SettingSource] = [] if agent.skills == [] else self._setting_sources…then skills_enabled=bool(skill_names) or bool(effective_sources), setting_sources=effective_sources, and _resolve_skill_filter(skill_names, effective_sources). Passing it to setting_sources= too matters for the reason you gave for the field existing at all: an agent that declared skills: [] shouldn't load the tier's hooks either, and gating only the skill filter would have left them.
Test: TestSettingSourcesWiring::test_agent_skills_opt_out_beats_a_workflow_tier — asserts setting_sources == [], skills == [], tools == [], and no --allowedTools in argv (with --tools "" as a positive anchor so the negative can't pass on a broken argv builder).
On the structural asymmetry: agreed, and I've left it as an asymmetry rather than adding an AgentDef-level field. skills: [] now gives you "this workflow loads the target repo's skills, but this one agent gets none", which is the case that had a real victim; the inverse ("only this one agent loads them") still isn't expressible. I documented the actual scope on the field instead (schema.py) and in the run-time warning. If you'd rather have the per-agent field or a refusal when a tier-using agent has no explicit working_dir — see my reply on the :997 thread — say so and I'll add it here rather than in a follow-up.
| # the tool and still have every call rejected. Skills discovered | ||
| # from a settings tier never pass through `skill_names`, so sending | ||
| # `[]` there permits nothing — the model lists the repo's skills | ||
| # (the listing leaks past this filter) and every invocation comes |
There was a problem hiding this comment.
BLOCKING
The comment claims that with skills=[] "the model lists the repo's skills (the listing leaks past this filter) and every invocation comes back 'not in this session's skills allowlist.'" That's not what the SDK does.
The installed SDK's own docstring for ClaudeAgentOptions.skills says the opposite: it's "a context filter, not a sandbox — unlisted skills are hidden from the model's listing and rejected by the Skill tool, but their files remain on disk." The bundled CLI backs this up: the function building the skill_listing attachment filters by the session skill allowlist and returns [] when nothing survives, and returns [] outright when the session holds no Skill tool. The "not in this session's skills allowlist" string is real, but it's the invocation backstop, reachable only if the model names a skill it was never shown. The premise that the model sees the skill anyway is wrong.
This isn't just a wording problem:
- It contradicts the unchanged comment at line 1003 (correct, pre-existing): "unlisted skills are hidden from the model's listing and rejected by the Skill tool." A reader can't tell which comment to trust, and the wrong one is newer.
- The real behavior argues for a narrower fix. If
skills=[]hides discovered skills rather than causing a rejection loop, the actual problem is "the tier loads them and then hides all of them" — and"all"is a blunt answer to that. Opting into["user"]now enables every skill in~/.claude/skills, which deserves to be a deliberate, stated tradeoff rather than a side effect of a mis-described failure mode.
The same false premise is repeated at lines 297-303 (_resolve_skill_filter docstring), 940-944 (skills_enabled comment), 1543-1545 (_resolve_tool_config docstring), and in the test docstring at tests/test_providers/test_claude_agent_sdk.py:2357-2360 — five copies to fix.
| # (the listing leaks past this filter) and every invocation comes | |
| # Skills discovered from a settings tier never pass through `skill_names`. | |
| # Sending `[]` sets the CLI's session skill allowlist to empty, which | |
| # suppresses them from the model's listing entirely -- so enabling a tier | |
| # would load the repo's skills and then hide every one of them. `"all"` | |
| # omits the filter (the SDK treats `"all"` and omitted as equivalent at the | |
| # wire level), leaving exactly what the enabled tiers discovered. |
And confirm that widening to "all" — rather than, say, refusing the combination — is actually the intended tradeoff for the ["user"] tier.
There was a problem hiding this comment.
You're right, and thanks for chasing it into the bundled CLI — the premise was wrong. skills=[] empties the session's skill allowlist, which hides discovered skills from the listing; the "not in this session's skills allowlist" string is the invocation backstop, reachable only when the model names a skill it was never shown. Fixed in 858f795 at all five sites you listed (_resolve_skill_filter docstring, the skills_enabled comment, the option-block comment, _resolve_tool_config's docstring, and the test docstring), plus the AGENTS.md line, so nothing contradicts the correct pre-existing comment at 1003 any more.
To confirm the tradeoff you asked about: yes, "all" is intended, and the corrected description is what argues for it. The failure mode isn't a rejection loop, it's "the tier loads the repo's skills and then hides every one of them" — discovery that can never be reached. Refusing the combination instead would mean refusing the feature: a target repo's .claude/skills is the whole motivating case, and Conductor never learns those names (they don't pass through skill_names), so there's nothing narrower than "all" to scope the filter to.
Your point about ["user"] is fair and I've stopped treating it as a side effect: user now carries an explicit "makes the run depend on the operator's machine, prefer [project]" caveat on the schema field, in comparison.md and in docs/workflow-syntax.md, and the run-time warning names the tiers it enabled. What I haven't done is forbid user — it's opt-in, [] by default, and refusing a tier the CLI supports felt like the wrong place to draw that line. If you'd rather user be rejected outright (or gated behind something louder), that's a one-line schema change and I'll take it.
| max_turns=max_agent_iterations, | ||
| max_session_seconds=max_session_seconds, | ||
| mcp_servers=mcp_servers, | ||
| setting_sources=getattr(provider_settings, "setting_sources", None), |
There was a problem hiding this comment.
RECOMMENDED
provider_settings is typed ProviderSettings | None and can't be a bare string here (a legacy string is normalized to provider_type with provider_settings=None earlier in create_provider). ProviderSettings is frozen with extra="forbid", so the attribute is always present on a real instance — the getattr default only ever fires for None.
Two problems. First, this is the only field read here without the provider-name guard every sibling branch uses (provider_settings is not None and provider_settings.name == "openai" at line 154, "claude" at 181, "hermes" at 213). Correctness rests entirely on registry.py:117 having nulled a mismatched object — invisible at this call site, and reachable precisely because the schema doesn't validate it (see the schema.py:2693 finding).
Second, getattr returns Any, which hides a genuine type error. Writing the guarded form directly fails ty today:
error[invalid-argument-type]: Expected `list[str] | None`,
found `list[Literal["user", "project", "local"]] | None`
because list is invariant and the provider's parameter is widened to list[str]. This line is quietly suppressing the narrowing loss described in the type-widening finding below, and as a side effect a future rename of the schema field would silently disable the feature forever instead of raising.
| setting_sources=getattr(provider_settings, "setting_sources", None), | |
| setting_sources=( | |
| provider_settings.setting_sources | |
| if provider_settings is not None | |
| and provider_settings.name == "claude-agent-sdk" | |
| else None | |
| ), |
Fix the parameter type first (see the sibling finding on _resolve_skill_filter's narrowing), then use this guarded form.
There was a problem hiding this comment.
Fixed in 858f795, in the order you prescribed — types first, then the guarded read, so the getattr isn't hiding the narrowing loss any more:
setting_sources=(
provider_settings.setting_sources
if provider_settings is not None and provider_settings.name == "claude-agent-sdk"
else None
),This now matches the openai / claude / hermes siblings, and correctness no longer rests on registry.py:117 having nulled a mismatched object — the schema rejects the mismatch outright as well (your :2693 finding), so there are two independent gates instead of one invisible one.
| ) | ||
|
|
||
|
|
||
| def _resolve_skill_filter(skill_names: list[str], setting_sources: list[str]) -> list[str] | str: |
There was a problem hiding this comment.
RECOMMENDED
Two type issues the project's typechecker can't currently catch here.
1. -> list[str] | str. The installed SDK declares skills: list[str] | Literal["all"] | None. The str arm is pure width with no value behind it — the docstring says "the literal "all"," the tests assert == "all", and no other string is reachable. A future return "All" or return "project" would typecheck and then fail inside the CLI.
2. Narrowing lost at line 693. The schema's list[Literal["user", "project", "local"]] degrades to list[str] in the constructor and stays list[str] on self._setting_sources, with no boundary validation. The SDK forwards it unvalidated into argv (--setting-sources={','.join(...)}), so ClaudeAgentSdkProvider(setting_sources=["prject"]) puts a typo straight on the CLI command line. The PR's own tests construct the provider directly nine times, so this is reachable, not hypothetical.
One more thing worth flagging: make typecheck passing here says nothing, because line 49 binds ClaudeAgentOptions: Any = None in the ImportError fallback, which poisons the symbol and disables argument checking at the ClaudeAgentOptions(...) call site entirely. Against the real symbol, ty flags both skills= (line 1016) and setting_sources= (line 997) as invalid-argument-type.
| def _resolve_skill_filter(skill_names: list[str], setting_sources: list[str]) -> list[str] | str: | |
| def _resolve_skill_filter( | |
| skill_names: list[str], setting_sources: Sequence[str] | |
| ) -> list[str] | Literal["all"]: |
Carry the tier narrowing through the constructor too — declare a module-level SettingSource = Literal["user", "project", "local"] (or import the SDK's under TYPE_CHECKING) and type both the parameter at line 693 and self._setting_sources as list[SettingSource]. That also makes the getattr in factory.py unnecessary.
There was a problem hiding this comment.
Fixed in 858f795 — both, and your point about make typecheck proving nothing here was the useful part.
_resolve_skill_filter(skill_names: list[str], setting_sources: Sequence[SettingSource]) -> list[str] | Literal["all"].- Added a module-level
SettingSource = Literal["user", "project", "local"]and carried it through the constructor parameter andself._setting_sources, so the schema's narrowing survives to the option. Kept it as a local alias rather than importing the SDK's underTYPE_CHECKINGprecisely because of theImportErrorfallback you flagged — an imported symbol would degrade toAnyin that path.
The typo case is now closed at both ends: the schema rejects an unknown tier (test_unknown_tier_rejected, parametrized over prject / workspace / PROJECT / ""), and the provider's own parameter is typed, so ClaudeAgentSdkProvider(setting_sources=["prject"]) is a type error rather than a string on the CLI command line.
On your ty observation: with the claude-agent-sdk extra actually installed (so ClaudeAgentOptions resolves to the real symbol rather than the Any fallback), ty check src/conductor/providers/claude_agent_sdk.py now reports zero invalid-argument-type — both skills= and setting_sources= line up with the SDK's declarations:
setting_sources list[Literal['user', 'project', 'local']] | None
skills list[str] | Literal['all'] | None
The only diagnostics left in that configuration are unused-ignore-comment warnings on the pre-existing # ty: ignore[unresolved-import] pragmas, which exist for the uninstalled case — I've left those alone since removing them would break the default typecheck run.
| # re-defaults an unset ``setting_sources`` to ``["user", "project"]`` | ||
| # whenever ``skills`` is set, so the empty list must be sent explicitly. | ||
| # See the option block in ``execute``. | ||
| self._setting_sources: list[str] = list(setting_sources or []) |
There was a problem hiding this comment.
RECOMMENDED
Enabling project makes the CLI read <cwd>/.claude/settings.json, whose hooks entries run arbitrary shell commands on tool events, from a directory that's by design a target repo. Grepping every setting_sources reference in src/ turns up only docstrings, this assignment, and the option at line 997 — no logger call, no verbose_log, no event. Nothing in the run output distinguishes a run that loaded the target repo's hooks from one that didn't, and per the has_structured_config() finding, -v doesn't show it either.
This is out of step with how the repo handles comparable trust decisions elsewhere. config/validator.py's _report_dropped_components warns about plugin hooks/ precisely because a component that behaves differently inside a workflow than in the CLI is, in its own words, "exactly the silent divergence this feature exists to remove, so the difference is named before the run rather than discovered after it." That reasoning applies with more force here, since the plugin case drops the hooks and this one enables them. This file already has six logger.warning/logger.info call sites, including _warn_if_session_lookup_unavailable.
| self._setting_sources: list[str] = list(setting_sources or []) | |
| if self._setting_sources: | |
| logger.warning( | |
| "claude-agent-sdk: ambient settings tiers enabled (%s). The session will " | |
| "load settings, instructions and HOOKS from those tiers -- 'project' reads " | |
| "<working_dir>/.claude/settings.json, whose hooks run shell commands on " | |
| "tool events. Enable only for repositories trusted as much as the workflow.", | |
| ", ".join(self._setting_sources), | |
| ) |
There was a problem hiding this comment.
Fixed in 858f795 — took your patch, with the scope caveat folded into the message since it's the same blast radius your :997 finding is about:
claude-agent-sdk: ambient settings tiers enabled (project). Sessions on this provider
load settings, instructions and HOOKS from those tiers -- 'project' reads
<working_dir>/.claude/settings.json, whose hooks run shell commands on tool events.
This applies to every agent on the provider, each resolving the tier against its own
working_dir (agents without one resolve against the directory `conductor run` was
launched in). Enable only for repositories trusted as much as the workflow.
One placement note: it's in the constructor where you suggested, so it fires once per provider instance (i.e. once per run for this provider type), not once per agent execution. That's deliberate — per-execute would repeat it on every step and every retry — but it means the message has to describe the per-agent resolution rather than naming a concrete directory, which is why it reads the way it does.
-v now shows it too via _describe_provider (your :2692 finding), so the toggle is visible in both the warning and the settings line. Test: test_enabling_a_tier_warns_about_hooks / test_default_warns_about_nothing.
| # | ||
| # A tier brings everything it defines, hooks included, so this is | ||
| # only for repositories trusted as much as the workflow itself. | ||
| setting_sources=self._setting_sources, |
There was a problem hiding this comment.
RECOMMENDED
self._setting_sources is set once per provider instance, and there's one instance per provider type. But the directory it resolves against is per agent — _resolve_session_cwd returns agent.working_dir or os.getcwd().
So setting_sources: [project], written for the one agent pointed at a target repo, also makes every other agent on that provider load .claude/settings.json — hooks included — from the directory conductor run was launched in. That's a wider blast radius than the field docstring (schema.py:2701-2706) describes, and it's the exact ambient-hook leakage AGENTS.md:434 cites as the reason the original invariant existed.
The field being workflow-global while working_dir is per-agent is the root cause, and it's the same mismatch behind the skills: [] finding above.
Suggested fix: either scope the setting per agent (an AgentDef-level field, so it can only apply where the workflow named a directory), or refuse the combination when an agent on this provider has no explicit working_dir, so a tier can never resolve against the launch directory by accident. At minimum, document the actual scope in the field docstring.
There was a problem hiding this comment.
Real, and it's the same root cause as the skills: [] finding. Here's what I did and didn't do, since you offered three remedies:
Done — the "at minimum" plus one of the two:
- The field docstring (schema.py) now states the actual scope: workflow-global field, per-agent
working_dir, agents without one resolving against the launch directory. Same wording incomparison.md,docs/workflow-syntax.mdand the new run-time warning, so it's stated in the three places someone would look. skills: []on an agent now opts that agent out of the tiers entirely, hooks included (see the:1016thread). That's the escape hatch for the blast radius: the summarizing/auditing agent that has no business reading the target repo can be made hermetic without giving up the feature. The new example workflow uses exactly that shape.
Not done — the refusal, and I'd like your call on it. Refusing the combination when a tier-using agent has no explicit working_dir is appealing (it's the accident, not the use case), but it's a provider-name-keyed cross-field check: setting_sources lives on runtime.provider while working_dir lives on each AgentDef, so it can't go in ProviderSettings._check_field_compatibility — it'd need config/validator.py and a run-time guard in the provider, since conductor run never calls the validator (the same doubling as _reject_unsupported_skills). That's a real chunk of work and it forbids a legitimate shape: conductor run launched from inside the target repo, with no working_dir anywhere, which is the simplest way to use this.
The per-agent AgentDef field is the other honest answer, and it's strictly more expressive than either. I didn't build it because it makes runtime.provider.setting_sources immediately redundant-ish and I'd rather not ship two spellings in one PR.
So: happy to add the refusal, or move the field to AgentDef entirely, or leave it documented as-is. Tell me which and I'll do it in this PR.
| assert captured["strict"] is True | ||
|
|
||
|
|
||
| class TestSettingSourcesWiring: |
There was a problem hiding this comment.
RECOMMENDED
class TestSettingSourcesWiring opens at line 2252, but TestMcpOptionsWiring was never closed first — everything from line 2406 to 2564 is now collected under the new class instead. Against origin/main, these six moved:
test_config_file_removed_when_query_raisestest_config_file_removed_on_interrupt_returntest_no_config_file_leaks_when_options_construction_failstest_secrets_reach_the_file_but_not_the_optionstest_concurrent_executions_get_independent_config_filestest_empty_tools_still_attaches_mcp_servers
TestMcpOptionsWiring went from 8 tests to 2. Nothing is orphaned or skipped — no class-scoped fixtures, all six still run, suite still reports 174 passing — which is exactly why CI won't catch this.
It still matters: test_secrets_reach_the_file_but_not_the_options and test_no_config_file_leaks_when_options_construction_fails are secret-leak and tempfile-leak regression guards. File them under a settings-sources heading and the next person doing MCP work greps TestMcpOptionsWiring, sees two tests, and assumes config-file cleanup is uncovered. The node IDs changed silently too, breaking any saved -k selection or test-ID reference.
Suggested fix: move the whole TestSettingSourcesWiring block (lines 2252-2404) down so it begins after line 2564, right before class TestMcpRequiredFields. Better still, place it after TestSkillsWiring — that's the class it logically extends, and it already provides _capture_options/_argv helpers these tests could reuse.
There was a problem hiding this comment.
Good catch, and thanks for naming why CI can't see it. Fixed in 858f795: the block moved to sit after TestSkillsWiring (your "better still" option), so TestMcpOptionsWiring is back to its 8 tests — including the two regression guards you singled out — and the node IDs for those six are restored to what they were on main.
Being next to TestSkillsWiring also let me take the reuse suggestion rather than hand-rolling fake_query per test: _capture_options gained a setting_sources= parameter, and the new class picks up _capture_options / _argv / _skill_dirs from it (bound as staticmethod(...) rather than by subclassing, which would have re-run all of TestSkillsWiring's tests under a second name).
Everything from your tests/…:1 review comment is in as well — argv assertions for both branches (bare Skill on the "all" path, Skill(conductor:conductor) on the declared one, both through execute), the skills: [] + tier case, the omitted-tools: preset path, tier parametrized over [["project"], ["user"], ["local"], ["user","project","local"]] asserting the comma-join in argv, the schema-rejection and model_dump round-trip tests, and test_setting_sources_isolated_unconditionally renamed to ..._by_default with the "ever" dropped from its docstring. The three weaker duplicates are gone.
The provider passed setting_sources=[] unconditionally, so no workflow could load the settings tier of the repository it operates on. An agent working a target repo could not use that repo's own .claude/skills/, CLAUDE.md, or .claude/rules/*.md; those conventions had to be duplicated into the workflow's prompts. Adds runtime.provider.setting_sources (user/project/local), defaulting to [] so behaviour is unchanged unless a workflow asks. The empty default is load-bearing rather than cosmetic: the SDK re-defaults an unset setting_sources to ["user", "project"] whenever skills is set, so [] has to be sent explicitly to keep a run hermetic. Two things this needed beyond the plumbing, both found by running it: - Grant the Skill tool when setting_sources is non-empty. CLI-discovered skills never pass through skill_names, so gating on that alone listed a repo's skills to the model with no tool to invoke them. - Resolve ClaudeAgentOptions.skills to "all" when tiers are enabled and the workflow named no skills. Otherwise every call failed with "not in this session's skills allowlist" — observed with 28 discovered skills, all rejected. A declared skills:/plugins: list still wins; discovery does not widen what the author asked for. Note that `project` loads the whole tier, hooks included: pointing it at untrusted code runs that code's hooks. Documented at the schema field. Verified against claude-agent-sdk 0.2.87 with a fixture repo whose .claude/rules is a symlink to a tool-agnostic .agents/rules and a CLAUDE.md that never references it. With [] the agent reports no rule; with ["project"] it returns the rule's contents. Skills, CLAUDE.md and rules all arrive through the one tier, symlinks included. Refs microsoft#501 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses review on microsoft#502. The field was added to the schema and the provider without the enforcement, serialization and per-agent scoping every other structured provider field already has. - Reject `setting_sources` unless `name: claude-agent-sdk`. Only that factory branch reads it, so on any other provider it was accepted and silently dropped -- worst on `aca`, where the runner's four-key `inner_provider_settings` allowlist never forwards it to the sandbox. - Count it in `has_structured_config()`, so the model serializer no longer collapses the object to a bare string (the opt-in survives a `model_dump` round-trip), `--provider` overrides warn before discarding it, and `_describe_provider` shows it under `-v`. - A per-agent `skills: []` now opts that agent out of the tiers entirely, hooks included, keeping it the one opt-out. The tier is workflow-global while `working_dir` is per agent, so without this an agent that asked for no skills got every skill in the target repo. - Warn once per provider when tiers are enabled, naming hooks and the actual scope. Nothing in the run output distinguished a run that loaded the target repo's hooks from one that did not. - Carry the `Literal["user", "project", "local"]` narrowing through the constructor and `_resolve_skill_filter`, and guard the factory read by provider name instead of `getattr`. Against the real SDK symbols `ty` now reports no `invalid-argument-type` at the options call site. - Correct the comments describing the `"all"` branch: `skills=[]` alongside a tier empties the session allowlist and *hides* the discovered skills from the model's listing rather than causing a rejection loop, so the failure being avoided is "load the repo's skills, then hide every one". `"all"` is kept deliberately. - Docs: AGENTS.md, comparison.md, experimental.md and skills/discovery.py stated the empty `setting_sources` as an unconditional guarantee; they now say "empty by default, opt-in per workflow" and carry the trust caveat. Adds the CHANGELOG entry, a compatibility-table row, a workflow-syntax section, and `examples/claude-agent-sdk-setting-sources.yaml`. - Tests: `TestSettingSourcesWiring` no longer swallows six `TestMcpOptionsWiring` tests (it opened without closing the previous class); it now sits after `TestSkillsWiring` and reuses its `_capture_options`/`_argv` helpers. Adds argv assertions for the bare `Skill` grant on the `"all"` path, the `skills: []` opt-out, declared skills not widened by a tier, the omitted-`tools:` preset path, tier parametrization over user/project/local/all-three, schema rejection per provider, and a serialization round-trip. Drops three weaker duplicates and renames `test_setting_sources_isolated_unconditionally` to `..._by_default`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d -v output Follows up the review fixes: the hook warning, the `--provider` override discard path and `_describe_provider` had no test behind them, and the `skills: []` opt-out asserted only the absence of `--allowedTools` with no positive argv anchor beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ea8eac2 to
a32efba
Compare
|
Thanks — this was a thorough review and every finding was actionable. All five blocking items and all five recommended ones are addressed in Per-finding replies are on each thread. This comment covers the two that had no line to anchor to. BLOCKING — stale invariant in AGENTS.md and the user-facing docsEvery row of your table is rewritten as "empty by default, opt-in per workflow via
Also added, all four:
You were right that AGENTS.md is the one that bites hardest, since it's loaded as agent instructions here — a stale invariant there gets asserted as fact by the next agent to touch the file. RECOMMENDED — the test reviewAll of it is in; details on the Two extras your review implied but didn't ask for, because they were the untested consequences of b2 and r3: Two things I did not do, both awaiting your call
Verification
|
Jason Robert (jrob5756)
left a comment
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM, thanks for contributing!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #502 +/- ##
=======================================
Coverage ? 91.91%
=======================================
Files ? 161
Lines ? 25925
Branches ? 0
=======================================
Hits ? 23830
Misses ? 2095
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Fixes #501.
Problem
ClaudeAgentSdkProviderpassedsetting_sources=[]unconditionally, so no workflow could load the settings tier of the repository it operates on. An agent working a target repo could not use that repo's own.claude/skills/,CLAUDE.md, or.claude/rules/*.md— those conventions had to be duplicated into the workflow's prompts.What changed
Adds an opt-in
runtime.provider.setting_sources(user/project/local), defaulting to[]so behaviour is unchanged unless a workflow asks for it.The empty default is load-bearing rather than cosmetic: the SDK re-defaults an unset
setting_sourcesto["user", "project"]wheneverskillsis set, so[]has to be sent explicitly to keep a run hermetic. That property is pinned by a test.Two things this needed beyond the plumbing, both found by running it rather than by reading:
Skilltool whensetting_sourcesis non-empty. CLI-discovered skills never pass throughskill_names, so gating on that alone listed a repo's skills to the model with no tool to invoke them — discovery without execution.ClaudeAgentOptions.skillsto"all"when tiers are enabled and the workflow named no skills. Otherwise every call failed with "not in this session's skills allowlist" — observed with 28 discovered skills, all rejected. A declaredskills:/plugins:list still wins; discovery does not widen what the author asked for.Security note
projectloads the entire tier, hooks included — pointing it at untrusted code runs that code's hooks. Documented on the schema field. The[]default means nobody gets this without asking for it.Whether
usershould be permitted at all is a fair question to settle in review: it makes a run depend on the operator's machine, which is why it is available but not something we use.Verification
Tested against
claude-agent-sdk0.2.87 with a fixture repo whose.claude/rulesis a symlink to a tool-agnostic.agents/rules, and aCLAUDE.mdthat never references it:setting_sources=[]→ the agent reports no rule existssetting_sources=["project"]→ the agent returns the rule's contents and its marker tokenSo skills,
CLAUDE.md, and.claude/rulesall arrive through the one tier, symlinks included.New tests were checked by mutation: reverting the Skill-tool gate fails
test_declared_sources_grant_the_skill_tool, and reverting the filter fails the two"all"tests.Note for reviewers
tests/test_providers/andtests/test_config/pass in full (2683 passed, 1 skipped).Three tests fail on my machine both with and without this change, so they look environment-dependent rather than related:
tests/test_skills/test_path_entries.py::TestUnreadableParent::test_unreadable_parent_is_reported_not_raised_rawtests/test_skills/test_path_entries.py::TestSkillsRootDiagnostics::test_mis_cased_skill_md_is_reportedtests/test_plugins/test_registry.py::TestUnreadableTrees::test_unreadable_skill_subdirectory_is_reportedAll three depend on a
chmod-unreadable path, which does not take effect for my user. Confirmed by running them on a clean checkout ofmain.