-
Notifications
You must be signed in to change notification settings - Fork 101
feat: add to_prompt() for XML skill prompt generation #1483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
neubig
wants to merge
46
commits into
main
Choose a base branch
from
feat/validation-prompt-utils
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+162
−0
Open
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
c246018
feat: add AgentSkills standard fields to Skill model
openhands-agent c033966
chore: remove unused agentskills optional dependency
openhands-agent c7b67b6
feat: support SKILL.md file convention and name validation
openhands-agent 2fb7735
feat: support .mcp.json for MCP server configuration
openhands-agent f24f016
feat: support resource directories (scripts/, references/, assets/)
openhands-agent e8430f0
feat: add validation and prompt generation utilities
openhands-agent 3427e4c
refactor: consolidate AgentSkills tests
openhands-agent fed0593
refactor: consolidate AgentSkills tests
openhands-agent b5e571f
refactor: consolidate AgentSkills tests
openhands-agent bd523d0
Merge main into feat/skill-md-convention
openhands-agent 1d5f15a
refactor: modularize load_skills_from_dir function
openhands-agent c661e2f
Resolve merge conflicts with main branch
openhands-agent b33afe8
Merge main into feat/skill-md-convention
openhands-agent 1d2c579
refactor: address code review feedback for SKILL.md convention
openhands-agent 5649c81
Merge branch 'main' into feat/skill-md-convention
neubig f1dc623
fix: AgentSkills (SKILL.md) always use progressive loading
openhands-agent feb7a2d
Update organization
neubig e86afb8
Remove file_content parameter from Skill.load()
neubig a53f3a0
Refactor Skill.load() into separate code paths for AgentSkills and le…
neubig 821f5b5
Merge remote-tracking branch 'origin/main' into feat/skill-md-convention
openhands-agent 4f85894
feat: Add agent_skills as third category for AgentSkills standard
openhands-agent e22f6c5
feat: support .mcp.json for MCP server configuration (agent_skills only)
openhands-agent 5a541f3
Merge branch 'main' into feat/resource-directories
openhands-agent 8565075
Resolve merge conflicts with main branch
openhands-agent bfc17a7
Merge main into feat/mcp-json-support, preserving .mcp.json support
openhands-agent bba56fa
fix: remove duplicate path.parent.name assignment
openhands-agent 99759d7
Merge origin/feat/mcp-json-support into feat/resource-directories
openhands-agent c633cd7
Merge branch 'main' into feat/validation-prompt-utils
neubig e34051e
Merge branch 'main' into feat/resource-directories
neubig e0963ec
Merge branch 'main' into feat/mcp-json-support
neubig 3f77eee
Merge branch 'main' into feat/mcp-json-support
neubig b13da52
Merge latest from feat/mcp-json-support into feat/resource-directories
openhands-agent de37286
Merge remote changes from feat/resource-directories
openhands-agent 0c5179b
refactor: move utility functions to utils.py
openhands-agent b9f83af
Merge feat/mcp-json-support into feat/resource-directories
openhands-agent e2be31c
refactor: remove non-resource-directory changes from PR
openhands-agent 85d0ea3
refactor: move discover_skill_resources and RESOURCE_DIRECTORIES to u…
openhands-agent 1fdc57b
Merge origin/main into feat/resource-directories
openhands-agent 317b258
Merge origin/feat/resource-directories into feat/validation-prompt-utils
openhands-agent d6deeff
refactor: remove validate_skill() in favor of Pydantic validation
openhands-agent 68667e9
refactor: keep only to_prompt() in this PR
openhands-agent 2b4103d
Merge origin/main into feat/validation-prompt-utils
openhands-agent d18349f
refactor: improve to_prompt() based on review feedback
openhands-agent 3fd1b53
refactor: use xml.sax.saxutils.escape and count remaining content as …
openhands-agent 3a223ac
refactor: inline _escape_xml function
openhands-agent 919093a
Merge branch 'main' into feat/validation-prompt-utils
neubig File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| """Tests for prompt generation utilities (Issue #1478).""" | ||
|
|
||
| from openhands.sdk.context.skills import ( | ||
| Skill, | ||
| to_prompt, | ||
| ) | ||
|
|
||
|
|
||
| def test_to_prompt_generates_xml() -> None: | ||
| """to_prompt() should generate valid XML for skills.""" | ||
| # Empty list shows "no available skills" | ||
| assert ( | ||
| to_prompt([]) | ||
| == "<available_skills>\n no available skills\n</available_skills>" | ||
| ) | ||
|
|
||
| # Single skill with description | ||
| skill = Skill(name="pdf-tools", content="# PDF", description="Process PDFs.") | ||
| result = to_prompt([skill]) | ||
| assert '<skill name="pdf-tools">' in result | ||
| assert "Process PDFs." in result | ||
| assert "<available_skills>" in result | ||
|
|
||
| # Multiple skills | ||
| skills = [ | ||
| Skill(name="pdf-tools", content="# PDF", description="Process PDFs."), | ||
| Skill(name="code-review", content="# Code", description="Review code."), | ||
| ] | ||
| result = to_prompt(skills) | ||
| assert result.count("<skill") == 2 | ||
|
|
||
|
|
||
| def test_to_prompt_escapes_xml() -> None: | ||
| """to_prompt() should escape XML special characters.""" | ||
| skill = Skill( | ||
| name="test", content="# Test", description='Handle <tags> & "quotes".' | ||
| ) | ||
| result = to_prompt([skill]) | ||
| assert "<tags>" in result | ||
| assert "&" in result | ||
| assert ""quotes"" in result | ||
|
|
||
|
|
||
| def test_to_prompt_uses_content_fallback() -> None: | ||
| """to_prompt() should use content when no description.""" | ||
| skill = Skill(name="test", content="# Header\n\nActual content here.") | ||
| result = to_prompt([skill]) | ||
| assert "Actual content here." in result | ||
| assert "# Header" not in result | ||
|
|
||
|
|
||
| def test_to_prompt_content_fallback_counts_remaining_as_truncated() -> None: | ||
| """to_prompt() should count content after first line as truncated.""" | ||
| # Content with header, description line, and additional content | ||
| content = "# Header\n\nFirst line used as description.\n\nMore content here." | ||
| skill = Skill(name="test", content=content, source="/skills/test.md") | ||
| result = to_prompt([skill]) | ||
|
|
||
| # Should use first non-header line as description | ||
| assert "First line used as description." in result | ||
| # Should indicate truncation for remaining content | ||
| assert "characters truncated" in result | ||
| assert "View /skills/test.md for complete information" in result | ||
|
|
||
|
|
||
| def test_to_prompt_truncates_long_descriptions() -> None: | ||
| """to_prompt() should truncate long descriptions with indicator.""" | ||
| long_desc = "A" * 250 # 250 characters | ||
| skill = Skill(name="test", content="# Test", description=long_desc) | ||
| result = to_prompt([skill]) | ||
|
|
||
| # Should contain truncation indicator | ||
| assert "... [50 characters truncated]" in result | ||
| # Should contain first 200 chars | ||
| assert "A" * 200 in result | ||
|
|
||
|
|
||
| def test_to_prompt_truncation_includes_source() -> None: | ||
| """to_prompt() should include source path in truncation message.""" | ||
| long_desc = "B" * 250 | ||
| skill = Skill( | ||
| name="test", | ||
| content="# Test", | ||
| description=long_desc, | ||
| source="/path/to/skill.md", | ||
| ) | ||
| result = to_prompt([skill]) | ||
|
|
||
| assert "... [50 characters truncated" in result | ||
| assert "View /path/to/skill.md for complete information]" in result |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands /codereview /codereview-roasted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I provided a Linus Torvalds-style code review for PR #1483 (
feat: add to_prompt() for XML skill prompt generation) as requested with the/codereview-roastedcommand.Review Verdict: 🟡 Acceptable - Worth merging
Key Findings:
Core logic is sound - The XML generation works correctly and uses the standard library (
xml.sax.saxutils.escape) for escaping, which is good.Main improvement opportunity (Lines 809-824): The manual character position tracking for content fallback is fragile and over-engineered. The code tracks
chars_before_desc,desc_end_pos, andcontent_truncatedthrough manual byte counting - this is error-prone and could be simplified to just detect "has more content" rather than counting exact characters.Over-engineering concern (Lines 827-840): The exact truncation character counts (e.g., "47 characters truncated") solve an imaginary problem. Users don't care about exact counts - they just need to know there's more content and where to find it.
Suggested simplification: Replace the character-counting logic with a simpler "are there more meaningful lines?" check, and simplify the truncation message to something like
"... [see /skills/test.md for full content]".No code changes were made - this was a review-only request. The feedback has been provided above for the PR author to consider.
View full conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xingyaoww Would you like to make this change? Or keep as is? I kinda feel as-is might be OK