Skip to content

fix(acp): instruct model to treat ACP attached-file blocks as inline context - #56742

Closed
NakePranob wants to merge 7 commits into
NousResearch:mainfrom
NakePranob:eva/acp-audit-fixes
Closed

fix(acp): instruct model to treat ACP attached-file blocks as inline context#56742
NakePranob wants to merge 7 commits into
NousResearch:mainfrom
NakePranob:eva/acp-audit-fixes

Conversation

@NakePranob

Copy link
Copy Markdown

Problem

When a user @-tags a file in Zed ACP, the adapter correctly inlines the file content into the prompt as [Attached file: ...] URI: ... followed by the file body. However, the LLM treats this as plain text and runs shell commands to fetch the file instead of reading the content already in its context window.

Root Cause

The system prompt had no instruction about [Attached file: ...] blocks from the ACP adapter.

Fix

Add a short instruction to the stable tier of agent/system_prompt.py:

[Attached file: ...] blocks contain the actual file — read the content inline, do not call tools to fetch it.

Verification

  • 298 passed, 3 failed (same pre-existing edit_approval macOS failures)
  • Module import OK
  • No regressions

…ll 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
…es, 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.
- _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.
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.
…g, 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).
…context

The ACP adapter inlines user-attached files as:
  [Attached file: <name>]
  URI: <path>

  <file content>

Models treat this as plain text rather than real file context and often
try to shell-fetch the file instead of reading the body already present
in the prompt. Add a short instruction in the stable system-prompt tier
so the model uses the inline content directly.
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/acp Agent Communication Protocol adapter comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/mcp MCP client and OAuth labels Jul 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Scope note for reviewers: the PR description presents this as a one-line addition to agent/system_prompt.py, but the diff is ~1024 additions across 10 files, including a substantial ACP-adapter rework in acp_adapter/server.py (block extraction, initialize() capabilities, plan-status updates, MCP capabilities), acp_adapter/events.py, acp_adapter/entry.py, and hermes_cli/main.py. It also carries a .hermes-acp-fix-spec.md planning doc and a stray .install_method file. Please label/review for the full ACP change, and consider dropping the artifacts and splitting into focused PRs. Related: #25611 (ACP embedded_context capability, open), #21407 (merged inline-attachment salvage).

@NakePranob

Copy link
Copy Markdown
Author

Per reviewer feedback, this PR has been split into three focused PRs:

  1. fix(acp): instruct model to treat ACP attached-file blocks as inline context #57509fix(acp): instruct model to treat ACP attached-file blocks as inline context (1 file, +7 lines)
    → The one-line system_prompt change originally claimed.

  2. feat(acp): adapter compliance v1 — capabilities, resource blocks, config modes, skills #57510feat(acp): adapter compliance v1 — capabilities, resource blocks, config modes, skills (6 files, +892/−64 lines)
    → All ACP adapter code, tests only. No artifacts, no CLI scope.

  3. feat(cli): forward --skills flag in ACP mode #57511feat(cli): forward --skills flag in ACP mode (1 file, +3 lines)
    → The standalone CLI change.

Planning artifact (.hermes-acp-fix-spec.md) and stray file (.install_method) have been removed.

Closing this PR in favor of the three focused ones above.

@NakePranob

Copy link
Copy Markdown
Author

Superseded by #57509, #57510, and #57511

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have tool/mcp MCP client and OAuth type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants