Skip to content

fix(acp): preload startup skills from --skills - #30560

Open
lsaether wants to merge 1 commit into
NousResearch:mainfrom
lsaether:fix/acp-startup-skills-24466
Open

fix(acp): preload startup skills from --skills#30560
lsaether wants to merge 1 commit into
NousResearch:mainfrom
lsaether:fix/acp-startup-skills-24466

Conversation

@lsaether

Copy link
Copy Markdown
Contributor

Summary

Fixes the ACP startup path so global --skills / -s flags are honored when launching Hermes as an ACP server.

Before this change, hermes -s <skill> acp parsed the requested startup skills in the top-level CLI, but dropped them before acp_adapter.entry.main() and ACP session creation. ACP could still discover/load skills later, but the explicitly requested startup skills were not preloaded into the session context.

This PR threads startup skills through the ACP stack and applies them to each ACP-created agent session:

  • forwards args.skills from hermes_cli/main.py into acp_adapter.entry.main(...)
  • accepts skills= in ACP entry and HermesACPAgent
  • stores normalized startup skills on SessionManager
  • supports repeated and comma-separated skill flags while preserving order and deduping
  • calls build_preloaded_skills_prompt(...) during ACP session agent construction
  • injects the loaded skill prompt via AIAgent(ephemeral_system_prompt=...)
  • reports missing skills as explicit Unknown skill(s): ... errors instead of silently ignoring them

Why

Issue #24466 reports that:

hermes -s <skillname> acp

silently starts ACP mode without preloading the requested skill. That differs from chat mode, where --skills is forwarded into session startup and becomes active before the model's first response.

ACP sessions are created after the stdio server starts, so the fix keeps the requested startup skills in ACP server/session-manager state and applies them when each ACP session constructs its AIAgent.

Implementation notes

  • Uses direct parameter plumbing rather than an environment-variable bridge, so the data flow is explicit and unit-testable.
  • Normalizes startup skills once at SessionManager initialization.
  • Keeps the no-skills path unchanged.
  • Uses ephemeral_system_prompt rather than prefill_messages, so the skill instructions are session guidance rather than persisted/user-visible conversation messages.
  • Does not print anything extra to ACP stdout, preserving ACP protocol cleanliness.

How to test

Focused and regression coverage added for the CLI, ACP entry/server/session layers, and the full ACP startup-skill path.

Run:

scripts/run_tests.sh tests/hermes_cli/test_acp_skills_forwarding.py tests/acp/test_entry.py tests/acp/test_server.py tests/acp/test_session.py tests/acp_adapter/test_acp_commands.py tests/acp_adapter/test_acp_skills.py tests/cli/test_cli_preloaded_skills.py
scripts/run_tests.sh tests/acp tests/acp_adapter
git diff --check
python scripts/check-windows-footguns.py --diff origin/main
python -m hermes_cli.main -s hermes-agent acp --check

Local results:

  • 146 passed for the targeted CLI/ACP/preloaded-skill regression set
  • 301 passed for tests/acp and tests/acp_adapter
  • git diff --check passed
  • Windows footgun checker passed
  • python -m hermes_cli.main -s hermes-agent acp --check passed under the Hermes dev environment

Manual interactive editor smoke test: not run. The ACP behavior is covered through unit/integration tests that assert the CLI-provided startup skills reach ACP session agent construction and are injected as an ephemeral system prompt.

Platforms tested

  • Linux / Arch local development environment

Cross-platform impact should be low: this is Python argument/session plumbing and prompt construction only. It does not add filesystem, process-management, shell, path, or dependency changes.

Related issue

Closes #24466

Scope

This PR only fixes startup skill propagation for ACP. It does not change skill discovery, skill installation, ACP protocol framing, editor configuration, or runtime /skill behavior.

@alt-glitch alt-glitch added type/bug Something isn't working comp/acp Agent Communication Protocol adapter tool/skills Skills system (list, view, manage) P2 Medium — degraded but workaround exists labels May 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This is the 6th competing PR for #24466 (after #24474, #24521, #24526, #24635, #24683, #26602 — all closed). If this version is preferred over #24474, please close the earlier ones explicitly.

briandevans added a commit to briandevans/hermes-agent that referenced this pull request Jun 2, 2026
…Research#30571)

`hermes -p <profile> acp` already set `HERMES_HOME` via the top-level
profile override, but ACP-created sessions could not see the canonical
profile name (HERMES_PROFILE was never published) and `hermes-acp -p
<profile>` direct invocations — used by editor configs like Zed agent
commands — had no `--profile` flag at all.

This change plumbs profile activation through both entrypoints:

1. `hermes_cli.main._apply_profile_override` now also publishes
   `HERMES_PROFILE` (canonical, setdefault-only so a shell-set value
   wins). Downstream callers that branch on profile name (kanban,
   gateway adapters, ACP entry) can read it without re-parsing
   `HERMES_HOME`.
2. `acp_adapter.entry` gains a `--profile/-p` argument that runs
   `resolve_profile_env` before `_load_env`, so direct
   `hermes-acp -p code-reviewer` invocations land in the same profile
   environment as `hermes -p code-reviewer acp`.
3. `hermes_cli.main.cmd_acp` forwards the active profile to `acp_main`
   via argv, keeping the two invocation paths consistent.

Mirrors lsaether's `--skills` plumbing pattern in PR NousResearch#30560 so the two
follow-ups land cleanly side by side.

Tests cover: both entrypoints set HERMES_HOME + HERMES_PROFILE before
`_load_env`; `-p` is equivalent to `--profile`; `--check` short-circuit
still honours `--profile`; missing `--profile` leaves inherited env
untouched; unknown profile name exits cleanly; explicit
`HERMES_PROFILE` from the spawning shell is preserved.
NakePranob pushed a commit to NakePranob/hermes-agent that referenced this pull request Jul 3, 2026
…Research#30571)

`hermes -p <profile> acp` already set `HERMES_HOME` via the top-level
profile override, but ACP-created sessions could not see the canonical
profile name (HERMES_PROFILE was never published) and `hermes-acp -p
<profile>` direct invocations — used by editor configs like Zed agent
commands — had no `--profile` flag at all.

This change plumbs profile activation through both entrypoints:

1. `hermes_cli.main._apply_profile_override` now also publishes
   `HERMES_PROFILE` (canonical, setdefault-only so a shell-set value
   wins). Downstream callers that branch on profile name (kanban,
   gateway adapters, ACP entry) can read it without re-parsing
   `HERMES_HOME`.
2. `acp_adapter.entry` gains a `--profile/-p` argument that runs
   `resolve_profile_env` before `_load_env`, so direct
   `hermes-acp -p code-reviewer` invocations land in the same profile
   environment as `hermes -p code-reviewer acp`.
3. `hermes_cli.main.cmd_acp` forwards the active profile to `acp_main`
   via argv, keeping the two invocation paths consistent.

Mirrors lsaether's `--skills` plumbing pattern in PR NousResearch#30560 so the two
follow-ups land cleanly side by side.

Tests cover: both entrypoints set HERMES_HOME + HERMES_PROFILE before
`_load_env`; `-p` is equivalent to `--profile`; `--check` short-circuit
still honours `--profile`; missing `--profile` leaves inherited env
untouched; unknown profile name exits cleanly; explicit
`HERMES_PROFILE` from the spawning shell is preserved.

@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 tracing the full CLI → ACP-session path. The underlying defect is still present on current main: cmd_acp drops args.skills at hermes_cli/main.py:12720-12731, acp_adapter/entry.py:260 creates an unconfigured agent, and SessionManager._make_agent has no preload path at acp_adapter/session.py:617-645.

Problems

  • The new ephemeral_system_prompt path would be omitted from ACP's context indicator. acp_adapter/server.py:683-686 estimates usage from _cached_system_prompt, but the actual request appends ephemeral_system_prompt at agent/conversation_loop.py:852-856. A large preloaded skill would therefore under-report the session's used context.

Suggested changes

  • Include ephemeral_system_prompt in _build_usage_update()'s effective system prompt and add a regression test covering a preloaded skill.

Automated hermes-sweeper review.

Comment thread acp_adapter/session.py
if missing_skills:
raise ValueError(f"Unknown skill(s): {', '.join(missing_skills)}")
if skills_prompt:
existing_prompt = kwargs.get("ephemeral_system_prompt") or ""

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.

ephemeral_system_prompt is sent on every request, but ACP usage estimation currently counts only _cached_system_prompt (acp_adapter/server.py:683-686). Please update that estimator and add coverage so startup-skill tokens are reflected in the ACP context indicator.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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 13, 2026
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 P2 Medium — degraded but workaround exists 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-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: --skills not passed to acp command.

3 participants