Skip to content

feat(acp): adapter compliance v1 — capabilities, resource blocks, config modes, skills - #57510

Closed
NakePranob wants to merge 6 commits into
NousResearch:mainfrom
NakePranob:feat/acp-adapter-rework
Closed

feat(acp): adapter compliance v1 — capabilities, resource blocks, config modes, skills#57510
NakePranob wants to merge 6 commits into
NousResearch:mainfrom
NakePranob:feat/acp-adapter-rework

Conversation

@NakePranob

@NakePranob NakePranob commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown

NakePranob added 6 commits July 3, 2026 10:42
@NakePranob
feat(ACP): fix config_options, skill loading, MCP tools retention, ad…
fc3a43a 
…d /skill command

- Bug 1: --skills flag in ACP mode (cli forwarding + entry preload)
- Bug 2: config_options=None -> model selector in Zed
- Bug 3: set_session_model() preserves MCP toolsets across model switch
- Bug 4: _cmd_model slash command preserves MCP toolsets too
- Feature: /skill slash command (list/load skills)
- Tests: update config_options assertion, advertised commands list
@NakePranob
feat(ACP): comprehensive v1 compliance — resource blocks, plans, conf…
2c64739 
…ig modes, init caps

Codex-driven fixes covering all spec gaps:

Critical:
- _extract_text() now handles ALL ACP content block types (Text, Resource,
  EmbeddedResource, Image, Audio) — tagged files (@-references) work now
- Empty prompt returns end_turn gracefully with client notification

Priority 2:
- Plan entry status tracking: todo tool. started sends session/update plan
- config_options now includes mode selection (category='mode')
- initialize() response: embedded_context=True, audio=False in promptCapabilities

Priority 3:
- All advertised commands have displayName, toolTip, icon via field_meta
- set_config_option handles mode ('mode' config_id) changes

New tests: 5 (plan update, empty prompt, config mode/model routing)
All 293 ACP tests passing.
@NakePranob
feat(ACP): expose each skill as its own /command like OpenCode
4e8ecf9 
- _skill_commands() classmethod enumerates all registered skills as
  individual AvailableCommand entries
- _available_commands() appends skill commands after built-in commands
- _handle_slash_command() routes /<skill-name> directly to skill loader
  (instead of requiring /skill load <name>)
- _cmd_help() shows skills section with type /name to load
- Test: available_commands list now includes dynamic skill entries

Zed's / menu now shows all ~94 skills alongside built-in commands,
matching OpenCode's behaviour.
@NakePranob
feat(ACP): skill /<name> auto-executes via LLM (like OpenCode)
516682f 
When a user types /<skill-name> in the ACP chat:
  - Skill loads into system prompt
  - _pending_skill_prompt flag stores the skill text
  - _handle_slash_command returns None to fall through to LLM
  - prompt() rewrites user_text/user_content to the skill's own prompt
  - LLM responds with the skill's voice/instructions immediately

Previously the handler returned 'Loaded skill(s): ...' and ended the
turn without invoking the LLM, leaving the skill loaded but idle.

Also fixes nested try/except lint issue in _handle_slash_command.
@NakePranob
fix(ACP): compliance fixes — tool_call in_progress status, stopReason…
ca3f51a 
… catalog, ext_method/notification, close_session, fs/* via ext_method

HIGH Priority:
- tool_progress_cb now emits ToolCallProgress(status=in_progress) immediately
  after ToolCallStart so clients see pending->in_progress->completed progression
- stopReason detects max_tokens (output length limit) and refusal
  (content_policy_blocked) from result.error, mapped to spec-correct values
- ext_method/ext_notification overrides: unknown methods now raise
  RequestError.method_not_found() instead of returning empty dict silently
  (the base class' empty-dict behaviour was incorrect per spec)

MEDIUM Priority:
- close_session() added to HermesACPAgent: maps to session/close (unstable),
  delegates to SessionManager.remove_session() and returns CloseSessionResponse
- fs/read_text_file and fs/write_text_file implemented via ext_method:
  resolve path relative to session cwd, validate existence, read/write with
  proper RequestError (invalid_params/internal_error) on failure,
  return TextResourceContents for reads and empty dict for writes

All 293 ACP tests pass (3 pre-existing edit_approval tests deselected
for macOS /private/var/folders/ sensitive-path issue).
@NakePranob
chore: drop artifacts and CLI scope from adapter PR
3232aca
@NakePranob NakePranob mentioned this pull request Jul 3, 2026
Closed
@github-actions github-actions Bot mentioned this pull request Jul 3, 2026
Open
@NakePranob
NakePranob force-pushed the feat/acp-adapter-rework branch from 76ee104 to 3232aca Compare July 3, 2026 04:29
@alt-glitch alt-glitch added type/feature New feature or request comp/acp Agent Communication Protocol adapter tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Jul 3, 2026
@alt-glitch

alt-glitch commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #25611 (embedded_context + resource-block extraction), #21407 (merged inline-attachment salvage), #57509 (companion system-prompt instruction from the same author). This is broad ACP spec-compliance work — a reviewer should reconcile it against the existing #25611 embedded_context PR rather than merging both.

@alt-glitch alt-glitch mentioned this pull request Jul 3, 2026
Closed
@NakePranob NakePranob mentioned this pull request Jul 3, 2026
Closed
teknium1
teknium1 reviewed Jul 15, 2026
View reviewed changes

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the broad ACP compliance work. It needs selective salvage onto current main rather than a direct merge.

Problems

  • The skill command path mutates and invalidates the live system prompt (acp_adapter/server.py:154-162, :2163-2168, :2511-2514). Current Hermes requires that prompt to remain stable for a session (agent/system_prompt.py:519-532; AGENTS.md:88-91).
  • The new filesystem extension handlers accept absolute and traversal paths without workspace containment (acp_adapter/server.py:677-735), including arbitrary directory creation on write.
  • config_options conflicts with the deliberate current ACP/Zed contract: config options replace the model-picker slot, so current main uses SessionModeState and leaves config_options absent (acp_adapter/server.py:534-565; tests/acp/test_server.py:56-67).
  • Resource inlining is already implemented on main by 733e297b8 and 7e2af0c2e; the remaining embedded_context capability overlaps open #25611.

Suggested changes

  • Salvage the focused capability work with #25611.
  • Preserve prompt-cache invariants for skill activation and add containment plus regression coverage before exposing filesystem RPCs.

This is an automated hermes-sweeper review.

Comment thread acp_adapter/server.py
else:
resolved = Path(file_path)

resolved.parent.mkdir(parents=True, exist_ok=True)

@teknium1 teknium1 Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This creates directories for any client-supplied path. Path(cwd) / file_path does not confine absolute paths, and neither handler resolves or rejects ..; add canonical workspace containment before any read or write.

Comment thread acp_adapter/server.py
# Load skill into system prompt, then fall through to LLM
prompt_text, loaded_names, missing_names = build_preloaded_skills_prompt([skill_name])
if loaded_names:
_inject_skills_prompt(state.agent, prompt_text or "")

@teknium1 teknium1 Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_inject_skills_prompt() changes and invalidates the cached system prompt mid-session. Hermes requires the system prompt to be byte-stable for the conversation to preserve prefix caching; route this through the established user-message skill invocation flow or defer it to a fresh session.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@GottZ GottZ mentioned this pull request Jul 26, 2026
Open
@NakePranob

NakePranob commented Jul 27, 2026

Copy link
Copy Markdown
Author

ปิด PR นี้ตามคำขอ (no longer needed / cleanup)

@NakePranob NakePranob closed this Jul 27, 2026
@GottZ GottZ mentioned this pull request Aug 3, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teknium1 teknium1 teknium1 left review comments

Assignees

No one assigned

Labels

comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/skills Skills system (list, view, manage) type/feature New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NakePranob @alt-glitch @teknium1