forked from phil65/agentpool
-
Notifications
You must be signed in to change notification settings - Fork 9
fix(opencode): expose skills in TUI slash command autocomplete #19
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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 |
|---|---|---|
|
|
@@ -1621,6 +1621,16 @@ async def execute_command( # noqa: PLR0915 | |
| ) | ||
| return await _execute_slashed_command(state, session_id, request) | ||
|
|
||
| # Fallback: check pool.skill_commands directly when CommandStore misses | ||
| # This handles cases where skills were registered after CommandStore init | ||
| # or where the CommandStore sync callback hasn't fired yet | ||
| if state.pool.skill_commands and request.command in state.pool.skill_commands: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback check for cmd_name = request.command.removeprefix("skill:")
if state.pool.skill_commands and cmd_name in state.pool.skill_commands: |
||
| logger.debug( | ||
| "Command '%s' found in skill_commands but not CommandStore, executing as skill", | ||
| request.command, | ||
| ) | ||
| return await _execute_skill_command(state, session_id, request) | ||
|
|
||
| # Fall back to MCP prompts (existing code remains unchanged) | ||
| prompts = await state.agent.tools.list_prompts() | ||
| # Find matching prompt by name | ||
|
|
||
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
199 changes: 199 additions & 0 deletions
199
tests/servers/opencode_server/test_command_model_and_hints.py
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,199 @@ | ||
| """Tests for _extract_hints() and Command model. | ||
|
|
||
| Unit tests for the skill autocomplete fix in AgentPool's OpenCode server. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from agentpool_server.opencode_server.models.agent import Command | ||
| from agentpool_server.opencode_server.routes.agent_routes import _extract_hints | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # _extract_hints() tests | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_extract_hints_no_placeholders() -> None: | ||
| """Template with no placeholders returns empty list.""" | ||
| assert _extract_hints("Just a plain string") == [] | ||
|
|
||
|
|
||
| def test_extract_hints_empty_string() -> None: | ||
| """Empty string returns empty list.""" | ||
| assert _extract_hints("") == [] | ||
|
|
||
|
|
||
| def test_extract_hints_none_input() -> None: | ||
| """None input returns empty list (defensive against provider bugs).""" | ||
| assert _extract_hints(None) == [] | ||
|
|
||
|
|
||
| def test_extract_hints_single_numbered() -> None: | ||
| r"""Single $1 placeholder returns ["$1"].""" | ||
| assert _extract_hints("Analyze $1") == ["$1"] | ||
|
|
||
|
|
||
| def test_extract_hints_multiple_numbered() -> None: | ||
| r"""Multiple numbered placeholders $1 $2 $3 sorted numerically.""" | ||
| assert _extract_hints("Analyze $1 and $2 then $3") == ["$1", "$2", "$3"] | ||
|
|
||
|
|
||
| def test_extract_hints_arguments_placeholder() -> None: | ||
| r"""$ARGUMENTS placeholder returns ["$ARGUMENTS"].""" | ||
| assert _extract_hints("Process $ARGUMENTS") == ["$ARGUMENTS"] | ||
|
|
||
|
|
||
| def test_extract_hints_mixed_numbered_and_arguments() -> None: | ||
| r"""Mix of numbered and $ARGUMENTS: $1 $2 $ARGUMENTS.""" | ||
| assert _extract_hints("Analyze $1 with $2 using $ARGUMENTS") == [ | ||
| "$1", | ||
| "$2", | ||
| "$ARGUMENTS", | ||
| ] | ||
|
|
||
|
|
||
| def test_extract_hints_numeric_sort_not_lexicographic() -> None: | ||
| r"""Numeric sort: $1 $10 $2 → ["$1", "$2", "$10"], NOT lexicographic.""" | ||
| result = _extract_hints("$1 $10 $2") | ||
| assert result == ["$1", "$2", "$10"] | ||
|
|
||
|
|
||
| def test_extract_hints_deduplicates() -> None: | ||
| r"""Duplicate placeholders are deduplicated: $1 and $1 → ["$1"].""" | ||
| assert _extract_hints("$1 and $1") == ["$1"] | ||
|
|
||
|
|
||
| def test_extract_hints_out_of_order_sorted() -> None: | ||
| r"""Out-of-order placeholders are sorted: $3 $1 $2 → ["$1", "$2", "$3"].""" | ||
| assert _extract_hints("$3 $1 $2") == ["$1", "$2", "$3"] | ||
|
|
||
|
|
||
| def test_extract_hints_non_placeholder_dollars() -> None: | ||
| r"""Dollar signs that aren't placeholders ($foo, $NOTANUMBER) are ignored.""" | ||
| assert _extract_hints("$foo $NOTANUMBER") == [] | ||
|
|
||
|
|
||
| def test_extract_hints_lone_dollar() -> None: | ||
| r"""Just $ alone is not a placeholder.""" | ||
| assert _extract_hints("$") == [] | ||
|
|
||
|
|
||
| def test_extract_hints_adjacent_placeholders() -> None: | ||
| r"""Adjacent $1$2 are both extracted.""" | ||
| assert _extract_hints("$1$2") == ["$1", "$2"] | ||
|
|
||
|
|
||
| def test_extract_hints_only_arguments_no_numbered() -> None: | ||
| r"""Template with only $ARGUMENTS and no numbered placeholders.""" | ||
| assert _extract_hints("Run with $ARGUMENTS") == ["$ARGUMENTS"] | ||
|
|
||
|
|
||
| def test_extract_hints_large_numbers() -> None: | ||
| r"""Large numeric placeholders sorted correctly: $100 $2 $1 → ["$1", "$2", "$100"].""" | ||
| assert _extract_hints("$100 $2 $1") == ["$1", "$2", "$100"] | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # Command model tests | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_command_default_construction() -> None: | ||
| """Command with only name uses correct defaults.""" | ||
| cmd = Command(name="test") | ||
| assert cmd.name == "test" | ||
| assert cmd.description is None | ||
| assert cmd.source == "command" | ||
| assert cmd.template == "" | ||
| assert cmd.subtask is False | ||
| assert cmd.hints == [] | ||
| assert cmd.agent is None | ||
| assert cmd.model is None | ||
|
|
||
|
|
||
| def test_command_full_construction() -> None: | ||
| """Command with all fields set.""" | ||
| cmd = Command( | ||
| name="my-skill", | ||
| description="A great skill", | ||
| agent="coder", | ||
| model="openai:gpt-4o", | ||
| source="skill", | ||
| template="Analyze $1 and $ARGUMENTS", | ||
| subtask=True, | ||
| hints=["$1", "$ARGUMENTS"], | ||
| ) | ||
| assert cmd.name == "my-skill" | ||
| assert cmd.description == "A great skill" | ||
| assert cmd.agent == "coder" | ||
| assert cmd.model == "openai:gpt-4o" | ||
| assert cmd.source == "skill" | ||
| assert cmd.template == "Analyze $1 and $ARGUMENTS" | ||
| assert cmd.subtask is True | ||
| assert cmd.hints == ["$1", "$ARGUMENTS"] | ||
|
|
||
|
|
||
| def test_command_model_dump_includes_all_fields() -> None: | ||
| """model_dump() includes all fields (None values included).""" | ||
| cmd = Command(name="test") | ||
| data = cmd.model_dump() | ||
| assert "name" in data | ||
| assert "description" in data | ||
| assert "source" in data | ||
| assert "template" in data | ||
| assert "subtask" in data | ||
| assert "hints" in data | ||
| assert "agent" in data | ||
| assert "model" in data | ||
|
|
||
|
|
||
| def test_command_model_dump_exclude_none() -> None: | ||
| """model_dump(exclude_none=True) omits fields with None value.""" | ||
| cmd = Command(name="test") | ||
| data = cmd.model_dump(exclude_none=True) | ||
| assert "name" in data | ||
| assert "description" not in data | ||
| assert "agent" not in data | ||
| assert "model" not in data | ||
| # Non-None defaults are still present | ||
| assert "source" in data | ||
| assert "template" in data | ||
| assert "subtask" in data | ||
| assert "hints" in data | ||
|
|
||
|
|
||
| def test_command_source_literal_command() -> None: | ||
| """Source accepts 'command' literal.""" | ||
| cmd = Command(name="test", source="command") | ||
| assert cmd.source == "command" | ||
|
|
||
|
|
||
| def test_command_source_literal_mcp() -> None: | ||
| """Source accepts 'mcp' literal.""" | ||
| cmd = Command(name="test", source="mcp") | ||
| assert cmd.source == "mcp" | ||
|
|
||
|
|
||
| def test_command_source_literal_skill() -> None: | ||
| """Source accepts 'skill' literal.""" | ||
| cmd = Command(name="test", source="skill") | ||
| assert cmd.source == "skill" | ||
|
|
||
|
|
||
| def test_command_description_optional() -> None: | ||
| """Description is optional and defaults to None.""" | ||
| cmd = Command(name="test") | ||
| assert cmd.description is None | ||
|
|
||
|
|
||
| def test_command_source_defaults_to_command() -> None: | ||
| """Source defaults to 'command' when not provided.""" | ||
| cmd = Command(name="test") | ||
| assert cmd.source == "command" | ||
|
|
||
|
|
||
| def test_command_source_none_is_valid() -> None: | ||
| """Source can be explicitly set to None.""" | ||
| cmd = Command(name="test", source=None) | ||
| assert cmd.source is None |
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.
The fallback check does not account for the
skill:prefix, which is supported by_execute_skill_command. If a user manually enters a command with the prefix and it hasn't been synced to theCommandStoreyet, this fallback will fail to identify it as a skill command. Normalizing the command name ensures consistency with the execution logic.