Skip to content

Refactor/simplify agent card logic#285

Closed
SyedaAnshrahGillani wants to merge 3 commits intoValueCell-ai:mainfrom
SyedaAnshrahGillani:refactor/simplify-agent-card-logic
Closed

Refactor/simplify agent card logic#285
SyedaAnshrahGillani wants to merge 3 commits intoValueCell-ai:mainfrom
SyedaAnshrahGillani:refactor/simplify-agent-card-logic

Conversation

@SyedaAnshrahGillani
Copy link

📝 Pull Request Template

1. Related Issue

Closes # (issue number)

2. Type of Change (select one)

Type of Change: Code Refactor / New Feature

3. Description

  • Use set intersection to simplify field removal logic in parse_local_agent_card_dict.
  • Restructure find_local_agent_card_by_agent_name to remove unnecessary continue statements and improve readability.
  • Parameterize the agent name in ExecutionPlanner to support different models.
  • Update PlanService to accept and pass the agent name to the planner.
  • Add a test case to verify that the custom agent name is used.

4. Testing

  • I have tested this locally.
  • I have updated or added relevant tests.

5. Checklist

- Use set intersection to simplify field removal logic in `parse_local_agent_card_dict`.
- Restructure `find_local_agent_card_by_agent_name` to remove unnecessary `continue` statements and improve readability.
- Parameterize the agent name in `ExecutionPlanner` to support different models.
- Update `PlanService` to accept and pass the agent name to the planner.
- Add a test case to verify that the custom agent name is used.
@hazeone hazeone requested review from DigHuang, su8su and vcfgv November 1, 2025 02:51
@vcfgv
Copy link
Collaborator

vcfgv commented Nov 1, 2025

Hi, thanks for taking the time to put this pull request together. I really appreciate the effort. We've had a look and decided not to merge these changes at this time. Our team's general approach is to prioritize changes that fix bugs or add new functionality, and these edits feel more like alternative implementations rather than essential improvements.

Looking at the specifics:

  • In card.py, the change from a simple if field in agent_card_dict to using set.intersection() adds a layer of complexity that, in my opinion, doesn't make the code's intent clearer than the original. The straightforward dictionary key check is very readable and idiomatic Python.
  • Similarly, while removing the magic string super_agent is often a good practice, in this case, it introduces new parameters to several function signatures. Since this appears to be a fixed, internal constant, the added complexity to the method calls might not justify the change.

While your suggestions are valid, the existing code is clear and functional, so we'd prefer sticking with it for now. Please don't let this discourage you from future contributions! We would be very grateful to review PRs that address bugs or introduce new functionality.

@vcfgv vcfgv closed this Nov 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants