fix(harness): resolve parent ToolsConfig before building subagent factories

[Dedre2001]

Summary

Fixes #3178 — a declared subagent lost every MCP tool when the parent's MCP servers came from the workspace tools.json and the declaration carried a non-empty tools allow-list.

Root cause

HarnessAgentBuilderSupport captured the parent config as b.toolsConfigOverride at factory-construction time, while HarnessAgent.build() resolves the effective ToolsConfig later (override, else ToolsConfigLoader.load(...)). For a tools.json-configured parent the captured value was therefore null:

• childToolsConfig(null, allow) skipped child.setMcpServers(parent.getMcpServers()), and
• the resulting strict child config (strictAllow=true, defaultToolsEnabled=false) also suppressed the child's own tools.json load.

Net effect: no MCP tools in the child even though the allow-list named them.

Changes

1. resolveEffectiveToolsConfig(builder, wsManager) — builder override, else workspace tools.json through the same loader, resolved before the subagent factories are built.
2. Threaded through both entry builders (buildSubagentEntries, buildStaticSubagentEntries), both factory builders (buildGeneralPurposeFactory, buildDeclaredFactory) and the two middleware builders. Every changed signature keeps a backwards-compatible overload that falls back to b.toolsConfigOverride, so existing callers and tests compile and behave exactly as before.
3. childToolsConfig:
   • empty allow-list → an explicit opt-out config with no inherited MCP servers, plus a WARN naming the servers that are no longer inherited;
   • non-empty allow-list → unchanged strict filter, parent MCP servers still propagated;
   • allow-list entries the parent does not offer are dropped with a WARN instead of silently.
4. Javadoc on the affected helpers documents the precedence and the opt-out semantics.

Deliberate deviation from the agreed spec (item 2)

Spec v2 phrased the non-empty branch as "copy only parent MCP servers named in allow". I kept the wholesale copy: the mcpServers map is keyed by server name, not tool name, so filtering it by a tool allow-list would drop every server and reintroduce the bug. The strict allow-list already restricts which tools survive registration.

Tests

New SubagentToolsConfigPropagationTest (4 cases, no MCP server needed) + existing ManagedSubagentBoundaryTest (unchanged):

Tests run: 6, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

• empty allow-list ⇒ explicit child config, no inherited MCP servers (the [Bug]: Declared/general-purpose subagents never inherit MCP tools (inherited toolkit captured from b.toolkit before MCP registration) #3178 regression)
• non-empty allow-list ⇒ strict filter kept, parent MCP servers still propagated
• unknown allow-list entry ⇒ dropped, not propagated
• null parent ⇒ tolerated in both cases
• existing allowlist-boundary / denied-tool-re-enable tests still pass

mvn -pl agentscope-harness spotless:apply was run, so the format check passes.

Note: this repository has no per-PR changelog file (recent merges are conventional-commit titles plus tests), so no changelog entry is included. The migration note lives in the helper javadoc: an empty allow-list is now a true MCP opt-out — list the required MCP tool names to restore inheritance.

Out of scope

Reusing the parent's MCP client/connections is deliberately not part of this PR (agreed in the issue thread); it needs its own lifecycle contract.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

❌ Patch coverage is 98.43750% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...cope/harness/agent/HarnessAgentBuilderSupport.java	98.36%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Fixes #3178 by resolving the parent ToolsConfig (builder override, else workspace tools.json) before the subagent factories are built, instead of capturing b.toolsConfigOverride — which was null for a tools.json-configured parent, so every declared child lost all MCP tools. The root-cause analysis is right and the new unit tests are genuinely good (they pin both the resolution order and the allow-list filtering). My concern is that the change reaches past the reported bug in three ways: it bypasses disableToolsConfig, it changes what an absent tools: list means for existing workspaces, and it now always overrides a child's own tools config.

Findings

• [Critical] HarnessAgentBuilderSupport.java:379-387 — resolveEffectiveToolsConfig mirrors HarnessAgent.build()'s precedence but drops the !disableToolsConfig gate (HarnessAgent.java:2761), so disableToolsConfig() no longer keeps a workspace tools.json (and its ${ENV}-substituted MCP credentials) away from subagents.
• [Warning] :330-341 — SubagentDeclaration.getTools() normalises an absent tools: key to List.of(), so the new "true opt-out" branch also catches every declaration that simply never mentioned tools; those children also lose the parent's allow / strictAllow / defaultToolsEnabled, which widens rather than narrows their tool surface, and the SubagentDeclaration javadoc ("Empty means inherit all parent tools") now contradicts the code.
• [Warning] :343 — both branches return a non-null config, so sub.toolsConfig(childTools) always fires and replaces an ISOLATED child's own tools.json.
• [Warning] :906-911 — tools.json is read twice per build (here and at HarnessAgent.java:2761); the reads can disagree if the file changes in between. Hoisting the resolution above the subagent block and passing the value in avoids both the duplicate workspace read and the divergence.
• [Info] :361 — deny is shared by reference with the parent while mcpServers is copied.

Verdict

Posting as COMMENT rather than REQUEST_CHANGES because the direction is sound and the fixes are small. Please address the disableToolsConfig gate before merge — it is a privilege-boundary regression rather than a style issue — and decide explicitly whether the absent-tools: case should change behaviour; if it should, a SubagentDeclaration javadoc update plus a release note would keep the upgrade from being silent. Happy to re-review on the next push. CLA is signed, CI is green, mergeable=true; merging stays with the maintainers.

---

Automated review by github-manager-bot

[oss-maintainer]

[Critical] This helper does not mirror HarnessAgent.build() completely: the build path gates the read with if (!disableToolsConfig) (HarnessAgent.java:2761), but here the workspace tools.json is loaded unconditionally whenever toolsConfigOverride == null.

So HarnessAgent.builder().disableToolsConfig() on a workspace that still contains a tools.json now has the opposite effect for children: the parent keeps a clean default toolkit, while resolveEffectiveToolsConfig hands the file's mcpServers to every subagent factory (buildGeneralPurposeFactory line 483, buildDeclaredFactory line 654) and the child then registers those MCP servers itself. Since ToolsConfigLoader performs ${ENV_VAR} substitution, that means servers with credential-bearing headers get attached to a subagent on a path where the caller explicitly asked for no tools config at all.

Suggested fix — read the same flag (it is package-private on Builder, so this class can see it):

static io.agentscope.harness.agent.tools.ToolsConfig resolveEffectiveToolsConfig(
        HarnessAgent.Builder b, WorkspaceManager wsManager) {
    if (b.disableToolsConfig) {
        return null;
    }
    if (b.toolsConfigOverride != null) {
        return b.toolsConfigOverride;
    }
    ...
}

and please add a regression test (disableToolsConfig + a tools.json on disk must yield a null effective config) — none of the current tests cover that flag.

[oss-maintainer]

[Warning] The blast radius of this branch is much larger than the case described in #3178, because it is not only an explicit empty list that lands here.

SubagentDeclaration.getTools() normalises an absent tools: key to List.of() (SubagentDeclaration.java:146, this.tools = b.tools != null ? List.copyOf(b.tools) : List.of()), so every declaration that does not mention tools: at all takes this path. Before this change those children got the parent's config object verbatim (return parent), i.e. they inherited the parent's allow, strictAllow and defaultToolsEnabled; after it they get a brand-new config that copies only deny and leaves allow == null with defaultToolsEnabled == true (field default). Two silent behaviour changes follow:

1. existing workspaces whose subagents relied on inherited MCP servers lose them on upgrade, with only a WARN line as the signal — and that is exactly the failure mode [Bug]: Declared/general-purpose subagents never inherit MCP tools (inherited toolkit captured from b.toolkit before MCP registration) #3178 reported, now applied to the inherit-all case;
2. a parent restricted by an allow list no longer restricts its children: childToolsConfig returns an unfiltered config (allow == null, defaultToolsEnabled == true, strictAllow == false), which widens the child's tool surface instead of narrowing it.

The Javadoc on the declaration side still says the opposite (SubagentDeclaration.java:315-318: "Empty means inherit all parent tools"), so the model docs and this method now disagree.

Would you consider one of:

• keeping inheritance for the absent case and only opting out when the author wrote tools: [] explicitly — that needs an "absent vs. empty" distinction on SubagentDeclaration (e.g. keep List<String> tools nullable, or add a toolsExplicitlyEmpty flag) since getTools() cannot tell them apart today;
• or, if the opt-out is really intended for both, copying parent.getAllow() / isStrictAllow() / isDefaultToolsEnabled() into optOut so only MCP is dropped, updating the SubagentDeclaration Javadoc, and calling the change out in the release notes so users migrating don't lose tools silently.

[oss-maintainer]

[Warning] Both branches of childToolsConfig now return a non-null object (return optOut here, return child below), so the call site if (childTools != null) sub.toolsConfig(childTools) at line 654 fires unconditionally and the declared child always ends up with an explicit override.

Before this PR a declared child with no allow-list and a parent whose config came only from tools.json received null, which let the child's own HarnessAgent.build() resolve tools.json from the child's own workspace. For WorkspaceMode.ISOLATED subagents that ship their own tools.json, this PR now replaces that file with the parent-derived config, so the child's own MCP servers / allow-deny lists are ignored with no warning.

Two options: keep returning null when there is nothing to propagate (empty allow-list and no parent config to derive from), or skip sub.toolsConfig(...) for WorkspaceMode.ISOLATED children that resolve their own workspace.

[oss-maintainer]

[Warning] tools.json is now read twice per agent build: once from resolveEffectiveToolsConfig here (and again in buildDynamicSubagentsMiddleware, line 930) and once in HarnessAgent.build() at HarnessAgent.java:2761. ToolsConfigLoader.load does a filesystem read through WorkspaceManager (which may be a sandbox or remote filesystem — the loader's own comment says it can be unreachable at build time) plus env substitution and a Jackson parse.

The bigger issue is consistency, not cost: if the file changes between the two reads (console edit, skill_manage writing the workspace), the parent and its children end up with different effective configs and nothing reports the divergence.

Since buildSubagentsMiddleware / buildDynamicSubagentsMiddleware are called from build() at lines 2638/2653, before the tools.json block at 2761, the cheap fix is to hoist the resolvedToolsConfig computation above the subagent block and pass it in as a parameter instead of resolving it a second time inside the helper.

[oss-maintainer]

[Info] Minor aliasing asymmetry: setMcpServers defensively copies into a new LinkedHashMap<> (ToolsConfig.java:96-98), but setDeny stores the reference, so parent and child share one mutable List in both branches here. Nothing mutates ToolsConfig today so it is not a live bug, but a List.copyOf(parent.getDeny()) (or a comment stating the config is treated as immutable) would keep the two fields consistent.

[Dedre2001]

@oss-maintainer Thanks for the review. Pushed follow-up commits 88aa170 and 356efd6 to this PR.

• Finding 1: Restored the disableToolsConfig() gate before resolving parent configuration. The follow-up also prevents shared-workspace declared children and the built-in general-purpose child from reloading the disabled parent file. Declaration-derived tool filters remain active, and isolated children can still load their own workspace configuration.
• Finding 2: Added explicit declaration-presence tracking. An absent tools key inherits the resolved parent configuration unchanged; an explicitly empty list opts out of inherited tools/MCP servers. Updated the declaration Javadoc accordingly. Explicit-empty behavior is intentional and differs from the base behavior.
• Finding 4: Resolve the parent configuration once before constructing subagent middleware and reuse that result for the parent toolkit and child factories.
• Finding 5: Copy deny when creating a derived child configuration rather than sharing the parent's list.

Finding 3 remains partially unresolved. When tools is absent and the parent has no effective configuration, we now leave the child override unset so its own workspace configuration can load. When the parent has an effective configuration, it still takes precedence over an ISOLATED child's tools.json.

A complete fix needs an explicit precedence contract for parent configuration, child configuration, and declaration-level tool restrictions. In particular, child configuration must not accidentally undo declaration filters, and the current boolean fields do not distinguish omitted values from explicitly supplied defaults. I have therefore not added automatic configuration merging in these commits. Could you advise whether this remaining case must be handled in this PR, or whether a separately scoped configuration-precedence change is acceptable? I am leaving Finding 3 open pending that decision.

Local validation on Java 17 / Windows:

• HarnessAgentTest: 51 tests passed.
• ManagedSubagentBoundaryTest: 2 tests passed.
• SubagentToolsConfigPropagationTest: 20 tests passed on the final targeted rerun, including 8 new spawned-child cases. These assert that disabled shared configuration is never loaded, enabled parent configuration is read once, declaration filtering survives, and an isolated child can load its own configuration.
• spotless:apply and git diff --check passed.
• Tests used -DforkCount=0; no new local JaCoCo coverage result is claimed.

Please re-review the updated commits, including the remaining scope decision for Finding 3.

[oss-maintainer]

Summary

Re-review of e947bfee..356efd69 (two new commits). Both blockers from the previous round are fixed, and fixed the way they needed to be — this is no longer a privilege-boundary risk. Posting as COMMENT rather than APPROVE only because CI has not finished on the new head (build (ubuntu-latest) and build (windows-latest) were still in_progress when I looked) and there are two semantics points left below; neither needs a redesign.

What the increment resolved:

• resolveEffectiveToolsConfig now returns null when disableToolsConfig() is set, so HarnessAgent.build() resolves tools.json once, above the subagent block, and the same value flows to the main toolkit and every child factory — that removes the previous Critical (workspace tools.json and its ${ENV} credentials reaching children past the opt-out), the double read, and the possible mid-build divergence between the two reads. disableToolsConfigKeepsWorkspaceToolsJsonAwayFromSubagents plus the loader.verify(times(disabled ? 0 : 1)) assertion in spawnedSharedChildHonoursParentToolsConfigSwitch pin both halves.
• The absent-vs-explicitly-empty ambiguity is now a real distinction via SubagentDeclaration.isToolsDeclared(), and the default branch (return parent) restores the pre-#3178 behaviour for declarations that never mention tools: — which was the blast-radius concern. The javadoc contradiction is gone, deny is copied defensively, and the new spawned-child tests go through the actual builder path rather than only childToolsConfig.

Findings

• [Warning] SubagentDeclaration.java:327 — the new distinction is not reachable from agents.md / spec files: AgentSpecLoader.java:337 maps tools.isEmpty() → null, so an explicit tools: [] is still indistinguishable from an absent key and inherits everything. The javadoc promises the opposite for "key is present but empty".
• [Warning] HarnessAgentBuilderSupport.java:679 — carry-over: a non-null parent config still shadows an ISOLATED child's own tools.json, silently. The new WorkspaceMode.SHARED gate in the else if shows the right instinct; worth deciding the ISOLATED precedence explicitly (code or docs + release note).
• [Info] HarnessAgentBuilderSupport.java:337 — parent is returned by reference and shared by the parent toolkit and every non-declaring child; copyList is right there if you would rather hand out a copy.

Verdict

The fix is correct and the test work is thorough — the two spawnedSharedChild* parameterized tests exercise real build() paths with a mocked static loader instead of asserting on helper return values, which is what this area needed. CLA is signed, mergeable=true, no conflicts. My remaining ask is the AgentSpecLoader gap: it is the difference between "the opt-out exists" and "a user can actually use it". @Dedre2001 — if you would rather keep tools: [] as a builder-only capability, saying so in the javadoc is enough for me. Merging stays with the maintainers; @mention me after the next push or when CI closes and I will re-check.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] This is the one semantics change in the PR that file-based declarations cannot express.

The absent-vs-explicitly-empty distinction is introduced on the builder field (this.toolsDeclared = b.tools != null), but the path that actually builds declarations from agents.md / agent-spec files collapses the two before they get here: AgentSpecLoader.java:337 does .tools(tools.isEmpty() ? null : tools), and parseToolNames returns List.of() for both a missing key and tools: []. So a spec file that says tools: [] arrives as null → toolsDeclared=false → inherits every parent tool, i.e. exactly the behaviour the author was trying to opt out of, and the opposite of the sentence above.

The consequence is that the new opt-out only works for programmatic SubagentDeclaration.builder().tools(List.of()) callers, while the javadoc promises it for "the key is present but empty" — which is precisely the file case that cannot be detected. A user reading this javadoc, writing tools: [], and getting the parent's MCP servers anyway has no signal to explain why.

Suggested fix: preserve presence in the loader rather than folding it away —

List<String> tools = parseToolNames(asString(fm.get("tools")));
boolean toolsPresent = fm.containsKey("tools") || fm.containsKey("tool_names");
...
.tools(toolsPresent ? tools : null)   // tools may be empty => explicit opt-out

and ideally add one AgentSpecLoader-driven test (front matter with tools: [] vs no tools key) so the distinction is pinned from the user-facing side, not only from the builder.

[oss-maintainer]

[Warning] Carry-over from last round: with a non-null parent config, an ISOLATED child still never reads its own tools.json.

childToolsConfig(capturedToolsConfig, decl.getTools(), decl.isToolsDeclared()) returns the parent object unchanged whenever the declaration does not list tools, so childTools != null and sub.toolsConfig(childTools) fires. The child's build path then prefers toolsConfigOverride over ToolsConfigLoader.load(wsManager), which means an ISOLATED child that ships its own agents/<name>/workspace/tools.json gets the parent's servers, allow, deny, strictAllow and defaultToolsEnabled instead — silently, and without the WARN that the empty-allow-list branch emits.

The new else if (capturedDisableToolsConfig && SHARED) branch shows the right instinct (workspace mode matters), and disabledParentStillAllowsIsolatedChildToLoadItsOwnConfig covers the disabled-parent case only. Worth deciding here too: either skip propagation for WorkspaceMode.ISOLATED when the declaration did not list tools (let the child's own workspace win), or keep the precedence and say so in the SubagentDeclaration javadoc plus a release note, because "the parent's tools.json overrides the isolated child's" is a visible behaviour change for every existing isolated declaration whose parent has a tools.json — which after this PR is every tools.json-configured parent.

[oss-maintainer]

[Info] Aliasing nit on the new branch: parent is returned by reference, so one mutable ToolsConfig instance is now shared by the parent and every non-declaring child, and childToolsConfig(null, …, false) returns null — see the test's assertNull(... "so the child can load its own workspace tools.json"), which is the behaviour that makes the previous comment matter.

That is defensible for a read-once config, but ToolsConfig is a mutable bean (setAllow / setDeny / setMcpServers / setDefaultToolsEnabled / setStrictAllow) and it is deliberately mutable elsewhere in this very method (child.setStrictAllow(true), optOut.setMcpServers(Map.of())). Since resolveEffectiveToolsConfig is now hoisted and the same object is handed to the main toolkit and to N child factories, a single future child-side mutation would change what every sibling and the parent register.

If sharing is intended, a copyOf(parent)-style helper here (the copyList helper is already in the file, and setMcpServers already copies the map) would cost nothing and keep the semantics identical, at which point assertNull(child == parent) in the test can stay as an equality check instead of an identity check.

[oss-maintainer]

Follow-up on the review above: build (ubuntu-latest), build (windows-latest), Check License, Check Module Sync and codecov/patch have all closed green on 356efd69, so the CI part of my "not approving yet" no longer applies.

What still holds me at COMMENT is only the two semantics points, and both are one-liners:

1. AgentSpecLoader.java:337 folds an explicit tools: [] into null, so the absent-vs-empty distinction this PR adds cannot be expressed in an agents.md / agent-spec declaration — while the new SubagentDeclaration.getTools() javadoc says it can. Either preserve key presence in the loader, or scope the javadoc to programmatic declarations.
2. A non-null parent ToolsConfig still shadows an ISOLATED child's own tools.json. Fine as an intended precedence, but it deserves one sentence in the javadoc (or a release note), because after this PR every tools.json-configured parent hits that path.

If 1 is deliberate, say so and I will approve on the next pass. Merging stays with the maintainers.

---

Automated review by github-manager-bot

[Dedre2001]

@oss-maintainer Pushed c82dc5f to address the two remaining semantics points. Could you give this a final re-review when CI completes?

1. File-based explicit opt-out: AgentSpecLoader now preserves the presence of the tools key rather than converting every empty list to null. An omitted key continues to inherit; tools: [] explicitly opts out of inherited tools/MCP servers. Two file-loading regression cases verify the declaration flag and the resulting parent MCP inheritance behavior.
2. ISOLATED precedence: This PR intentionally retains parent-derived configuration precedence and does not introduce configuration merging. The Javadoc now explicitly states that a non-null parent configuration, including one loaded from the parent's workspace, is passed as a child override after declaration-level tool selection and takes precedence over the child's own tools.json, including for ISOLATED children. With no parent configuration and no declared tool list, the child can load its own file. This documents the remaining Finding 3 behavior; it does not claim that child-local configuration now overrides or merges with the parent.

Javadoc source: SubagentDeclaration.getTools().

Validation on Java 17 / Windows, using an ASCII checkout and a forked test JVM with a fresh clean test:

• HarnessAgentTest: 51 passed.
• ManagedSubagentBoundaryTest: 2 passed.
• SubagentDeclarationPhaseATest: 9 passed.
• SubagentToolsConfigPropagationTest: 22 passed.
• Total: 84 tests, 0 failures, 0 errors, 0 skipped.
• Formatting and git diff --check passed.
• Fresh local JaCoCo XML intersected with changed production-code lines: this increment has 1/1 lines and 2/2 branches covered. The full PR against be7211d has 59/59 executable changed lines covered and 43/44 branches on those lines covered (97.73%). One pre-existing changed line is partially branch-covered; counting only fully covered lines yields 58/59 (98.31%). These are local JaCoCo measurements, not a claim about the pending remote Codecov result.

The two remaining asks are addressed through the loader fix and the explicit precedence documentation, respectively. Please let me know if anything else is needed for approval; merging remains with the maintainers.

[oss-maintainer]

Summary

Re-reviewed e947bfee..c82dc5f6; the two semantics items from the previous round are both closed, and this is now a coherent fix for #3178. What remains is one aliasing hazard on the inherit path and one consistency note, neither of which is a redesign.

Closed since last round

• AgentSpecLoader.java:337 maps the front-matter key itself (fm.containsKey("tools")) instead of tools.isEmpty(), so tools: [] in an agents.md really is the opt-out the javadoc advertises, and fileDeclarationPreservesAbsentVersusEmptyTools pins both the absent and empty arms against childToolsConfig — exactly the regression guard the last round asked for.
• SubagentDeclaration#getTools() now states the precedence rule out loud (a non-null parent config overrides the child's own tools.json, including under ISOLATED, no merge). Choosing documentation over code here is defensible, and it is the reading the SHARED gate in the builder already implements.

Open

• [Warning] HarnessAgentBuilderSupport.java:337 — the inherit branch returns the parent instance itself; every other branch in the method defensively copies. See the inline comment.
• [Info] AgentSpecLoader.java:338 — skills still conflates absent and explicitly-empty, one line below the fix that removed that conflation for tools.

Not verified locally: no build or test run on my side; build (ubuntu-latest) and build (windows-latest) were still queued at review time, and Check License / Check Module Sync are green. Also license/cla reports no status on this head (it does on neighbouring PRs), so I am leaving this as COMMENT rather than APPROVE — worth confirming the CLA bot picked up 代志杰 before merge.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] The non-declaring branch hands the parent's ToolsConfig out by reference, so parent and every non-declaring child share one mutable instance.

allowDeclared == false returns parent itself, and the result is then passed to each child builder via sub.toolsConfig(childTools). buildStaticSubagentEntries builds the factories once and each spawned child receives the same object, so any future per-child mutation — a setAllow/setDeny/setMcpServers on the child's resolved config, or a skill-bound tool group registering into getMcpServers() (which returns the live map, ToolsConfig.java:92) — writes through to the parent's toolkit and to every sibling.

The rest of the method is careful about exactly this (copyList(parent.getDeny()) on both other branches, and setMcpServers copies the map on write), so this one line is the outlier. Cheap to close: return a shallow copy on the inherit branch (the same field-by-field copy the else paths already build), and it also makes the assertNotNull(child) in the new fileDeclarationPreservesAbsentVersusEmptyTools test able to detect aliasing if you add assertNotSame(child, parentToolsConfig).

[oss-maintainer]

[Info] tools now distinguishes absent from explicitly-empty, but skills two lines below still collapses them.

.skills(skills.isEmpty() ? null : skills) is the exact shape that made tools: [] unreachable from a spec file, which c82dc5f just fixed one line above. So today tools: [] means "no inherited tools" while skills: [] still means "inherit everything" — two opposite readings of the same YAML idiom in the same builder chain, and the new javadoc in SubagentDeclaration#getTools() promises readers the opposite.

Not something to widen this PR for: worth either the same fm.containsKey("skills") treatment here (it is a one-token change), or a follow-up issue so the asymmetry is deliberate and recorded rather than accidental.

[Dedre2001]

@oss-maintainer Pushed 7f0c357 to address the remaining configuration-aliasing warning. Could you re-review this increment when CI completes?

The undeclared-tools branch now returns a separate ToolsConfig, copying allow and deny and using the existing defensive map copy in setMcpServers. Both boolean flags retain their parent values, and a null parent still yields null so the child can load its own workspace configuration. This is the requested shallow copy, consistent with the other branches: server configuration values are retained, while the config bean, lists, and server map are isolated. Tool selection and the documented parent-over-child precedence are unchanged.

Regression tests now verify value preservation, distinct parent/child and sibling configurations, isolation after parent-list and child-config/map mutations, both boolean flag settings, and preservation of null fields/defaults. The file-based declaration test also checks that the child does not receive the parent config object itself.

Validation: 86 tests passed, 0 failures/errors/skips (HarnessAgentTest: 51; ManagedSubagentBoundaryTest: 2; SubagentDeclarationPhaseATest: 9; SubagentToolsConfigPropagationTest: 24). Formatting and git diff --check passed. A fresh forked Java 17 clean test in the ASCII checkout generated JaCoCo data: this increment covers 8/8 changed executable lines and 2/2 branches; the full PR against be7211d covers 66/66 changed executable lines and 45/46 branches on those lines (97.83%). These are local JaCoCo measurements; the new remote CI/Codecov run is separate.

I have left the informational skills semantics point outside this PR, as suggested. I also verified the new head: license/cla is now SUCCESS (Contributor License Agreement is signed). Module Sync has passed; the Ubuntu/Windows builds are running.

Please let me know if anything remains for approval. Merging stays with the maintainers.

[oss-maintainer]

Summary

Re-reviewed c82dc5f6..7f0c3572. That closes the last open item, and with license/cla now reported success on this head and every check green, this is approved from my side.

childToolsConfig no longer hands the parent instance to non-declaring children: the inherit branch builds a separate ToolsConfig, copies allow/deny through copyList, relies on setMcpServers' own defensive map copy, carries both boolean flags across, and still returns null for a null parent so a child with no parent configuration loads its own workspace config. That is the shallow-copy semantics the other two branches already had, so the method is now internally consistent — no branch leaks a shared mutable object into a child.

The test work is what makes this more than a one-line change: assertNotSame on parent/child and across siblings, mutation of parent lists and of the child's config and server map proving neither direction leaks, both boolean flags in both states, and null fields/defaults preserved. fileDeclarationPreservesAbsentVersusEmptyTools also now checks the child did not receive the parent object itself — which is the assertion that would have caught the original aliasing.

On skills: leaving the absent-versus-explicitly-empty asymmetry out of this PR is the right call, and thanks for saying so rather than silently expanding the scope. It is a real inconsistency (in the same three lines of AgentSpecLoader, tools: [] is now an opt-out while skills: [] still means "inherit everything"), so it deserves its own issue rather than disappearing — thanks also for confirming #3178's own behaviour is unaffected either way.

Not verified locally: no build or test run on my side, so the "86 tests / 8/8 changed lines" figures quoted in the thread are the author's local JaCoCo numbers and I am taking them as reported. What I am confirming independently is GitHub's own state on 7f0c3572: Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch and license/cla all green, mergeable=true. Merging stays with the maintainers.

---

Automated review by github-manager-bot