Skip to content

Conversation

@supreme-gg-gg
Copy link
Contributor

@supreme-gg-gg supreme-gg-gg commented Nov 12, 2025

Completes #1068

  • Integrates OpenAI SDK (fixed event converters, session service, tools, tracing) and added e2e test for OpenAI SDK
  • Refactor skills prompt and tool logic into kagent-skills, update ADK and OpenAI SDK to wrap them with their functions tool implementations
  • Improvements to handling bash tool venv and python path

EItanya and others added 13 commits December 4, 2025 00:29
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>
Co-authored-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
@supreme-gg-gg supreme-gg-gg changed the title Refactor agent skills and support OpenAI Agents SDK (feat) refactor agent skills and support OpenAI Agents SDK Dec 4, 2025
@supreme-gg-gg supreme-gg-gg changed the title (feat) refactor agent skills and support OpenAI Agents SDK feat: refactor agent skills and support OpenAI Agents SDK Dec 4, 2025
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
@supreme-gg-gg supreme-gg-gg marked this pull request as ready for review December 4, 2025 21:45
Copilot AI review requested due to automatic review settings December 4, 2025 21:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors agent skills architecture and adds OpenAI Agents SDK support by creating a centralized kagent-skills package that both ADK and OpenAI integrations can leverage. The refactoring improves code reusability and maintainability while enabling OpenAI agents to utilize the same skills infrastructure as ADK agents.

Key Changes:

  • Created kagent-skills package with core skills logic (discovery, session management, shell operations, prompts)
  • Added kagent-openai package with OpenAI Agents SDK integration including tools, session service, event conversion, and agent executor
  • Refactored kagent-adk to use shared skills package, removing duplicated code

Reviewed changes

Copilot reviewed 50 out of 54 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
python/packages/kagent-skills/ New centralized package for skills discovery, file operations, shell execution, and session management
python/packages/kagent-openai/ New OpenAI SDK integration with A2A protocol support, session persistence, and tools
python/samples/openai/basic_agent/ Sample OpenAI agent demonstrating calculator and weather tools
python/packages/kagent-adk/src/kagent/adk/tools/ Refactored to use kagent-skills package instead of local implementations
python/packages/kagent-crewai/ Import order fixes and CrewAI version update to >= 1.2.0
python/Dockerfile Added kagent-skills package, updated PYTHONPATH and venv configuration
go/test/e2e/ Added e2e tests for OpenAI agent invocation with mocked LLM responses
python/pyproject.toml Added samples/openai to workspace members

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


try:
stdout, stderr = await asyncio.wait_for(process.communicate(), timeout=timeout)
except TimeoutError:
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The TimeoutError exception should be asyncio.TimeoutError for clarity and to avoid potential conflicts with the built-in TimeoutError (added in Python 3.10). While they may be the same in Python 3.13+, being explicit improves code clarity.

Change:

except TimeoutError:

to:

except asyncio.TimeoutError:

Copilot uses AI. Check for mistakes.
Comment on lines 117 to +118
# Create a separate venv for bash tool commands (sandbox environment)
# This venv does not have pip installed
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The comment on line 118 states "This venv does not have pip installed" but this is misleading. The bash tool description in prompts.py mentions "pip install" with a 120s timeout, and the _get_command_timeout_seconds function specifically handles "pip install" commands. If pip is not installed in the sandbox venv, then pip install commands will fail, making the timeout configuration useless.

Either:

  1. Install pip in the sandbox venv, OR
  2. Remove pip-related timeout logic and documentation if pip is intentionally excluded

Copilot uses AI. Check for mistakes.
Comment on lines +157 to +160
footer = (
"\n\n---"
"The skill has been loaded. Follow the instructions above and use the bash tool to execute commands."
)
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

Missing a newline after the "---" separator on line 158. The footer should be:

footer = (
    "\n\n---\n"
    "The skill has been loaded. Follow the instructions above and use the bash tool to execute commands."
)

This ensures proper markdown formatting with whitespace before and after the horizontal rule.

Copilot uses AI. Check for mistakes.
"index": 0,
"message": {
"role": "assistant",
"content": "The weather in london is Rainy, 52°F"
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

Inconsistent spelling: "london" should be capitalized to "London" to match the input and be consistent with the rest of the response. The weather data uses "london" as the key (line 58) which is fine for internal lookup, but the response should preserve the proper capitalization.

Change line 122 from:

"content": "The weather in london is Rainy, 52°F"

to:

"content": "The weather in London is Rainy, 52°F"
Suggested change
"content": "The weather in london is Rainy, 52°F"
"content": "The weather in London is Rainy, 52°F"

Copilot uses AI. Check for mistakes.
ENV PATH="/.kagent/.venv/bin:$PATH"
ENV UV_PROJECT_ENVIRONMENT=/app/.venv
ENV BASH_VENV_PATH="/.kagent/sandbox-venv"
ENV PYTHONPATH="/.kagent:/"
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

Inconsistent environment variable naming: The Dockerfile uses PYTHONPATH=/.kagent:/ (line 126) which adds /.kagent to the path, but the kagent-skills shell.py adds the working directory and /skills to PYTHONPATH (line 124). This could lead to import confusion.

The /.kagent path in the Dockerfile doesn't seem to be referenced anywhere else in the codebase, and it's unclear what should be imported from there. Consider documenting why /.kagent needs to be on PYTHONPATH or removing it if it's not necessary.

Suggested change
ENV PYTHONPATH="/.kagent:/"

Copilot uses AI. Check for mistakes.
Comment on lines +131 to +136
bash_venv_path = os.environ.get("BASH_VENV_PATH")
if bash_venv_path:
bash_venv_bin = os.path.join(bash_venv_path, "bin")
# Prepend bash venv to PATH so its python and pip are used
env["PATH"] = f"{bash_venv_bin}:{env.get('PATH', '')}"
env["VIRTUAL_ENV"] = bash_venv_path
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

Missing error handling for the case when BASH_VENV_PATH environment variable is set but the path doesn't exist or is invalid. If bash_venv_path is set but the directory doesn't exist, this could lead to the command failing with unclear errors.

Consider adding validation:

if bash_venv_path:
    if not os.path.exists(bash_venv_path):
        logger.warning(f"BASH_VENV_PATH {bash_venv_path} does not exist, using system python")
    else:
        bash_venv_bin = os.path.join(bash_venv_path, "bin")
        env["PATH"] = f"{bash_venv_bin}:{env.get('PATH', '')}"
        env["VIRTUAL_ENV"] = bash_venv_path

Copilot uses AI. Check for mistakes.
timeout=self._config.execution_timeout,
)

except TimeoutError:
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The TimeoutError exception should be asyncio.TimeoutError for clarity and consistency. While they may be the same in Python 3.13+, being explicit about using asyncio's TimeoutError improves code clarity, especially since asyncio.wait_for is being used above.

Change line 284 from:

except TimeoutError:

to:

except asyncio.TimeoutError:
Suggested change
except TimeoutError:
except asyncio.TimeoutError:

Copilot uses AI. Check for mistakes.
"SkillsTool",
"SkillsPlugin",
"SkillsToolset",
"generate_shell_skills_system_prompt",
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The name 'generate_shell_skills_system_prompt' is exported by all but is not defined.

Copilot uses AI. Check for mistakes.
AnthropicInstrumentor(use_legacy_attributes=False).instrument(event_logger_provider=event_logger_provider)
else:
AnthropicInstrumentor().instrument()
except ImportError:
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

'except' clause does nothing but pass and there is no explanatory comment.

Copilot uses AI. Check for mistakes.
from opentelemetry.instrumentation.google_generativeai import GoogleGenerativeAiInstrumentor

GoogleGenerativeAiInstrumentor().instrument()
except ImportError:
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except ImportError:
except ImportError:
# Google GenerativeAI SDK is not installed; skipping instrumentation.

Copilot uses AI. Check for mistakes.
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