da-daken commented on issue #1193: URL: https://github.com/apache/flink-agents/issues/1193#issuecomment-6076587532
@wenjin272 Thanks for the clarification. Before implementing the `Skills`-object form, I'd like to confirm two points. **1. Approach A or B?** There are two ways to resolve "all skills from these sources" at runtime: **A: Record "source → skill names" at plan time, expand at open time.** The descriptor stores a marker meaning "all skills from these sources." During `open()`, `SkillManager` loads the actual skill names from the sources and appends them to the descriptor's skill list (de-duplicated, keeping order). The agent only gets skills from the sources the user passed. Requires changes to the `ResourceContext` interface and `SkillManager` in both Java and Python. **B: "Passed a `Skills` object" means "expose all skills the agent can see."** Simpler: when the constructor receives a `Skills` object, the descriptor calls `SkillManager.getAllSkillNames()` at `open()` time. Every skill loaded from any source becomes available to the agent. The agent gets skills from all registered sources, not just the ones the user passed. Skills also carry bash execution capability, which could unintentionally expose privileged skills (e.g. a `db-admin` registered at the environment level by another team member). Which approach do you prefer? **2. If A: Java and Python currently resolve duplicate skill names differently. OK to align Java with Python?** When two sources define a skill with the same name, `SkillManager.registerRepo` simply overwrites (with a WARN log). The question is *which* one wins. Java and Python currently differ: - **Python:** `_add_skills` iterates `skills_objects` in insertion order. The effective order is `@Skills` methods → environment → `agent.add_resource` (including the constructor). The last one wins, so agent-level skills override environment-level ones. This matches the existing precedence for resources with the same name (`env | agent.resources[type]`). - **Java:** `Agent.resources` is a `HashMap`, and `addResourcesIfAbsent` merges environment resources into the same map, losing the level information. `AgentPlan.addSkills` then sorts resources by name (for determinism across JDKs, not as a precedence rule) before merging sources. The winner is simply the one whose resource name sorts last. Under approach A this matters more, because "skills from these sources" is part of the semantic contract — a skill from the constructor's `Skills` object that gets silently overwritten would disappear from the expanded name list entirely. I'd propose making Java follow a level-aware order (`@Skills` methods sorted by name for determinism → environment → agent level), aligning with Python. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
