fix(acp): honour --profile from CLI and hermes-acp direct entry (#30571) - #30612
Closed
briandevans wants to merge 4 commits into
Closed
fix(acp): honour --profile from CLI and hermes-acp direct entry (#30571)#30612briandevans wants to merge 4 commits into
briandevans wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR ensures the active Hermes profile name is propagated via HERMES_PROFILE (alongside HERMES_HOME) and is forwarded into the ACP adapter so direct hermes-acp usage and hermes -p <name> acp behave consistently.
Changes:
- Publish canonical
HERMES_PROFILEwhen a profile override is applied inhermes_cli.main._apply_profile_override. - Add
--profile/-psupport tohermes-acpstartup and apply it before env loading and subcommand short-circuits. - Add regression tests covering
HERMES_PROFILEpublication and ACP profile propagation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/hermes_cli/test_apply_profile_override.py | Adds regression tests asserting HERMES_PROFILE is published (and preserved) when profile overrides apply. |
| tests/acp/test_entry.py | Adds ACP entrypoint tests verifying --profile/-p sets env before _load_env and works with --check. |
| hermes_cli/main.py | Publishes HERMES_PROFILE and forwards it into hermes-acp argv when running hermes ... acp. |
| acp_adapter/entry.py | Adds --profile/-p flag and applies profile override early to scope env/config loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tmp_path, | ||
| monkeypatch, | ||
| hermes_home=None, | ||
| active_profile="Coder", |
Comment on lines
+175
to
+198
| @@ -189,6 +190,12 @@ def _apply_profile_override() -> None: | |||
| ) | |||
| return | |||
| os.environ["HERMES_HOME"] = hermes_home | |||
| # Also publish the canonical profile name so downstream code that | |||
| # branches on profile identity (kanban, gateway adapters, ACP entry) | |||
| # can pick it up without re-parsing HERMES_HOME. Honour any value the | |||
| # caller already exported — a deliberate ``HERMES_PROFILE=...`` on | |||
| # the spawning shell should win over this best-effort default. | |||
| os.environ.setdefault("HERMES_PROFILE", canonical_name) | |||
Comment on lines
+181
to
+184
| # Profile resolution must never prevent ACP from starting; surface | ||
| # the issue and fall back to whatever environment the caller had. | ||
| print( | ||
| f"Warning: profile override failed ({exc}), using default", |
Comment on lines
+13268
to
+13275
| # Forward the active profile name so ACP can announce it in logs | ||
| # and so direct ``hermes-acp`` invocations from editor configs | ||
| # behave the same as ``hermes -p <name> acp``. The env var is | ||
| # populated by ``_apply_profile_override`` when ``-p/--profile`` | ||
| # was passed; absence means the default profile. | ||
| active_profile = os.environ.get("HERMES_PROFILE", "").strip() | ||
| if active_profile and active_profile != "default": | ||
| acp_argv.extend(["--profile", active_profile]) |
Comment on lines
+157
to
+191
| """Resolve ``--profile`` and publish HERMES_HOME / HERMES_PROFILE. | ||
|
|
||
| Runs at ACP startup so that ``hermes-acp -p <name>`` direct invocations | ||
| (used by editor configs like Zed agent commands) end up with the same | ||
| environment as ``hermes -p <name> acp`` — i.e. ``get_hermes_home()`` | ||
| returns the profile directory and downstream callers that branch on | ||
| ``HERMES_PROFILE`` see the canonical name. | ||
|
|
||
| When the env is *already* pointing at this profile (because the parent | ||
| ``hermes_cli.main._apply_profile_override`` ran first), the resolver is | ||
| a no-op: ``setdefault`` preserves the inherited HERMES_PROFILE and the | ||
| HERMES_HOME assignment writes the same string back. | ||
| """ | ||
| if not profile_name: | ||
| return | ||
| try: | ||
| from hermes_cli.profiles import normalize_profile_name, resolve_profile_env | ||
|
|
||
| hermes_home = resolve_profile_env(profile_name) | ||
| canonical_name = normalize_profile_name(profile_name) | ||
| except (ValueError, FileNotFoundError) as exc: | ||
| print(f"Error: {exc}", file=sys.stderr) | ||
| sys.exit(1) | ||
| except Exception as exc: | ||
| # Profile resolution must never prevent ACP from starting; surface | ||
| # the issue and fall back to whatever environment the caller had. | ||
| print( | ||
| f"Warning: profile override failed ({exc}), using default", | ||
| file=sys.stderr, | ||
| ) | ||
| return | ||
| os.environ["HERMES_HOME"] = hermes_home | ||
| os.environ.setdefault("HERMES_PROFILE", canonical_name) | ||
|
|
||
|
|
alt-glitch
added
type/bug
Contributor
Author
|
@copilot Findings addressed:
|
Contributor
Author
|
CI note — the single That test is a browser-secret-exfil subprocess test (added in 712aa44, security:); it has no code path overlap with this PR's |
Contributor
Author
|
@copilot All review nits addressed in commit 7061eadfb:
|
briandevans
force-pushed
the
fix/acp-profile-propagation-30571
branch
from
May 25, 2026 03:12
7061ead to
0636edb
Compare
briandevans
force-pushed
the
fix/acp-profile-propagation-30571
branch
from
May 27, 2026 12:15
0636edb to
a6825cd
Compare
briandevans
force-pushed
the
fix/acp-profile-propagation-30571
branch
from
May 28, 2026 00:18
a6825cd to
bef4ebd
Compare
briandevans
force-pushed
the
fix/acp-profile-propagation-30571
branch
from
May 29, 2026 03:14
bef4ebd to
7554fd3
Compare
briandevans
force-pushed
the
fix/acp-profile-propagation-30571
branch
from
May 30, 2026 04:11
7554fd3 to
a8b73ae
Compare
…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.
The fixture creates ``profiles/<active_profile>`` literally, but production resolves ``-p Coder`` to ``profiles/coder`` via ``normalize_profile_name``. When ``active_profile="Coder"`` was passed, the resolver couldn't find ``profiles/coder`` and raised FileNotFoundError, causing test_explicit_flag_sets_hermes_profile_canonical_name to fail on CI. Lowercase the directory name to match what production looks up.
Addresses Copilot review on NousResearch#30612. When ``HERMES_HOME`` is already a profile directory (set by an outer ``hermes -p <name>`` invocation that re-execs into the gateway, or by a child process inheriting the resolved env), ``_apply_profile_override`` returns early. The new ``HERMES_PROFILE`` contract was unset on that branch, so downstream callers that branch on profile identity still couldn't see the active profile when re-entering this layer. Mirror the late-path ``setdefault`` from the basename of HERMES_HOME so the contract holds on every execution path. Also clarify the warning text in ``acp_adapter.entry`` — the fallback preserves whatever HERMES_HOME / HERMES_PROFILE the caller already had, which may itself be a non-default profile, so "using default" was misleading during debugging. Adds two regression tests for the inherited-HERMES_HOME path: one for the default setdefault behaviour and one proving the caller-set HERMES_PROFILE still wins.
…_argv Two Copilot review nits on NousResearch#30612, fixed together because they share a seam: 1. ``_apply_profile_override`` was duplicated across ``hermes_cli.main`` and ``acp_adapter.entry`` with very similar resolve / setenv / exit logic. Drift risk: the entry-point that diverges first (e.g. on a new env var) silently desyncs the two ACP call paths. Extracted ``apply_profile_env(profile_name) -> bool`` to ``hermes_cli.profiles``; both call sites now delegate. Behaviour preserved verbatim, including the existing-environment warning wording (main.py previously said "using default", now matches entry.py's "using existing environment" — the misleading "default" string was Copilot's NousResearch#3 on the original PR). 2. ``cmd_acp``'s argv-forwarding logic (the NousResearch#30571 fix surface) had no test coverage because the function is nested inside ``main()`` and not directly importable. Pulled the argv-building into a module-level ``_build_acp_argv(args, env=None)`` helper and added ``tests/hermes_cli/test_build_acp_argv.py`` covering: profile set forwards ``--profile <name>``, ``HERMES_PROFILE=default`` does NOT forward, unset/empty/whitespace does NOT forward, flag order stable, env defaults to ``os.environ`` for the production call. No behaviour change for ``hermes acp`` or ``hermes-acp``; both still mirror ``--profile`` through to the ACP server and resolve via the same code path.
briandevans
force-pushed
the
fix/acp-profile-propagation-30571
branch
from
June 2, 2026 23:20
a8b73ae to
748242c
Compare
Contributor
Author
|
Closing to focus the queue on security/file-safety work where civilian merges are landing. Happy to reopen if maintainers want this picked up. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
Plumbs
--profilepropagation through both ACP entrypoints sohermes -p <name> acpAND directhermes-acp -p <name>invocations (used by editor configs like Zed agent commands) run ACP under the requested profile — config.yaml, .env, skills, memory, gateway state, and the canonical profile name all visible to downstream callers.Root cause split:
hermes_cli.main._apply_profile_overridesetHERMES_HOMEforhermes -p <name> acpbut never published the canonical profile name. Downstream code that branches onHERMES_PROFILE(kanban, gateway adapters) couldn't see the active profile.acp_adapter.entry._parse_argshad no--profileflag at all, sohermes-acp -p code-reviewer— the form an editor config would use — failed with "unrecognized arguments".Mirrors
_apply_profile_overrideprecedence: explicit--profile/-parg wins, then falls through to the inheritedHERMES_HOME/HERMES_PROFILEfrom the parent shell. UsessetdefaultforHERMES_PROFILEso a deliberately-exported value from the spawning shell is preserved.Sibling code paths I considered:
lsaether's PR #30560 (open) wires--skillsfrom CLI →acp_main→HermesACPAgent→SessionManagerfor the same family of bugs. This PR uses an env-var-based propagation instead because profile activation is a process-wide concern (HERMES_HOME flips multiple subsystems at once), whereas--skillsis per-session ephemeral state. Both PRs touchacp_adapter/entry.pybut in independent ways and should merge cleanly side by side.Related Issue
Fixes #30571
Type of Change
Changes Made
hermes_cli/main.py—_apply_profile_overridenow publishesHERMES_PROFILE(canonical, setdefault-only) alongsideHERMES_HOME;cmd_acpforwards the active profile toacp_mainvia argv so the two invocation paths converge on the same environment.acp_adapter/entry.py— adds--profile/-pto the argument parser and a new_apply_profile_override(profile_name)helper that callsresolve_profile_env, setsHERMES_HOME, and seedsHERMES_PROFILE. Runs BEFORE every subcommand branch (--version,--check,--setup,--setup-browser, server start) so all of them observe the profile-scoped environment.tests/acp/test_entry.py— six new tests covering:--profile XsetsHERMES_HOMEbefore_load_env;-pequivalent to--profile;--checkshort-circuit still honours--profile; absence of--profileleaves inherited env untouched; unknown profile exits with status 1; explicitHERMES_PROFILEfrom the shell wins over setdefault.tests/hermes_cli/test_apply_profile_override.py— four new tests covering the parent_apply_profile_override: explicit flag sets canonical name; stickyactive_profilesets canonical name; existingHERMES_PROFILEis preserved;defaultprofile does not publish either env var.How to Test
Create two profiles with different default models:
hermes profile create code-reviewer hermes profile create fast-coder # Edit ~/.hermes/profiles/code-reviewer/config.yaml + .envDirect ACP entrypoint (the form editor configs use):
CLI entrypoint:
hermes -p fast-coder acp # cmd_acp forwards --profile fast-coder into acp_argvFocused tests:
uv run --with pytest --with pytest-xdist --with pytest-asyncio --with pytest-timeout --with 'agent-client-protocol==0.9.0' python3 -m pytest tests/acp/test_entry.py tests/hermes_cli/test_apply_profile_override.py -v24 tests pass locally (10 pre-existing + 10 new across the two files; 4 of the new tests fail on a baseline-revert check, confirming the regression guard).
Checklist
Code
fix(acp): …)tests/acp/test_entry.pyandtests/hermes_cli/test_apply_profile_override.py)Documentation & Housekeeping
--helptext on the new--profileflag is self-describing)cli-config.yaml.example— N/A (no new config keys)CONTRIBUTING.mdorAGENTS.md— N/A (no architecture change)Contract Protected
Invariant: Both
hermes -p <name> acpandhermes-acp -p <name>(andhermes-acp --profile=<name>) MUST leave the process withHERMES_HOMEpointing at the profile directory andHERMES_PROFILEset to the canonical name, before any session manager, runtime provider, or.envloader runs.Known-bad inputs covered: unknown profile name (exits 1, doesn't silently fall back); empty
--profile(no env mutation); operator-setHERMES_PROFILEfrom the shell (preserved via setdefault).Negative case:
test_main_profile_flag_rejects_unknown_profileproves the bad path fails loudly instead of silently using the default profile.Repro case from the issue:
test_main_profile_flag_sets_hermes_home_before_load_envexercises the exact scenario from #30571's "Steps to Reproduce" — launch with--profile <name>, observe_load_envsees the profile-scopedHERMES_HOME.Related / Positioning