diff --git a/docs/features/skill-commands.md b/docs/features/skill-commands.md index 9ba2c28f4..e7e95d2fb 100644 --- a/docs/features/skill-commands.md +++ b/docs/features/skill-commands.md @@ -37,19 +37,25 @@ Detailed instructions for the agent... ## Protocol-Specific Usage ### ACP Protocol -Skills appear as AvailableCommand in AgentCapabilities: +Skills appear as `AvailableCommand` via the `session/update` notification with `available_commands_update` after session creation: ```json { - "slash_commands": [ - { - "name": "my-skill", - "description": "A description...", - "input": {"hint": "Arguments for skill"} - } - ] + "sessionId": "sess_abc123", + "update": { + "sessionUpdate": "available_commands_update", + "availableCommands": [ + { + "name": "my-skill", + "description": "A description...", + "input": {"hint": "Arguments for skill"} + } + ] + } } ``` +> **Note**: Per the ACP specification, available commands are declared via `session/update` after session creation, not in the `initialize` response. + ### AG-UI Protocol Skills appear as Tools with `skill__` prefix: ```json diff --git a/docs/rfcs/draft/RFC-0032-acp-slash-commands-session-update.md b/docs/rfcs/draft/RFC-0032-acp-slash-commands-session-update.md new file mode 100644 index 000000000..ee8bfc67d --- /dev/null +++ b/docs/rfcs/draft/RFC-0032-acp-slash-commands-session-update.md @@ -0,0 +1,616 @@ +--- +rfc_id: RFC-0032 +title: "ACP Slash Commands Protocol Compliance: Move from initialize to session/update" +status: DRAFT +author: yuchen.liu +reviewers: + - name: Oracle + status: completed + - name: Metis + status: completed +created: 2026-05-26 +last_updated: 2026-05-26 +decision_date: +related_rfcs: + - RFC-0016 (Skill Slash Commands) + - RFC-0031 (ACP Server Per-Session Agent Isolation) +--- + +# RFC-0032: ACP Slash Commands Protocol Compliance + +## Overview + +This RFC proposes aligning AgentPool's ACP slash command advertisement with the official Agent Client Protocol (ACP) specification. Currently, AgentPool declares available slash commands during the `initialize` handshake via `AgentCapabilities.slash_commands`. The ACP specification mandates that slash commands be advertised **after** session creation through the `session/update` notification with `available_commands_update`. This RFC outlines the migration path to remove `slash_commands` from `AgentCapabilities` and rely exclusively on the per-session `session/update` mechanism — which AgentPool already partially implements. + +## Table of Contents + +- [Background & Context](#background--context) +- [Problem Statement](#problem-statement) +- [Goals & Non-Goals](#goals--non-goals) +- [Evaluation Criteria](#evaluation-criteria) +- [Options Analysis](#options-analysis) +- [Recommendation](#recommendation) +- [Technical Design](#technical-design) +- [Review Findings](#review-findings) +- [Implementation Plan](#implementation-plan) +- [Open Questions](#open-questions) +- [Decision Record](#decision-record) +- [References](#references) + +--- + +## Background & Context + +### Current State + +AgentPool's ACP server (`AgentPoolACPAgent`) advertises slash commands in two places: + +1. **`initialize` response** (`acp_agent.py:482-505`): The `initialize()` method builds `InitializeResponse` with `slash_commands=skill_commands`, populating `AgentCapabilities.slash_commands` in the JSON-RPC response. +2. **`session/update` notification** (`session.py:562-572`): The `ACPSession.send_available_commands_update()` method sends `AvailableCommandsUpdate` via `ACPNotifications.update_commands()` after session creation. + +The schema layer supports both paths: +- `AgentCapabilities.slash_commands: list[AvailableCommand]` (`capabilities.py:273`) +- `InitializeResponse.create(slash_commands=...)` (`agent_responses.py:284-339`) +- `AvailableCommandsUpdate` (`session_updates.py:354-363`) + +**Current `initialize` response path** (`acp_agent.py:490-505`): +```python +skill_commands = self.get_skill_commands() +return InitializeResponse.create( + protocol_version=version, + name="agentpool", + title="AgentPool", + version=_version("agentpool"), + # ... other capabilities ... + slash_commands=skill_commands, # ← ADVERTISED AT INIT TIME +) +``` + +**Current `session/update` path** (`acp_agent.py:547-550`, `621`, `732`): +```python +# After new_session / load_session / resume_session +self.tasks.create_task(session.send_available_commands_update()) +``` + +The `session/update` path is already invoked after `new_session()`, `load_session()`, and `resume_session()`, meaning AgentPool currently sends slash commands **twice**: once globally at initialization, and once per session. + +### ACP Protocol Specification + +The official ACP specification (`agent-client-protocol/docs/protocol/slash-commands.mdx`) states: + +> After creating a session, the Agent **MAY** send a list of available commands via the `available_commands_update` session notification. + +The spec provides this example: +```json +{ + "jsonrpc": "2.0", + "method": "session/update", + "params": { + "sessionId": "sess_abc123def456", + "update": { + "sessionUpdate": "available_commands_update", + "availableCommands": [...] + } + } +} +``` + +Key protocol requirements: +- Commands are **per-session**, not global +- Commands can be **dynamically updated** during a session +- Commands are advertised via `session/update`, not `initialize` + +### Glossary + +| Term | Definition | +|------|------------| +| `AgentCapabilities` | ACP schema object advertised in `initialize` response | +| `AvailableCommandsUpdate` | ACP session update type for advertising slash commands | +| `session/update` | ACP JSON-RPC notification method for session state changes | +| `initialize` | ACP JSON-RPC method for capability negotiation | +| `ACPSkillBridge` | AgentPool component that exposes skill commands as slash commands | + +--- + +## Problem Statement + +### The Problem + +AgentPool's current implementation violates the ACP protocol specification for slash command advertisement: + +1. **Wrong lifecycle phase**: Commands are advertised during `initialize` (global/static) rather than after `session/new` (per-session/dynamic). +2. **Double advertisement**: Commands are sent both at initialization and per-session, causing redundant protocol traffic. +3. **Schema drift**: `AgentCapabilities.slash_commands` is not part of the official ACP spec for the `initialize` response. While the field exists in AgentPool's schema, it has no equivalent in the protocol's `AgentCapabilities` definition. +4. **Per-session semantics lost**: By advertising at initialization, AgentPool implies commands are global and static. In reality, commands vary per session based on: agent type, loaded skills, MCP server prompts, prompt hub commands, and session-specific context. + +### Evidence + +- `capabilities.py:273`: `slash_commands: list[AvailableCommand]` defined on `AgentCapabilities` +- `agent_responses.py:301`: `InitializeResponse.create()` accepts `slash_commands` parameter +- `acp_agent.py:504`: `slash_commands=skill_commands` passed to `InitializeResponse.create()` +- `slash-commands.mdx:10`: "After creating a session, the Agent MAY send a list of available commands via the `available_commands_update` session notification" +- `slash-commands.mdx:71-73`: "The Agent can update the list of available commands at any time during a session by sending another `available_commands_update` notification" + +### Impact of Inaction + +- **Risk**: ACP-compliant clients may ignore `initialize`-time `slash_commands` entirely, causing skill commands to be invisible until a session is created — but since some clients rely on the spec-compliant `session/update` path, they will work. +- **Risk**: Non-compliant clients that only look at `initialize`-time `slash_commands` will break when AgentPool eventually aligns with the spec. +- **Risk**: Protocol divergence makes AgentPool harder to integrate with third-party ACP clients that strictly follow the specification. +- **Cost**: Maintaining the dual-path approach increases schema and test complexity. + +--- + +## Goals & Non-Goals + +### Goals (In Scope) + +1. Remove `slash_commands` from `AgentCapabilities` schema and `InitializeResponse` +2. Ensure all slash command advertisement flows through `session/update` (`AvailableCommandsUpdate`) +3. Maintain backward compatibility for existing ACP clients during a deprecation window +4. Update all tests that assert on `initialize`-time `slash_commands` +5. Update documentation to reflect the protocol-compliant behavior + +### Non-Goals (Out of Scope) + +1. **Not**: Changing the content or semantics of available commands themselves +2. **Not**: Adding new command types or command registration mechanisms +3. **Not**: Refactoring `ACPSkillBridge` or skill command discovery logic +4. **Not**: Modifying how commands are executed (`process_prompt` / `execute_slash_command`) +5. **Not**: Changing the ACP protocol spec — this RFC aligns AgentPool with the existing spec + +### Success Criteria + +- [ ] `initialize` response no longer contains `slash_commands` in `agent_capabilities` +- [ ] `AgentCapabilities` schema no longer has a `slash_commands` field +- [ ] All existing tests pass after updating assertions +- [ ] `session/send_available_commands_update()` continues to work after session creation +- [ ] ACP clients receive commands via `session/update` as per spec +- [ ] No regression in skill command visibility for supported clients + +--- + +## Evaluation Criteria + +| Criterion | Weight | Description | Minimum Threshold | +|-----------|--------|-------------|-------------------| +| Protocol Compliance | Critical | Aligns with ACP spec `slash-commands.mdx` | Must remove `slash_commands` from `initialize` | +| Backward Compatibility | High | Existing clients continue to work | No client-visible regressions for 1 release cycle | +| Minimality | High | Smallest change that achieves compliance | ≤ 6 files modified | +| Test Coverage | High | All affected tests updated | 100% of `slash_commands`-related tests updated | +| Documentation | Medium | Docs reflect new behavior | At least 1 doc page updated | + +--- + +## Options Analysis + +### Option 1: Complete Removal from `initialize` (Recommended) + +Remove `slash_commands` entirely from `AgentCapabilities`, `InitializeResponse.create()`, and `AgentPoolACPAgent.initialize()`. Rely solely on the existing `session/update` (`AvailableCommandsUpdate`) path that is already invoked after `new_session`, `load_session`, and `resume_session`. + +**Advantages**: +- Full protocol compliance — aligns exactly with `slash-commands.mdx` +- Eliminates double advertisement (reduced protocol traffic) +- Correct per-session semantics — commands are scoped to the session +- Minimal code change — the `session/update` path already exists and works +- Removes schema drift between AgentPool and the ACP specification + +**Disadvantages**: +- Clients that relied solely on `initialize`-time `slash_commands` will no longer see commands until session creation +- Requires updating test assertions in `test_capabilities.py` and integration tests +- May require a brief deprecation window if external consumers depend on the field + +**Evaluation Against Criteria**: + +| Criterion | Rating | Notes | +|-----------|--------|-------| +| Protocol Compliance | ✅ Excellent | Fully aligns with ACP spec | +| Backward Compatibility | ⚠️ Moderate | Clients must support `session/update`; most already do | +| Minimality | ✅ Excellent | ~5 files, removes code rather than adding | +| Test Coverage | ✅ Good | Update existing tests, no new test infrastructure needed | +| Documentation | ✅ Good | Update existing docs to remove `initialize` references | + +**Effort Estimate**: +- Complexity: Low +- Resources: 1 engineer, 0.5–1 day +- Dependencies: None + +**Risk Assessment**: + +| Risk | Likelihood | Impact | Mitigation | +|------|------------|--------|------------| +| Client incompatibility | Low | Medium | Major clients (Zed, Toad) already handle `session/update`; verify during testing | +| Test regressions | Low | Low | Tests are mechanical assertion updates | +| External consumer breakage | Low | Medium | `AgentCapabilities` is internal to AgentPool; external clients parse JSON | + +--- + +### Option 2: Deprecation with Fallback + +Keep `AgentCapabilities.slash_commands` but set it to an empty list in `initialize()`. Continue sending actual commands via `session/update`. Add a deprecation comment/note indicating the field will be removed in a future release. + +**Advantages**: +- Maximum backward compatibility — field still exists in schema +- Zero risk of breaking external consumers that depend on the field +- Gradual migration path for any non-compliant clients + +**Disadvantages**: +- Not protocol compliant — the field should not exist in `AgentCapabilities` at all +- Perpetuates schema drift from the ACP specification +- Increases technical debt — the field becomes dead code with no spec backing +- Confusing for new developers — "why does this field exist if it's always empty?" + +**Evaluation Against Criteria**: + +| Criterion | Rating | Notes | +|-----------|--------|-------| +| Protocol Compliance | ❌ Poor | Field still exists, contradicting spec | +| Backward Compatibility | ✅ Excellent | Zero breaking changes | +| Minimality | ⚠️ Moderate | Must keep and maintain dead code | +| Test Coverage | ✅ Good | Existing tests pass with minor modifications | +| Documentation | ❌ Poor | Must document a deprecated, non-spec field | + +**Effort Estimate**: +- Complexity: Low +- Resources: 1 engineer, 0.5 day +- Dependencies: None + +**Risk Assessment**: + +| Risk | Likelihood | Impact | Mitigation | +|------|------------|--------|------------| +| Technical debt accumulation | High | Medium | Schedule removal in next release | +| Developer confusion | Medium | Low | Add prominent deprecation comments | + +--- + +### Option 3: Keep Both Paths with Client Detection + +Retain `initialize`-time `slash_commands` and add client capability detection: only send `slash_commands` in `initialize` if the client advertises that it does not support `session/update` notifications. Otherwise, rely on `session/update`. + +**Advantages**: +- Backward compatible for all clients +- Spec-compliant for modern clients +- Graceful degradation for legacy clients + +**Disadvantages**: +- Over-engineered — no known client lacks `session/update` support +- Adds complexity to `initialize()` logic +- No ACP capability flag exists for "does not support session/update" — would require inventing one +- Perpetuates the schema drift problem + +**Evaluation Against Criteria**: + +| Criterion | Rating | Notes | +|-----------|--------|-------| +| Protocol Compliance | ⚠️ Moderate | Still sends non-spec field conditionally | +| Backward Compatibility | ✅ Excellent | Supports all clients | +| Minimality | ❌ Poor | Adds conditional logic and invented capability flags | +| Test Coverage | ❌ Poor | Requires testing both code paths | +| Documentation | ❌ Poor | Must document invented capability protocol | + +**Effort Estimate**: +- Complexity: Medium +- Resources: 1 engineer, 2–3 days +- Dependencies: None + +--- + +### Options Comparison Summary + +| Criterion | Option 1: Complete Removal | Option 2: Deprecation | Option 3: Client Detection | +|-----------|---------------------------|----------------------|---------------------------| +| Protocol Compliance | ✅ Full | ❌ Partial | ⚠️ Conditional | +| Backward Compatibility | ⚠️ One release window | ✅ Perfect | ✅ Perfect | +| Minimality | ✅ ~5 files | ⚠️ Dead code | ❌ Over-engineered | +| Test Coverage | ✅ Straightforward | ✅ Straightforward | ❌ Complex | +| Maintenance Burden | ✅ Low | ⚠️ Medium | ❌ High | +| **Overall** | **Recommended** | Rejected | Rejected | + +--- + +## Recommendation + +**Option 1: Complete Removal from `initialize`.** + +The `session/update` path for slash commands is already fully implemented and tested. Removing the `initialize`-time path is a net reduction in code and aligns AgentPool with the ACP specification. The risk of client breakage is low because: + +1. The `session/update` notification is a baseline ACP requirement — all compliant clients must support it +2. AgentPool already sends commands via `session/update` after every session creation +3. No known ACP client relies solely on `initialize`-time `slash_commands` for command discovery + +### Accepted Trade-offs + +1. **Brief deprecation window**: The `slash_commands` field will be removed in the next minor release. No formal deprecation cycle is needed because the field is not part of the public ACP spec. +2. **Client adaptation**: Any client that only reads commands from `initialize` will need to adapt. This is considered acceptable because such a client would already be non-compliant with the ACP specification. + +### Conditions + +- All tests must pass after the change +- `session/send_available_commands_update()` must be verified to work correctly in integration tests +- Documentation must be updated before marking RFC as COMPLETED + +--- + +## Technical Design + +### Architecture Overview + +``` +BEFORE (Current — Non-Compliant): +┌─────────────────┐ ┌─────────────────────────────┐ +│ Client │────▶│ initialize │ +│ │ │ └─ AgentCapabilities │ +│ │◀────│ └─ slash_commands[] │ ← WRONG PHASE +└─────────────────┘ └─────────────────────────────┘ + │ + │ session/new + ▼ +┌─────────────────────────────┐ +│ session/update │ +│ └─ available_commands_update│ ← CORRECT (already done) +│ └─ availableCommands[] │ +└─────────────────────────────┘ + +AFTER (Protocol-Compliant): +┌─────────────────┐ ┌─────────────────────────────┐ +│ Client │────▶│ initialize │ +│ │ │ └─ AgentCapabilities │ +│ │◀────│ (no slash_commands) │ ← REMOVED +└─────────────────┘ └─────────────────────────────┘ + │ + │ session/new + ▼ +┌─────────────────────────────┐ +│ session/update │ +│ └─ available_commands_update│ ← SOLE PATH +│ └─ availableCommands[] │ +└─────────────────────────────┘ +``` + +### Key Components + +#### 1. `AgentCapabilities` Schema Change + +**File**: `src/acp/schema/capabilities.py` + +**Remove**: +```python +slash_commands: list[AvailableCommand] = Field(default_factory=list) +``` + +**Update `AgentCapabilities.create()`**: Remove `slash_commands` parameter and its usage in the method body. + +#### 2. `InitializeResponse` Schema Change + +**File**: `src/acp/schema/agent_responses.py` + +**Update `InitializeResponse.create()`**: Remove `slash_commands` parameter and its forwarding to `AgentCapabilities.create()`. + +#### 3. `AgentPoolACPAgent.initialize()` Update + +**File**: `src/agentpool_server/acp_server/acp_agent.py` + +**Remove**: +```python +skill_commands = self.get_skill_commands() +# ... +slash_commands=skill_commands, +``` + +The `get_skill_commands()` method on `AgentPoolACPAgent` is currently used **only** by `initialize()`. After removal, it becomes dead code and should be evaluated for deletion. Note: `ACPSession.send_available_commands_update()` does **not** call `get_skill_commands()` — it calls `self.get_acp_commands()` which operates on the session's `command_store` directly (`session.py:614-628`). + +#### 4. Test Updates + +**File**: `tests/acp/schema/test_capabilities.py` + +- Remove or repurpose all `TestAgentCapabilitiesSlashCommands` test methods +- Add tests verifying that `AgentCapabilities` does **not** contain `slash_commands` after deserialization from old JSON (backward compat) + +**File**: `tests/servers/acp_server/test_acp_skill_commands.py` + +- **Critical**: Tests in this file (e.g., `test_initialize_exposes_skill_commands`, `test_initialize_without_skills_has_empty_commands`) are **fundamentally testing removed behavior**. These must be **rewritten or deleted**, not merely updated with new assertions. +- Replace with tests verifying that `initialize()` returns `AgentCapabilities` without `slash_commands`, and that commands are received via `session/update` notification after session creation. + +**Files**: `tests/server/acp/test_skill_commands.py`, `tests/integration/test_skill_commands_e2e.py` + +- Update any tests that assert on `initialize`-time `slash_commands` +- Add assertions verifying commands arrive via `session/update` instead +- Verify e2e/integration tests do not have hidden assertions on the removed field + +#### 5. Documentation Updates + +**File**: `docs/features/skill-commands.md` + +- Remove any references to `initialize`-time command advertisement +- Clarify that commands are advertised per-session via `session/update` + +--- + +## Implementation Plan + +### Phase 1: Schema and Agent Layer Changes + +**Scope**: Remove `slash_commands` from schema and `initialize()` + +**Files**: +| File | Changes | +|------|---------| +| `src/acp/schema/capabilities.py` | Remove `slash_commands` field from `AgentCapabilities`; update `create()` | +| `src/acp/schema/agent_responses.py` | Remove `slash_commands` parameter from `InitializeResponse.create()` | +| `src/agentpool_server/acp_server/acp_agent.py` | Remove `slash_commands=skill_commands` from `initialize()` | + +**Duration**: 0.5 day + +### Phase 2: Test Updates + +**Scope**: Rewrite/delete tests that assert on `initialize`-time `slash_commands`; add tests for `session/update` path + +**Files**: +| File | Changes | +|------|---------| +| `tests/acp/schema/test_capabilities.py` | Remove `TestAgentCapabilitiesSlashCommands`; add backward-compat deserialization test | +| `tests/servers/acp_server/test_acp_skill_commands.py` | **Rewrite**: replace `initialize`-time tests with `session/update` path tests | +| `tests/server/acp/test_skill_commands.py` | Update assertions | +| `tests/integration/test_skill_commands_e2e.py` | Update assertions; verify `session/update` path end-to-end | + +**Duration**: 0.5–1 day + +### Phase 3: Documentation and Validation + +**Scope**: Update docs and run full test suite + +**Files**: +| File | Changes | +|------|---------| +| `docs/features/skill-commands.md` | Remove `initialize` references; clarify `session/update` path | + +**Validation**: +- `pytest tests/acp/schema/` +- `pytest tests/server/acp/` +- `pytest tests/integration/test_skill_commands_e2e.py` +- `pytest tests/servers/acp_server/test_acp_skill_commands.py` + +**Duration**: 0.5 day + +### Rollback Strategy + +Revert by restoring: +1. `slash_commands` field in `AgentCapabilities` +2. `slash_commands` parameter in `InitializeResponse.create()` +3. `slash_commands=skill_commands` in `AgentPoolACPAgent.initialize()` +4. Original test assertions + +--- + +## Review Findings + +### Metis Review (2026-05-26) + +**Ambiguities and AI Failure Points Identified**: + +1. **Backward compatibility contradiction** (Addressed): The RFC originally claimed backward compatibility as a goal while rejecting the only backward-compatible option (deprecation). The "Accepted Trade-offs" section has been updated to clarify that no formal deprecation cycle is needed because the field is not part of the public ACP spec, but implementers should verify no external consumers depend on it. + +2. **`get_skill_commands()` usage analysis error** (Addressed): The original RFC incorrectly stated that `get_skill_commands()` is used by `send_available_commands_update()`. Code inspection shows `send_available_commands_update()` calls `self.get_acp_commands()` (session-level) instead. `get_skill_commands()` is only used by `initialize()` and becomes dead code after removal. The Technical Design section has been corrected. + +3. **Test scope underestimated** (Addressed): The original RFC described test changes as "assertion updates." In reality, `tests/servers/acp_server/test_acp_skill_commands.py` contains tests whose entire premise is `initialize`-time command exposure — these must be rewritten or deleted, not patched. The Implementation Plan now explicitly calls out test rewriting. + +4. **Race condition: `session/update` timing** (Documented): `send_available_commands_update()` is scheduled as a background task (`self.tasks.create_task()`) after the `session/new` response is returned. There is no ordering guarantee between the response and the notification. A fast client could query for commands before the async task runs. The current behavior is accepted as-is because: + - The gap is typically one network round-trip + - All compliant ACP clients must support `session/update` + - Commands are per-session by design — there is no valid use case for commands before session creation + +5. **Missing edge cases** (Added to Open Questions): + - Session creation failure after successful `initialize`: client gets zero commands (acceptable per spec) + - Empty command lists: `send_available_commands_update()` sends `availableCommands: []` — this is spec-compliant + - Mid-session command updates: Already supported via `_register_mcp_prompts_as_commands()` and `_register_prompt_hub_commands()` + +### Oracle Review (2026-05-26) + +**Technical Assessment**: + +1. **Recommended approach is correct** (Confirmed): The `session/update` path is already fully implemented and robust. Removing the `initialize`-time path is a net code reduction with zero new infrastructure needed. + +2. **Schema safety verified** (Confirmed): `AgentCapabilities` inherits from `AnnotatedObject` → Pydantic `BaseModel`. Pydantic v2 default is `extra='ignore'`, so old JSON with `slash_commands` will deserialize safely. However, implementers should add an explicit backward-compat test. + +3. **Client impact: Low** (Confirmed): `src/acp/client/` has zero references to `slash_commands`. Major ACP clients (Zed, Toad) strictly follow the spec and already handle `session/update`. The risk of breaking real clients is low. + +4. **Missing: Deprecation warning phase** (Recommendation): Oracle recommends a hybrid approach: + - **Phase 1** (this release): Set `slash_commands=[]` in `initialize()`, keep field in schema, emit `DeprecationWarning` + - **Phase 2** (next minor release): Remove field entirely + This costs ~1 line (`warnings.warn(...)`) and provides measurable safety. The RFC author has considered this and decided on hard removal due to the field being non-spec, but acknowledges the risk. + +5. **`InitializeResponse.create()` docstring bug** (Drive-by): The docstring incorrectly says "Create an instance of AgentCapabilities" — it creates an `InitializeResponse`. This pre-existing bug should be fixed as a drive-by. + +6. **Dynamic command updates: Verified** (Resolved): `session.py:582` calls `send_available_commands_update()` after `_register_mcp_prompts_as_commands()`. `session.py:268-272` handles nested ACP agent command updates. This is already complete. + +7. **Criteria weighting: Appropriate** (Confirmed): Protocol Compliance (Critical), Backward Compatibility (High), Minimality (High), Test Coverage (High), Documentation (Medium) are correctly weighted. + +### Oracle + Metis Consensus + +- **Core recommendation (Option 1) is sound** and should proceed +- **Test rewriting is required**, not just assertion updates +- **`get_skill_commands()` should be evaluated for deletion** after removal +- **Add backward-compat deserialization test** as a blocking condition +- **Verify `extra='ignore'` on `AnnotatedObject`** before merge +- **Close Open Question #3** — dynamic updates are already handled + +--- + +## Open Questions + +1. **External client dependency on `initialize`-time `slash_commands`** + - Context: Are there any external ACP clients (outside AgentPool's test suite) that read commands from `initialize`? + - Owner: yuchen.liu + - Status: Open — investigate before merging + +2. **`AgentCapabilities` backward compatibility** + - Context: Old JSON with `slash_commands` key must still deserialize without errors after removing the field. `AnnotatedObject` inherits from Pydantic `BaseModel`; Pydantic v2 default is `extra='ignore'`, so unknown fields are dropped safely. This should still be verified with an explicit test. + - Owner: Implementer + - Status: **RESOLVED in design** — add backward-compat deserialization test as blocking condition; verify `extra='ignore'` on `AnnotatedObject` + +3. **Dynamic command updates during session** + - Context: The ACP spec allows commands to be updated at any time. Code inspection confirms AgentPool already supports this: `session.py:582` calls `send_available_commands_update()` after `_register_mcp_prompts_as_commands()`, and `session.py:268-272` handles nested ACP agent command updates. + - Owner: Implementer + - Status: **RESOLVED** — verified existing implementation covers dynamic updates + +4. **Client verification** + - Context: Has any production ACP client been verified to work without `initialize`-time `slash_commands`? The ACP baseline spec (`capabilities.py:207-208`) states all agents MUST support `session/update`, so compliant clients are expected to handle it. + - Owner: yuchen.liu + - Status: Open — verify with Zed/Toad before merge + +5. **`get_skill_commands()` dead code** + - Context: After removing `slash_commands` from `initialize()`, `AgentPoolACPAgent.get_skill_commands()` has no remaining callers. It should be evaluated for deletion or retained if future features need it. + - Owner: Implementer + - Status: Open — decide during implementation + +--- + +## Decision Record + +> To be completed after RFC review. + +### Decision + +**Status**: PENDING REVIEW + +**Date**: + +**Approvers**: +- [Name 1] +- [Name 2] + +### Decision Summary + +[To be filled after review] + +### Key Discussion Points + +1. [Point 1] +2. [Point 2] + +### Conditions of Approval + +[To be filled after review] + +--- + +## References + +### Related Documents + +- [ACP Slash Commands Protocol Spec](../../agent-client-protocol/docs/protocol/slash-commands.mdx) +- [RFC-0016: Skill Slash Commands](./draft/RFC-0016-skill-slash-commands.md) +- [RFC-0031: ACP Server Per-Session Agent Isolation](./RFC-0031-acp-per-session-agent-isolation.md) + +### Code References + +- `src/acp/schema/capabilities.py:273` — `AgentCapabilities.slash_commands` +- `src/acp/schema/agent_responses.py:284-339` — `InitializeResponse.create()` +- `src/agentpool_server/acp_server/acp_agent.py:482-505` — `AgentPoolACPAgent.initialize()` +- `src/agentpool_server/acp_server/session.py:562-572` — `ACPSession.send_available_commands_update()` +- `src/acp/agent/notifications.py:336-339` — `ACPNotifications.update_commands()` +- `src/acp/schema/session_updates.py:354-363` — `AvailableCommandsUpdate` + +### External Resources + +- [Agent Client Protocol — Slash Commands](https://agentclientprotocol.com/protocol/slash-commands) diff --git a/src/acp/schema/agent_responses.py b/src/acp/schema/agent_responses.py index bd88f90ad..a3f6ba6be 100644 --- a/src/acp/schema/agent_responses.py +++ b/src/acp/schema/agent_responses.py @@ -19,7 +19,6 @@ if TYPE_CHECKING: from acp.schema import ModelInfo, SessionMode - from acp.schema.slash_commands import AvailableCommand StopReason = Literal[ @@ -298,9 +297,8 @@ def create( resume_session: bool = False, stop_session: bool = False, auth_methods: Sequence[AuthMethod] | None = None, - slash_commands: Sequence[AvailableCommand] | None = None, ) -> Self: - """Create an instance of AgentCapabilities. + """Create an instance of InitializeResponse. Args: name: The name of the agent. @@ -317,7 +315,6 @@ def create( resume_session: Whether the agent supports `session/resume` (unstable). stop_session: Whether the agent supports `session/stop` (unstable). auth_methods: The authentication methods supported by the agent. - slash_commands: Available slash commands exposed by the agent. """ caps = AgentCapabilities.create( load_session=load_session, @@ -329,7 +326,6 @@ def create( list_sessions=list_sessions, resume_session=resume_session, stop_session=stop_session, - slash_commands=list(slash_commands) if slash_commands else None, ) return cls( agent_info=Implementation(name=name, title=title, version=version), diff --git a/src/acp/schema/capabilities.py b/src/acp/schema/capabilities.py index 3d16a37ef..89df1618b 100644 --- a/src/acp/schema/capabilities.py +++ b/src/acp/schema/capabilities.py @@ -7,7 +7,6 @@ from pydantic import Field, field_validator from acp.schema.base import AnnotatedObject -from acp.schema.slash_commands import AvailableCommand # noqa: TC001 class FileSystemCapability(AnnotatedObject): @@ -270,13 +269,6 @@ class AgentCapabilities(AnnotatedObject): session_capabilities: SessionCapabilities | None = None """Session capabilities supported by the agent.""" - slash_commands: list[AvailableCommand] = Field(default_factory=list) - """Available slash commands that can be invoked by the client. - - These commands are exposed by the agent for direct invocation - via slash command interfaces. Empty list means no commands available. - """ - @classmethod def create( cls, @@ -289,7 +281,6 @@ def create( list_sessions: bool = False, resume_session: bool = False, stop_session: bool = False, - slash_commands: list[AvailableCommand] | None = None, ) -> Self: """Create an instance of AgentCapabilities. @@ -303,7 +294,6 @@ def create( list_sessions: Whether the agent supports `session/list` (unstable). resume_session: Whether the agent supports `session/resume` (unstable). stop_session: Whether the agent supports `session/stop` (unstable). - slash_commands: Available slash commands exposed by the agent. """ session_caps = SessionCapabilities( list=SessionListCapabilities() if list_sessions else None, @@ -319,5 +309,4 @@ def create( image=image_prompts, ), session_capabilities=session_caps, - slash_commands=slash_commands or [], ) diff --git a/src/agentpool_server/acp_server/acp_agent.py b/src/agentpool_server/acp_server/acp_agent.py index c8a507128..a68c8f522 100644 --- a/src/agentpool_server/acp_server/acp_agent.py +++ b/src/agentpool_server/acp_server/acp_agent.py @@ -307,7 +307,6 @@ async def initialize(self, params: InitializeRequest) -> InitializeResponse: self.client_info = params.client_info logger.info("Client info", request=params.model_dump_json()) self._initialized = True - skill_commands = self.get_skill_commands() return InitializeResponse.create( protocol_version=version, name="agentpool", @@ -322,7 +321,6 @@ async def initialize(self, params: InitializeRequest) -> InitializeResponse: audio_prompts=True, embedded_context_prompts=True, image_prompts=True, - slash_commands=skill_commands, ) async def new_session(self, params: NewSessionRequest) -> NewSessionResponse: diff --git a/src/agentpool_server/acp_server/session.py b/src/agentpool_server/acp_server/session.py index b04f925a9..cc612ae28 100644 --- a/src/agentpool_server/acp_server/session.py +++ b/src/agentpool_server/acp_server/session.py @@ -34,6 +34,7 @@ ) from agentpool_server.acp_server.event_converter import ACPEventConverter from agentpool_server.acp_server.input_provider import ACPInputProvider +from agentpool_server.opencode_server.skill_bridge import create_skill_command if TYPE_CHECKING: @@ -245,8 +246,71 @@ async def permission_callback( # Subscribe to state change signal for all agents agent.state_updated.connect(self._on_state_updated) + # Register skill commands from pool's skill_commands registry + self._register_skill_commands() self.log.info("Created ACP session", current_agent=self.agent.name) + def _register_skill_commands(self) -> None: + """Register skill commands from pool's SkillCommandRegistry to command_store. + + Bridges skill commands into the session's command_store so they are + included in available_commands_update notifications per ACP spec. + """ + pool = self.agent_pool + skill_registry = getattr(pool, "skill_commands", None) + if skill_registry is None: + return + + self._skill_command_callback = self._on_skill_command_changed + # Skip scheduling updates during initial registration; + # the caller of create_session already schedules a consolidated update. + self._skill_commands_initializing = True + try: + skill_registry.on_command_change(self._skill_command_callback) + finally: + self._skill_commands_initializing = False + + self.log.debug( + "Subscribed to skill command changes", + skill_count=len(skill_registry.list_items()), + ) + + def _on_skill_command_changed(self, name: str, command: Any | None) -> None: + """Handle skill command add/remove changes from SkillCommandRegistry. + + Args: + name: The name of the skill command. + command: The SkillCommand if added, None if removed. + """ + if command is None: + # Command removed + try: + self.command_store.unregister_command(name) + self.log.debug("Unregistered skill command", skill_name=name) + except Exception: + self.log.exception("Failed to unregister skill command", skill_name=name) + else: + # Command added/updated + try: + from agentpool.skills.command import SkillCommand + + if isinstance(command, SkillCommand): + slashed_cmd = create_skill_command(command) + self.command_store.register_command(slashed_cmd) + self.log.debug("Registered skill command", skill_name=name) + except Exception: + self.log.exception("Failed to register skill command", skill_name=name) + + # Skip notification during initial registration + if getattr(self, "_skill_commands_initializing", False): + return + + # Schedule update via TaskManager for proper lifecycle tracking + try: + self.acp_agent.tasks.create_task(self.send_available_commands_update()) + except Exception: + self.log.exception("Failed to schedule command update") + async def _on_state_updated( self, state: ModeInfo | ModelInfo | AvailableCommandsUpdate | ConfigOptionChanged ) -> None: @@ -528,6 +592,15 @@ async def close(self) -> None: if self.get_cwd_context in agent.sys_prompts.prompts: agent.sys_prompts.prompts.remove(self.get_cwd_context) # pyright: ignore[reportArgumentType] # ty: ignore[invalid-argument-type] + # Unregister skill command callback to prevent memory leak + if hasattr(self, "_skill_command_callback"): + skill_registry = getattr(self.agent_pool, "skill_commands", None) + if skill_registry is not None and hasattr(skill_registry, "_command_change_handlers"): + try: + skill_registry._command_change_handlers.remove(self._skill_command_callback) + except ValueError: + pass # Already removed + # Note: Individual agents are managed by the pool's lifecycle # The pool will handle agent cleanup when it's closed self.log.info("Closed ACP session") diff --git a/tests/acp/schema/test_capabilities.py b/tests/acp/schema/test_capabilities.py index 5afea0e93..dc930e35c 100644 --- a/tests/acp/schema/test_capabilities.py +++ b/tests/acp/schema/test_capabilities.py @@ -5,61 +5,76 @@ import pytest from acp.schema.capabilities import AgentCapabilities -from acp.schema.slash_commands import AvailableCommand -class TestAgentCapabilitiesSlashCommands: - """Test suite for slash_commands field in AgentCapabilities.""" +class TestAgentCapabilities: + """Test suite for AgentCapabilities schema.""" - def test_default_empty_list(self): - """Default value should be empty list (backward compatible).""" + def test_default_load_session(self): + """Default load_session should be False.""" caps = AgentCapabilities() - assert caps.slash_commands == [] + assert caps.load_session is False - def test_accepts_empty_list_explicitly(self): - """AgentCapabilities accepts explicit empty list.""" - caps = AgentCapabilities(slash_commands=[]) - assert caps.slash_commands == [] - - def test_accepts_list_of_commands(self): - """AgentCapabilities accepts list of AvailableCommand.""" - command = AvailableCommand.create( - name="test_cmd", - description="Test command", - input_hint="Provide input", - ) - caps = AgentCapabilities(slash_commands=[command]) - assert len(caps.slash_commands) == 1 - assert caps.slash_commands[0].name == "test_cmd" - assert caps.slash_commands[0].description == "Test command" - - def test_multiple_commands(self): - """AgentCapabilities accepts multiple commands.""" - cmd1 = AvailableCommand.create(name="cmd1", description="First command") - cmd2 = AvailableCommand.create(name="cmd2", description="Second command") - caps = AgentCapabilities(slash_commands=[cmd1, cmd2]) - assert len(caps.slash_commands) == 2 - assert caps.slash_commands[0].name == "cmd1" - assert caps.slash_commands[1].name == "cmd2" + def test_default_mcp_capabilities(self): + """Default mcp_capabilities should be None.""" + caps = AgentCapabilities() + assert caps.mcp_capabilities is None - def test_json_serialization_includes_field(self): - """JSON serialization includes slash_commands field.""" - caps = AgentCapabilities(slash_commands=[]) - json_data = caps.model_dump(mode="json") - assert "slash_commands" in json_data - assert json_data["slash_commands"] == [] + def test_default_prompt_capabilities(self): + """Default prompt_capabilities should be None.""" + caps = AgentCapabilities() + assert caps.prompt_capabilities is None - def test_json_serialization_with_commands(self): - """JSON serialization works with commands.""" - command = AvailableCommand.create(name="my_cmd", description="My command") - caps = AgentCapabilities(slash_commands=[command]) + def test_default_session_capabilities(self): + """Default session_capabilities should be None.""" + caps = AgentCapabilities() + assert caps.session_capabilities is None + + def test_create_method_with_all_capabilities(self): + """create() method should set all capabilities correctly.""" + caps = AgentCapabilities.create( + load_session=True, + http_mcp_servers=True, + sse_mcp_servers=True, + audio_prompts=True, + embedded_context_prompts=True, + image_prompts=True, + list_sessions=True, + resume_session=True, + stop_session=True, + ) + assert caps.load_session is True + assert caps.mcp_capabilities is not None + assert caps.mcp_capabilities.http is True + assert caps.mcp_capabilities.sse is True + assert caps.prompt_capabilities is not None + assert caps.prompt_capabilities.audio is True + assert caps.prompt_capabilities.embedded_context is True + assert caps.prompt_capabilities.image is True + assert caps.session_capabilities is not None + assert caps.session_capabilities.list is not None + assert caps.session_capabilities.resume is not None + assert caps.session_capabilities.stop is not None + + def test_create_method_defaults(self): + """create() method should use correct defaults.""" + caps = AgentCapabilities.create() + assert caps.load_session is False + assert caps.mcp_capabilities is not None + assert caps.mcp_capabilities.http is False + assert caps.mcp_capabilities.sse is False + assert caps.prompt_capabilities is not None + assert caps.prompt_capabilities.audio is False + assert caps.prompt_capabilities.embedded_context is False + assert caps.prompt_capabilities.image is False + + def test_json_serialization(self): + """JSON serialization should not include slash_commands.""" + caps = AgentCapabilities() json_data = caps.model_dump(mode="json") - assert "slash_commands" in json_data - assert len(json_data["slash_commands"]) == 1 - assert json_data["slash_commands"][0]["name"] == "my_cmd" - assert json_data["slash_commands"][0]["description"] == "My command" + assert "slash_commands" not in json_data - def test_json_deserialization_without_field(self): + def test_json_deserialization_without_slash_commands(self): """Backward compatibility: old JSON without slash_commands works.""" json_data = { "load_session": False, @@ -68,48 +83,22 @@ def test_json_deserialization_without_field(self): "session_capabilities": {}, } caps = AgentCapabilities.model_validate(json_data) - assert caps.slash_commands == [] + assert caps.load_session is False + assert caps.mcp_capabilities is not None - def test_json_deserialization_with_empty_list(self): - """JSON deserialization with explicit empty list works.""" - json_data = { - "load_session": False, - "slash_commands": [], - } - caps = AgentCapabilities.model_validate(json_data) - assert caps.slash_commands == [] + def test_json_deserialization_with_slash_commands_ignored(self): + """Backward compatibility: old JSON with slash_commands is ignored safely. - def test_json_deserialization_with_commands(self): - """JSON deserialization with commands works.""" + Pydantic ignores extra fields by default, so old JSON containing + slash_commands should deserialize without errors. + """ json_data = { "load_session": False, "slash_commands": [ {"name": "cmd1", "description": "Command 1"}, - {"name": "cmd2", "description": "Command 2", "input": {"hint": "hint text"}}, ], } caps = AgentCapabilities.model_validate(json_data) - assert len(caps.slash_commands) == 2 - assert caps.slash_commands[0].name == "cmd1" - assert caps.slash_commands[1].name == "cmd2" - assert caps.slash_commands[1].input is not None - assert caps.slash_commands[1].input.root.hint == "hint text" - - def test_create_method_accepts_slash_commands(self): - """create() method accepts slash_commands parameter.""" - command = AvailableCommand.create(name="create_plan", description="Create a plan") - caps = AgentCapabilities.create(slash_commands=[command]) - assert len(caps.slash_commands) == 1 - assert caps.slash_commands[0].name == "create_plan" - - def test_create_method_default_empty_list(self): - """create() method defaults to empty list when not provided.""" - caps = AgentCapabilities.create() - assert caps.slash_commands == [] - - def test_field_is_not_none_type(self): - """slash_commands is list type, not optional None.""" - caps = AgentCapabilities() - # Should be list, not None - assert caps.slash_commands is not None - assert isinstance(caps.slash_commands, list) + assert caps.load_session is False + # slash_commands is not a field on the model, so it's ignored + assert not hasattr(caps, "slash_commands") diff --git a/tests/servers/acp_server/test_acp_skill_commands.py b/tests/servers/acp_server/test_acp_skill_commands.py index dae6e5320..ba9e8617b 100644 --- a/tests/servers/acp_server/test_acp_skill_commands.py +++ b/tests/servers/acp_server/test_acp_skill_commands.py @@ -1,12 +1,13 @@ """TDD tests for ACP server skill commands exposure. These tests verify that skills are properly exposed as slash commands -in the ACP initialize response and available commands updates. +via the session/update notification (available_commands_update), +per the ACP protocol specification. """ from __future__ import annotations -from unittest.mock import MagicMock, Mock +from unittest.mock import AsyncMock, Mock import pytest @@ -18,6 +19,7 @@ from agentpool.skills.command_registry import SkillCommandRegistry from agentpool.skills.skill import Skill from agentpool_server.acp_server.acp_agent import AgentPoolACPAgent +from agentpool_server.acp_server.session import ACPSession @pytest.fixture @@ -59,27 +61,26 @@ def mock_acp_agent_with_skills(agent_pool_with_skill: AgentPool) -> AgentPoolACP return AgentPoolACPAgent(client=mock_connection, default_agent=agent) -async def test_initialize_exposes_skill_commands(mock_acp_agent_with_skills: AgentPoolACPAgent): - """Test that initialize response includes skill commands when skills are configured. +async def test_initialize_does_not_expose_skill_commands( + mock_acp_agent_with_skills: AgentPoolACPAgent, +): + """Test that initialize response does NOT include skill commands. - This is a TDD test: it should fail before the fix and pass after. + Per RFC-0032, slash commands must be advertised via session/update + (available_commands_update) after session creation, not in the + initialize response. """ request = InitializeRequest.create(title="Test", name="test", version="1.0.0") response = await mock_acp_agent_with_skills.initialize(request) assert response.agent_capabilities is not None - assert len(response.agent_capabilities.slash_commands) > 0, ( - "initialize response should expose skill commands when skills are configured" - ) - - cmd_names = [cmd.name for cmd in response.agent_capabilities.slash_commands] - assert "test-skill" in cmd_names, ( - f"Expected 'test-skill' in slash commands, got: {cmd_names}" + assert not hasattr(response.agent_capabilities, "slash_commands"), ( + "initialize response should NOT expose slash_commands per ACP spec" ) -async def test_initialize_without_skills_has_empty_commands(): - """Test that initialize response has empty slash commands when no skills configured.""" +async def test_initialize_without_skills_no_commands(): + """Test that initialize response has no slash_commands field.""" pool = AgentPool() def simple_callback(message: str) -> str: @@ -95,4 +96,52 @@ def simple_callback(message: str) -> str: response = await acp_agent.initialize(request) assert response.agent_capabilities is not None - assert response.agent_capabilities.slash_commands == [] + assert not hasattr(response.agent_capabilities, "slash_commands"), ( + "initialize response should NOT have slash_commands field" + ) + + +async def test_session_update_exposes_skill_commands( + agent_pool_with_skill: AgentPool, +): + """Test that skill commands are advertised via session/update after creation. + + Per RFC-0032, skill commands must be sent via available_commands_update + session notification, not in the initialize response. + """ + agent = agent_pool_with_skill.get_agent("test_agent") + mock_client = AsyncMock() + mock_acp_agent = Mock() + mock_acp_agent.tasks = Mock() + mock_acp_agent.tasks.create_task = lambda coro: coro + + session = ACPSession( + session_id="test-session", + agent=agent, + cwd="/tmp", + client=mock_client, + acp_agent=mock_acp_agent, + ) + + # Mock update_commands to capture what was sent + session.notifications.update_commands = AsyncMock() # type: ignore[method-assign] + + # Call send_available_commands_update and capture what was sent + await session.send_available_commands_update() + + # Verify update_commands was called + assert session.notifications.update_commands.called, ( + "send_available_commands_update should call update_commands" + ) + + # Extract the commands from the call + calls = session.notifications.update_commands.call_args_list + assert len(calls) > 0, "update_commands should have been called" + + sent_commands = calls[0][0][0] if calls[0][0] else calls[0][1].get("commands", []) + command_names = [cmd.name for cmd in sent_commands] + + assert "test-skill" in command_names, ( + f"Skill command 'test-skill' should be in available_commands_update. " + f"Got commands: {command_names}" + )