Skip to content

[Tech Debt][API][ReActAgent] Support skills in constructors - #1223

Merged
wenjin272 merged 6 commits into
apache:mainfrom
da-daken:support_skills_in_constructors
Oct 13, 2026
Merged

wenjin272 merged 6 commits into
apache:mainfrom
da-daken:support_skills_in_constructors

Conversation

@da-daken

Copy link
Copy Markdown
Contributor

Linked issue: #1193

Purpose of change

ReActAgent gains a fourth constructor parameter, skills, so skills can be named where the agent is built
instead of on the chat model descriptor. Java overloads on List<String> and Skills; Python takes
Sequence[str] | Skills | None. Passing nothing keeps today's behavior.

Names go into a copy of the descriptor's skills (descriptor's own first, constructor's appended, first
occurrence kept). A Skills object is registered as an agent-level SKILLS resource plus a
skills_sources marker; SkillManager loads it at plan time and open() resolves the marker into skill
names via ResourceContext.resolveSkillNamesForSources, appending them after the explicit skills. A shared
descriptor is never mutated.

Behavioral Semantics

  • No skills, or an empty Skills → the descriptor's list is used as declared.
  • Names → combined with the descriptor's own; the bash command policy is untouched.
  • Non-empty Skills → names resolved from its sources at runtime, added after the explicit skills.
  • A bare str for Python skills raises TypeError.
  • Ordering: @Skills/@skills methods merge first (Java by method name, Python by declaration order), then
    environment, then agent resources; the later source wins a duplicate name.
  • Degraded paths (unknown source, no skills support, missing marker, null from the bridge) yield no names
    and no exception; only a marker element Jackson cannot convert raises.

Tests

  • Java — ReActAgentTest (constructor form: overlay order, no-op on empty/None, descriptor not
    mutated, language preserved, allowed_commands untouched), ReActAgentSkillsTest (the SKILLS resource
    and its marker, resolved at runtime), SkillManagerTest (source spec → skill names),
    BaseChatModelSetupSkillsTest (the marker survives an AgentPlan round trip), JavaResourceAdapterTest
    (bridge conversion and delegation), AgentPlanDeclareSkillsTest (merge order, and a SKILLS resource
    named the same at the environment and agent level keeping the agent's value).
  • Python — test_react_agent.py (the same constructor contracts as the Java side),
    test_react_agent_skills.py (runtime resolution, merged with the explicit skills),
    test_cross_language_skill_sources.py (specs crossing the bridge as flat {scheme, params} maps).
  • e2e — SkillsIntegrationTest (Java) and agent_skills_test.py (Python) now name the skill on the
    ReActAgent constructor instead of the descriptor

API

  • ReActAgent — Java: new 4-arg constructors over List<String> / Skills, the 3-arg form unchanged (a
    bare null fourth argument is now ambiguous and needs a cast). Python: new skills=None keyword.
  • ResourceContext / JavaResourceAdapter / SkillManager / AgentPlan — new methods; nothing existing
    breaks.
  • Agent.addResourcesIfAbsent — environment entries now come first in iteration order, and a same-name
    clash keeps the agent's value, matching the Javadoc.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

Was this patch authored or co-authored using generative AI tooling?

  • Yes
  • No

Generated-by: cursor (deepseek-v4.1-flash)

@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Oct 11, 2026
@da-daken da-daken changed the title Support skills in constructors [Tech Debt][API][ReActAgent] Support skills in constructors Oct 11, 2026
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. and removed doc-included Your PR already contains the necessary documentation updates. labels Oct 11, 2026

@wenjin272 wenjin272 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this, @da-daken! The constructor support makes skills easier to configure. I left a few inline comments on cross-language wiring and state synchronization, resource precedence, and documentation.


def resolveSkillNamesForSources(self, sources: list[dict]) -> list[str]:
"""Resolve the names of Java-hosted skills declared by source specs."""
return list(self._bridge.resolveSkillNamesForSources(sources) or [])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The actual bridge here is Java PythonResourceAdapterImpl, but resolveSkillNamesForSources() was added to JavaResourceAdapter. A Java agent using a Python model with constructor-provided Skills therefore raises AttributeError, confirmed through real Pemja. Could we add the missing delegation and test the actual bridge?

merged.addAll(this.skills);
}
merged.addAll(resolved);
this.skills = new ArrayList<>(merged);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we synchronize the resolved skill names into both cross-language wrappers after the underlying model opens? PythonChatModelSetup.open() only delegates to Python, leaving the Java wrapper’s skills at its original descriptor value. With source-only configuration, getSkills() remains null, so Java ChatModelAction omits the skill directories from the bash call’s allowed_script_dirs, even after the bridge issue is fixed. The reverse wrapper has the same gap: Python JavaChatModelSetup.skills is not refreshed after Java open(). Copying the final list after each successful open() should address this. Please verify the resulting bash arguments in both directions.

Map<String, Object> existing = this.resources.get(type);
// Rebuild to establish env-before-agent iteration order.
Map<String, Object> merged = new LinkedHashMap<>(envTyped);
merged.putAll(existing);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LinkedHashMap.putAll() replaces values without moving existing keys. With environment keys [z, a] and an agent override for z, the order becomes [agent-z, env-a], allowing environment skills to override agent skills. Could we remove overlapping keys before appending agent entries? The annotation-to-resource merge needs the same treatment.

- The descriptor you pass in is not modified; the agent uses a copy with the combined skills.
- Only skill names are combined. The command policy of the built-in `bash` tool, such as `allowed_commands`, is always taken from the chat model descriptor, so set it there.

The skill sources must still be registered on the execution environment with `add_resource(..., ResourceType.SKILLS, ...)`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also document the Skills-object constructor with Java/Python examples? This section only describes skill-name lists and says sources must be registered on the environment, but passing a Skills object registers its sources on the agent automatically. Please distinguish the two forms and document how source-derived names combine with explicit descriptor skills, including the annotation → environment → agent precedence.

@da-daken
da-daken force-pushed the support_skills_in_constructors branch from e0058c9 to d5ca3f2 Compare October 12, 2026 17:38
@da-daken

Copy link
Copy Markdown
Contributor Author

@wenjin272 good catch! Thank you for your review. I have fixed the four issues mentioned above:

  • Missing implementation in the cross-language bridge.
  • Sync the skills state of the cross-language wrapper; after open(), start its own skill resolution.
  • Ensure that the later-overriding order is loaded first.
  • Supplement the Skills object documentation in react_agent.md.

@wenjin272 wenjin272 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing my comments. LGTM. Could you rebase this PR onto the latest main and resolve the merge conflicts?

da-daken and others added 6 commits October 13, 2026 15:23
…ReActAgent constructors (apache#1193)

- Java/Python ReActAgent: accept Skills object in constructor, register SkillSourceSpec marker via descriptor arguments, resolve at runtime through SkillManager.open().
- Agent-level skills override environment-level skills (layer-priority).
- BaseChatModelSetup.open(): merge skills_sources from descriptor args.
- ResourceContext: add resolveSkillNamesForSources() API.
- SkillManager: maintain source-to-skills reverse index.
- Agent: LinkedHashMap for insertion-order resource preservation.
- AgentPlan: TreeMap-sorted skills before agent resources for correct priority.
- 12 new tests across 4 files (api, plan, runtime, Python).
- Missing implementation in the cross-language bridge.
- Sync the skills state of the cross-language wrapper; after open(), start its own skill resolution.
- Ensure that the later-overriding order is loaded first.
- Supplement the Skills object documentation in react_agent.md.
@da-daken
da-daken force-pushed the support_skills_in_constructors branch from d5ca3f2 to 9e8a60d Compare October 13, 2026 07:36
@wenjin272
wenjin272 merged commit 5d49238 into apache:main Oct 13, 2026
55 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants