Skip to content

Add experimental Hermes agent target#1726

Merged
sergio-sisternes-epam merged 3 commits into
microsoft:mainfrom
altaranenco:altaranenco/hermes-agent-support
Jun 10, 2026
Merged

Add experimental Hermes agent target#1726
sergio-sisternes-epam merged 3 commits into
microsoft:mainfrom
altaranenco:altaranenco/hermes-agent-support

Conversation

@altaranenco

Copy link
Copy Markdown
Contributor

TL;DR

Adds the Hermes Agent (Nous Research) as a new experimental APM compile/install target. apm install --target hermes and apm compile -t hermes now deploy APM primitives where Hermes natively reads them. Validated end-to-end against a real Hermes v0.16.0 deployment (Docker, z.ai GLM backend, live Telegram gateway).

Problem (WHY)

Hermes is a terminal-native autonomous agent that lives in ~/.hermes/ and talks to users over 20+ messaging platforms. It already consumes two open standards APM emits -- agentskills.io SKILL.md and AGENTS.md -- yet APM had no hermes target, so users had to hand-wire skills, context, and MCP servers.

Approach (WHAT)

Model hermes as one data-driven KNOWN_TARGETS entry plus a single new MCP adapter. No new skill/instruction transformers -- skills reuse the existing skill_standard format and instructions reuse the agents (AGENTS.md) compile family. Only MCP needed a new writer because Hermes uses a YAML mcp_servers: schema distinct from the JSON mcpServers/mcp of other clients.

The target is gated behind a new hermes experimental flag (mirrors openclaw/copilot-cowork); it is invisible and excluded from --all until apm experimental enable hermes.

APM primitive Hermes surface Location
skills native skills (agentskills.io) .agents/skills/<n>/SKILL.md (project) / ~/.hermes/skills/<n>/SKILL.md (--global)
instructions AGENTS.md context file AGENTS.md at repo root (via apm compile)
MCP servers mcp_servers: YAML block ~/.hermes/config.yaml (user scope)

HERMES_HOME overrides the Hermes home directory (mirrors Claude's CLAUDE_CONFIG_DIR).

Implementation (HOW)

  • adapters/client/hermes.py -- HermesClientAdapter: writes the YAML mcp_servers: block (stdio command/args/env, http url/headers, per-server enabled), merges into existing config, and preserves all sibling top-level keys. Uses utils/yaml_io (no raw yaml.dump).
  • integration/targets.py -- hermes KNOWN_TARGETS entry, resolve_hermes_root(), HERMES_HOME handling in for_scope.
  • core/experimental.py, core/target_detection.py, factory.py, install/phases/targets.py, integration/mcp_integrator_install.py, bundle/lockfile_enrichment.py -- flag registration, MCP-client registration, enable-hint, opt-in gating, cross-target skill-path map.
  • Docs: new integrations/hermes.md + sidebar; producer/compile.md, apm-usage commands.md, CHANGELOG.md.

Spec-first ordering: unit + integration tests were written (and confirmed red) before the implementation.

Validation evidence

  • Lint (CI mirror): ruff check, ruff format --check, pylint R0801 (10.00/10), auth-signals -- all green.
  • Tests: new tests/unit/adapters/test_hermes_client_adapter.py + tests/integration/test_hermes_target.py, plus registry/scope/detection updates. Full targeted + broad suites pass.
  • Live deployment (real Hermes v0.16.0, Docker on a remote host, z.ai glm-4.6):
    • apm install --target hermes --global -> Hermes lists the deployed skill natively (local / enabled) and the MCP server (uvx mcp-server-time ... enabled); ~/.hermes/config.yaml gains the mcp_servers entry with every sibling key preserved.
    • apm compile -t hermes -> AGENTS.md emitted with the instruction content.
    • Gateway boots, registers the APM MCP server's tools at runtime, connects to Telegram (polling).
    • A real one-shot agent turn on z.ai returned the exact marker defined by the APM-deployed skill, proving the full install -> Hermes -> model pipeline.

How to test

apm experimental enable hermes
apm install --target hermes --global   # skills -> ~/.hermes/skills/, MCP -> ~/.hermes/config.yaml
apm compile -t hermes                  # -> AGENTS.md

Notes

  • No secrets are committed. All live-test credentials (Telegram token, allowed user, z.ai key) stayed in a local/remote 600-mode env file outside the repo; the live-test Docker harness is a session artifact, not part of this PR.
  • .agents/skills/ is shared at project scope; detect_by_dir=False keeps hermes explicit---target-only so it never silently activates.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Add the 'hermes' compile/install target (Nous Research Hermes agent),
gated behind the new 'hermes' experimental flag. Skills reuse the
agentskills.io SKILL.md format (.agents/skills/ at project scope,
~/.hermes/skills/ at user scope); instructions reuse the AGENTS.md
compile family; MCP servers are written to the YAML mcp_servers: block
of ~/.hermes/config.yaml via a new HermesClientAdapter. HERMES_HOME
overrides the Hermes home directory.

Spec-first: unit + integration tests precede the implementation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 10, 2026 16:40

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a new experimental hermes target integrating the Hermes agent into APM’s target registry, compilation routing, and MCP server configuration.

Changes:

  • Register hermes as an experimental target (registry, CLI target detection/constants, factory wiring).
  • Implement Hermes MCP adapter writing mcp_servers to ~/.hermes/config.yaml (with HERMES_HOME override) and gate runtime auto-discovery.
  • Add unit/integration tests and documentation/guide updates for Hermes behavior and flag-gating.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/unit/integration/test_targets_registry_completeness.py Adds Hermes adapter to registry completeness checks.
tests/unit/integration/test_targets.py Adds hermes target invariants + scope/flag tests.
tests/unit/core/test_target_detection.py Updates experimental target constant membership to include hermes.
tests/unit/core/test_scope.py Updates KNOWN_TARGETS expected set to include hermes.
tests/unit/adapters/test_hermes_client_adapter.py New unit tests for Hermes adapter conversion + YAML merge semantics.
tests/integration/test_hermes_target.py New E2E coverage for hermes flag gating, deploy paths, and compile routing.
src/apm_cli/integration/targets.py Adds hermes TargetProfile and resolve_hermes_root; extends user-scope env var logic.
src/apm_cli/integration/mcp_integrator_install.py Refactors runtime discovery; adds hermes opt-in detection gating.
src/apm_cli/install/phases/targets.py Adds hermes enable-hint logic via shared experimental-target check helper.
src/apm_cli/factory.py Registers HermesClientAdapter in ClientFactory.
src/apm_cli/core/target_detection.py Adds hermes to compile routing, descriptions, and EXPERIMENTAL_TARGETS.
src/apm_cli/core/experimental.py Registers hermes experimental flag metadata.
src/apm_cli/bundle/lockfile_enrichment.py Adds hermes path translation mapping for skills.
src/apm_cli/adapters/client/hermes.py New Hermes MCP adapter writing YAML mcp_servers block.
packages/apm-guide/.apm/skills/apm-usage/commands.md Documents hermes usage in apm install command reference.
docs/src/content/docs/producer/compile.md Documents experimental targets incl. hermes; adds link to integration guide.
docs/src/content/docs/integrations/hermes.md New Hermes integration documentation page.
docs/astro.config.mjs Adds Hermes integration page to docs nav.
CHANGELOG.md Notes new experimental Hermes target support and behavior.

Comment thread src/apm_cli/integration/targets.py Outdated
Comment thread src/apm_cli/adapters/client/hermes.py Outdated
Comment thread src/apm_cli/core/target_detection.py Outdated
Comment thread docs/src/content/docs/producer/compile.md Outdated
@altaranenco

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

- resolve_hermes_root(): expanduser().resolve(strict=False) so traversal
  segments in $HERMES_HOME cannot create stray intermediate dirs on
  mkdir(parents=True); aligns with TargetProfile.for_scope normalization.
- _to_hermes_format(): only emit url/command when truthy so a malformed
  entry never serializes as url: null / command: null into Hermes config;
  add regression tests for both remote-without-url and stdio-without-command.
- target_detection: fix '~/.hermes/ skills' typo -> '~/.hermes/skills/'.
- compile.md: fix relative link depth ../../integrations -> ../integrations.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@altaranenco

Copy link
Copy Markdown
Contributor Author

Status update

All four Copilot review findings are addressed in 49799aa (path normalization, null-key guards for malformed MCP entries + regression tests, doc typo, and the relative-link depth fix). The earlier inline comments are on the first commit (bd4fa65) and predate the fix.

Local CI mirror is green on the latest commit: ruff check, ruff format --check, pylint R0801 (10.00/10), and the auth-signals guard all pass; the new unit + integration suites pass.

The CI / CodeQL / Merge Gate / Spec conformance workflows currently show action_required (first-time fork contributor) - they need a maintainer to approve the workflow runs. Happy to address anything that surfaces once they run.

@altaranenco

Copy link
Copy Markdown
Contributor Author

Live validation against a real Hermes deployment

Validated end to end against a real Hermes Agent v0.16.0 install (Docker), using this branch's APM build. The proof below is deterministic and secret-free, so a maintainer can reproduce it. No credentials are required for steps 1-7.

Hermes: Hermes Agent v0.16.0 (2026.6.5)
APM:    Agent Package Manager (APM) CLI version 0.19.0

# 1) BEFORE apm install - our skill is absent (only Hermes builtins exist)
   /root/.hermes/skills/hello-hermes : ABSENT

# 2) apm install --target hermes --global
   1 skill(s) integrated -> .hermes/skills/
   Successfully configured MCP server 'livetest-time' for Hermes
   Configured 1 server
   Installed 1 APM dependency and 1 MCP server in 0.8s

# 3) Provenance - deployed file is byte-identical to the APM package source
   APM source sha256 : abc7763c5b7332781558c961f0d0eed4b9160bbe17e9649540362fdaf644c245
   deployed   sha256 : abc7763c5b7332781558c961f0d0eed4b9160bbe17e9649540362fdaf644c245
   MATCH -> came from the APM package, unmodified

# 4) Hermes itself tags the APM skill as source=local (vs builtin)
   Name           Source    Trust    Status
   hello-hermes   local     local    enabled    <- deployed by APM
   hermes-agent   builtin   builtin  enabled    <- Hermes builtin

# 5) apm compile -t hermes -> AGENTS.md carries the instruction marker
   AGENTS.md : marker present

# 6) MCP written into ~/.hermes/config.yaml (sibling keys preserved)
   mcp_servers.livetest-time: {'command': 'uvx', 'args': ['mcp-server-time', '--local-timezone=UTC'], 'enabled': True}
   sibling top-level keys preserved: True

# 7) Hermes lists the APM-written MCP server as enabled
   Name            Transport                     Tools   Status
   livetest-time   uvx mcp-server-time --loc...  all     enabled

Full agent round-trip (model + messaging gateway)

Beyond the filesystem assertions, the APM-deployed skill was exercised through a live Hermes messaging gateway driving a real LLM turn. A task that matches the skill returned its exact, skill-defined marker, and Hermes' own trace shows it loaded the skill:

skill_view: "hello-hermes"
APM-HERMES-LIVETEST-OK: hello from the APM-deployed skill, Alexey!

That string is defined only inside the deployed SKILL.md, so its appearance confirms the full path: apm install -> ~/.hermes/skills/ -> Hermes skill loader -> model.

Notes

  • Backend model credentials and messaging tokens used for the round-trip are kept private and are not part of this PR. The Docker live-test harness is a local/session artifact, not committed.
  • Steps 1-7 require no credentials and reproduce the skills + MCP + AGENTS.md deployment deterministically.

@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 10, 2026
@github-actions

Copy link
Copy Markdown

APM Review Panel: needs_rework

NEEDS REWORK: HermesClientAdapter writes credentials world-readable (0o644); fix to 0o600 required before merge -- all other findings are post-merge follow-ups on a well-structured integration

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The supply-chain-security-expert's blocking finding is unambiguous and load-bearing: HermesClientAdapter writes credential-bearing YAML to ~/.hermes/config.yaml via open(path, 'w') under the process umask, producing world-readable files on typical Linux/macOS (0o644). Every peer adapter -- Claude, Codex, Cursor, Gemini -- applies 0o600 at write time via atomic_write_text(new_file_mode=0o600) or an explicit os.chmod call. This is not an opinion finding; it is a verifiable divergence from the security contract every other adapter upholds. The auth-expert's recommended finding compounds the risk: when config.yaml is malformed, _load_document() returns {} and update_config() proceeds, silently overwriting and discarding all non-MCP credentials -- model-provider API keys, Telegram tokens -- contrary to its own docstring promise of key preservation. Both must be resolved before merge; both are one-to-three line changes with a clear, established fix pattern already in the codebase.

The test-coverage-expert returned two missing integration tests at the required tier (integration-with-fixtures) for surfaces that carry real operational stakes. No test currently invokes apm compile -t hermes as a CLI command, leaving the AGENTS.md routing claim unverified end-to-end; the existing test_hermes_compiles_agents_md is a static function-call assertion, not a CLI invocation. No test verifies that _hermes_runtime_opted_in() correctly gates the config write when the flag is enabled but Hermes is absent -- exactly the scenario that fires for a user who opts in on a host without Hermes installed. Both findings carry outcome: missing on governed-by-policy and secure-by-default surfaces respectively, ranking them above opinion-only findings. They do not determine the ship stance but must ship as filed issues in the same milestone.

Aside from the security rework, this is a clean, well-scoped integration. The python-architect found one OCP violation in TargetProfile.for_scope name-dispatch -- genuine design debt addressable via a single home_env_var dataclass field and suitable for a follow-up issue. The devx-ux-expert found three reference doc gaps (install.md, experimental.md, targets-matrix.md) and a hint-text ambiguity conflating apm install with apm compile for AGENTS.md -- the latter is a concrete user confusion risk worth folding into the security rework PR. The oss-growth-hacker correctly identifies that Hermes opens a net-new community-chat-native persona segment; two one-line changes (a tip callout in hermes.md and a README hero-line append) unlock the growth signal at negligible cost.

Dissent. No panelist challenged the blocking classification of the 0o600 finding; it stands uncontested. The auth-expert and supply-chain-security-expert converged on the same _load_document() parse-error gap from independent angles -- auth (silent credential loss) and security (yaml.YAMLError propagates uncaught, preventing the rewrite-from-scratch guard from ever firing). The fix is the same: add yaml.YAMLError to the except clause and abort on empty-document rather than proceeding. Performance-expert's lru_cache recommendation for _hermes_runtime_opted_in() is correctly scoped as deferred given the current single call site.

Aligned with: Portable by manifest (MET -- SKILL.md and AGENTS.md already emitted by APM, zero new format for producers), Multi-harness multi-host (MET -- registry-first adapter + target-profile patterns followed correctly), OSS community-driven (OPPORTUNITY -- Nous Research Discord/Telegram-native persona is a net-new segment for APM), Governed by policy (MET with gap -- flag gate present but integration test missing at required tier), Secure by default (VIOLATED -- 0o600 permission gap and parse-error abort missing in HermesClientAdapter), Pragmatic as npm (AT RISK -- silent full-config loss on malformed YAML contradicts the 'it just works' expectation).

Growth signal. Hermes (Nous Research) is APM's first integration reaching community-chat-native AI -- agents that live in Telegram and Discord, not just the terminal. The zero-format-migration story (Hermes natively reads SKILL.md and AGENTS.md, which APM already emits) is the sharpest adoption hook available and costs the community nothing to adopt. Distribution channels to activate at ship: Nous Research Discord, agentskills.io community forum, AGENTS.md ecosystem threads. Two one-line changes maximize the launch signal: promote 'zero new format' to a tip callout immediately below the caution block in hermes.md, and append Hermes (experimental) to the README hero target list.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 3 Clean registry-first extension following established adapter and target-profile patterns; one OCP violation in TargetProfile.for_scope name-dispatch is the only substantive finding.
CLI Logging Expert 0 1 2 ASCII-clean, credential-safe, enable-hint through CommandLogger; catch-all exception suppresses diagnostic type, and rich* call-sites omit symbol= prefix.
DevX UX Expert 0 4 2 Strong integration fundamentals, but three reference docs are incomplete and the post-enable hint misstates that apm install deploys AGENTS.md.
Supply Chain Security Expert 1 1 1 Literal credentials land in config.yaml without 0o600 protection -- every other adapter with the same threat model applies it; rework required.
OSS Growth Hacker 0 4 1 Genuine novel persona-expansion story (community-chat-native AI) and zero-format-migration proof point; guide is written for APM users rather than inbound Hermes users.
Auth Expert 0 2 1 No blocking auth issues; two recommended gaps: expanduser() absent in for_scope() and full-config loss on YAML parse error.
Doc Writer 0 1 3 New hermes.md is structurally sound and code-accurate; external product URL unverified, deprecated --target all referenced, sidebar.order duplicated.
Test Coverage Expert 0 2 1 Adapter unit and install-pipeline integration tests are strong; two gaps: apm compile -t hermes lacks a CLI-level test and _hermes_runtime_opted_in() double-gate is untested.
Performance Expert 0 1 3 Zero I/O overhead when flag is OFF; _hermes_runtime_opted_in() is one @lru_cache away from being permanently safe against future double-call.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Supply Chain Security Expert] (blocking-severity) Replace dump_yaml call in update_config with atomic_write_text(path, payload, new_file_mode=0o600) -- Every peer adapter (Claude, Codex, Cursor, Gemini) enforces 0o600; HermesClientAdapter inherits OS-default umask (0o644). Literal credentials in a world-readable file is a concrete, verifiable exposure surface. The fix is one line and the pattern is already in the codebase.
  2. [Auth Expert] Return False from update_config when _load_document fails to parse, and add yaml.YAMLError to the _load_document except clause -- Parse failure today silently overwrites config.yaml with only the MCP block, discarding model-provider API keys and Telegram tokens. The docstring promises non-MCP key preservation; the implementation breaks it. load_yaml() raises yaml.YAMLError, not ValueError or OSError, so the guard never fires for the most common real-world failure.
  3. [Test Coverage Expert] Add CLI-level integration test for apm compile -t hermes asserting AGENTS.md is written to the project root -- Missing at integration-with-fixtures tier on a governed-by-policy surface. The current test is a static function-call assertion, not a CLI invocation; the compile routing promise is unverified end-to-end.
  4. [Test Coverage Expert] Add integration test for _hermes_runtime_opted_in() double-gate: flag-on + hermes-absent must not write config.yaml -- Missing at integration-with-fixtures tier on a secure-by-default surface. Verifies the exact scenario a user triggers when enabling the experimental flag on a host without Hermes installed.
  5. [DevX UX Expert] Fix ExperimentalFlag.hint to distinguish apm install (skills only) from apm compile -t hermes (AGENTS.md), and add hermes to install.md, experimental.md, and targets-matrix.md -- The hint text today tells users that apm install deploys AGENTS.md; it does not. Three reference doc gaps mean a user who discovers hermes via apm experimental list cannot verify it against any reference page.

Architecture

classDiagram
    direction LR
    class MCPClientAdapter {
        <<Abstract>>
        +_fetch_server_info(url, cache) dict
        +_determine_config_key(url, name) str
        +configure_mcp_server(url, name) bool
    }
    class CopilotClientAdapter {
        +_client_label str
        +mcp_servers_key str
        +_format_server_config(info, env, vars) dict
        +get_current_config() dict
        +update_config(updates, enabled) bool
    }
    class HermesClientAdapter {
        +target_name str
        +mcp_servers_key str
        +_supports_runtime_env_substitution bool
        +_to_hermes_format(entry, enabled) dict
        +_config_path() Path
        +_load_document() dict
        +get_current_config() dict
        +update_config(updates, enabled) bool
        +configure_mcp_server(url) bool
    }
    class ClaudeClientAdapter {
        +target_name str
        +mcp_servers_key str
    }
    class TargetProfile {
        <<ValueObject>>
        +name str
        +root_dir str
        +user_root_dir str
        +requires_flag str
        +compile_family str
        +for_scope(user_scope) TargetProfile
    }
    class PrimitiveMapping {
        <<ValueObject>>
        +subdir str
        +extension str
        +format_id str
    }
    class ClientFactory {
        <<Factory>>
        +create_client(name) MCPClientAdapter
    }
    MCPClientAdapter <|-- CopilotClientAdapter
    CopilotClientAdapter <|-- HermesClientAdapter
    CopilotClientAdapter <|-- ClaudeClientAdapter
    ClientFactory ..> HermesClientAdapter : creates
    TargetProfile *-- PrimitiveMapping : primitives
    class HermesClientAdapter:::touched
    class TargetProfile:::touched
    class ClientFactory:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A["apm install --target hermes --global"] --> B["install/phases/targets.run()"]
    B --> C["_resolve_targets_legacy\nintegration/targets.resolve_targets\nuser_scope=True, explicit=hermes"]
    C --> D["KNOWN_TARGETS[hermes]\n_flag_gated -> is_enabled(hermes)\ncore/experimental.py"]
    D -- "flag OFF" --> E["target not resolved, empty list"]
    D -- "flag ON" --> F["TargetProfile.for_scope(user_scope=True)\nreads HERMES_HOME\nintegration/targets.py"]
    F --> G["scope-resolved TargetProfile\nroot_dir = .hermes or abs(HERMES_HOME)"]
    B --> H["_check_hermes_flag_gate\n-> _check_experimental_target_hint\ninstall/phases/targets.py"]
    H -- "flag OFF + hermes in explicit" --> I["emit: apm experimental enable hermes"]
    G --> J["SkillIntegrator.deploy\nwrite ~/.hermes/skills/name/SKILL.md"]
    B --> K["mcp_integrator_install._hermes_runtime_opted_in\nis_enabled(hermes) AND resolve_hermes_root().is_dir\nprobe ~/.hermes/"]
    K -- "opted in" --> L["HermesClientAdapter.configure_mcp_server\nadapters/client/hermes.py"]
    L --> M["_fetch_server_info(url, cache)\nregistry HTTP\ninherited from MCPClientAdapter"]
    M --> N["_format_server_config(info, env, vars)\ninherited from CopilotClientAdapter\nreturns Copilot-format dict"]
    N --> O["update_config({key: copilot_dict})\nhermes.py:update_config"]
    O --> P["_load_document\nload_yaml config.yaml\nutils/yaml_io.py"]
    O --> Q["_to_hermes_format(copilot_dict)\ndrops type/tools/id, stamps enabled"]
    Q --> R["dump_yaml config.yaml\nutils/yaml_io.py\nMISSING: 0o600 permission"]
Loading

Recommendation

Do not merge until the 0o600 permission fix and the parse-error abort path are both in place. The security gap is concrete, the fix pattern is established by every peer adapter, and there is no trade-off to weigh against fixing it. Once those two are green, fold in the hint-text correction (apm install vs. apm compile for AGENTS.md) and verify the external Nous Research URL, then ship. File the OCP refactor, the three reference doc gaps, and the two missing integration tests as issues in the same milestone. The Hermes integration is a well-architected addition with a genuine growth story and a zero-format-migration proof point that no existing target can match -- the only thing between it and merge is a permissions flag and an abort guard.


Full per-persona findings

Python Architect

  • [recommended] TargetProfile.for_scope name-dispatches on self.name instead of reading a dataclass field at src/apm_cli/integration/targets.py:390
    for_scope() contains if self.name in ('claude', 'hermes') to pick the correct env-var name (CLAUDE_CONFIG_DIR vs HERMES_HOME). Encoding identity-based branching inside a generic method violates OCP: a third tool with its own home env var requires editing for_scope again. The fix is a single optional home_env_var: str | None = None field on TargetProfile.
    Suggested: Add home_env_var: str | None = None to TargetProfile. Set home_env_var='CLAUDE_CONFIG_DIR' on claude and home_env_var='HERMES_HOME' on hermes in KNOWN_TARGETS. In for_scope replace the name-dispatch with if self.home_env_var: env = os.environ.get(self.home_env_var, '').strip().

  • [nit] _client_label not overridden; HermesClientAdapter silently inherits 'Copilot CLI' at src/apm_cli/adapters/client/hermes.py
    Suggested: Add _client_label: str = 'Hermes' alongside the existing class attributes.

  • [nit] _check_hermes_flag_gate and _check_openclaw_flag_gate are needless one-liner wrappers at src/apm_cli/install/phases/targets.py
    Suggested: Call _check_experimental_target_hint directly from run() with named arguments and remove the two wrapper functions.

CLI Logging Expert

  • [recommended] catch-all except Exception emits zero diagnostic signal even though exception type is credential-safe to log at src/apm_cli/adapters/client/hermes.py:159
    The exception TYPE (OSError, ValueError, ConnectionError) carries no credentials and would let users self-diagnose disk-full, permission-denied, or network failures.
    Suggested: except Exception as exc: _rich_error(f'Error configuring MCP server ({type(exc).__name__})', symbol='error')

  • [nit] All five rich* call-sites omit symbol= so messages render with no prefix on non-color terminals at src/apm_cli/adapters/client/hermes.py
    Suggested: Add symbol='error', symbol='success', symbol='warning' to each call-site respectively.

  • [nit] 'Successfully configured' wording is redundant -- _rich_success already signals success at src/apm_cli/adapters/client/hermes.py:157
    Suggested: _rich_success(f"Configured MCP server '{config_key}' for Hermes", symbol='success')

DevX UX Expert

  • [recommended] --target help text in install.md and install.py omits hermes from the experimental target list at docs/src/content/docs/reference/cli/install.md
    Suggested: Extend the experimental target list to include hermes (and openclaw).

  • [recommended] reference/experimental.md Available Flags table does not list hermes at docs/src/content/docs/reference/experimental.md
    Suggested: Add a hermes row: | 'hermes' | Deploy skills, AGENTS.md, and MCP servers to the Hermes agent. |

  • [recommended] reference/targets-matrix.md is missing a hermes (experimental) section at docs/src/content/docs/reference/targets-matrix.md
    Suggested: Add a section parallel to the openclaw section covering detection (none), enable command, deploy directories, and supported primitives.

  • [recommended] ExperimentalFlag.hint for hermes conflates apm install with apm compile for AGENTS.md at src/apm_cli/core/experimental.py
    The hint says 'deploy skills + AGENTS.md' but install deploys only skills; AGENTS.md requires apm compile -t hermes.
    Suggested: 'Use --target hermes to deploy skills to your project (.agents/skills/), then run apm compile -t hermes to emit AGENTS.md. Use --target hermes --global for ~/.hermes/ skills + MCP servers in config.yaml.'

  • [nit] MCP servers doc section doesn't call out the mcp_servers vs mcpServers key difference at docs/src/content/docs/integrations/hermes.md

  • [nit] Troubleshooting entry for skills.external_dirs tells users what to do but not how at docs/src/content/docs/integrations/hermes.md

Supply Chain Security Expert

  • [blocking] update_config writes credential-bearing YAML with OS-default (world-readable) permissions at src/apm_cli/adapters/client/hermes.py
    ClaudeClientAdapter uses atomic_write_text(new_file_mode=0o600); Codex, Cursor, Gemini all call os.chmod(config_path, 0o600). HermesClientAdapter does neither. Literal secrets written with umask-default permissions (0o644) are readable by any process that can access $HOME.
    Suggested: Use atomic_write_text(path, yaml_to_str(data), new_file_mode=0o600) in update_config.

  • [recommended] _load_document catches (OSError, ValueError) but not yaml.YAMLError, so the 'rewrite from scratch' guard never fires for malformed YAML at src/apm_cli/adapters/client/hermes.py
    Suggested: Change except (OSError, ValueError): to except (OSError, ValueError, yaml.YAMLError): in _load_document.

  • [nit] _to_hermes_format returns non-dict input as-is without stamping enabled at src/apm_cli/adapters/client/hermes.py

OSS Growth Hacker

  • [recommended] Integration guide opens by describing Hermes to APM users rather than answering 'why APM?' for an inbound Hermes user at docs/src/content/docs/integrations/hermes.md
    Suggested: Add a 'Why APM for Hermes users?' opener: APM gives Hermes a manifest, lockfile, and transitive dependency resolution for the SKILL.md / AGENTS.md content Hermes already reads.

  • [recommended] Zero-new-format proof point is buried mid-paragraph rather than called out as the headline at docs/src/content/docs/integrations/hermes.md
    Suggested: Promote 'Hermes natively reads two open standards that APM already emits' to a :::tip callout immediately below the caution block.

  • [recommended] README hero line omits Hermes, missing a free ecosystem-momentum signal at README.md
    Suggested: Append Hermes (experimental) to the target list on the README hero line.

  • [recommended] Hermes' Telegram/Discord-native persona is mentioned descriptively but not framed as a new community-native user segment APM is now reaching at docs/src/content/docs/integrations/hermes.md

  • [nit] CHANGELOG entry is technically accurate but doesn't surface the zero-format-migration story as a user-facing benefit at CHANGELOG.md

Auth Expert

  • [recommended] expanduser() absent in for_scope() but present in resolve_hermes_root() -- tilde-style HERMES_HOME causes skills-path / MCP-write path divergence at src/apm_cli/integration/targets.py
    resolve_hermes_root() calls Path(env).expanduser().resolve(); for_scope() calls Path(env).resolve() directly. If HERMES_HOME=~/foo, skills and MCP config land in different roots.
    Suggested: Change Path(env).resolve() to Path(env).expanduser().resolve() for the hermes branch in for_scope().

  • [recommended] _load_document() returns {} on parse error and update_config() silently overwrites full config.yaml, discarding non-MCP credentials at src/apm_cli/adapters/client/hermes.py
    The docstring promises 'all other top-level config keys are preserved'; the implementation breaks this guarantee on parse failure, discarding model-provider API keys, Telegram tokens, and any other native Hermes config.
    Suggested: When _load_document() fails to parse, have update_config() return False with error: 'config.yaml is malformed; refusing to overwrite -- fix or remove the file manually.'

  • [nit] configure_mcp_server bare except Exception masks programming errors (AttributeError, TypeError from refactors) at src/apm_cli/adapters/client/hermes.py

Doc Writer

  • [recommended] External product link (hermesagent.nousresearch.com/redacted) is unverified at docs/src/content/docs/integrations/hermes.md
    Suggested: Verify the URL resolves to the canonical Nous Research Hermes product page before merge; supplement with a GitHub repo URL if available.

  • [nit] sidebar.order: 6 in hermes.md duplicates copilot-app.md's order: 6 at docs/src/content/docs/integrations/hermes.md
    Suggested: Set sidebar.order: 8 (next free slot after external-scanners.md at 7).

  • [nit] Caution block says 'excluded from --target all' -- references a deprecated install flag at docs/src/content/docs/integrations/hermes.md
    Suggested: Replace with 'excluded from apm compile --all'.

  • [nit] Install code-block comment conflates both commands into one line, obscuring that AGENTS.md requires apm compile, not apm install at docs/src/content/docs/integrations/hermes.md

Test Coverage Expert

  • [recommended] apm compile -t hermes is not integration-tested; test_hermes_compiles_agents_md is a static function-call assertion, not a CLI invocation at tests/integration/test_hermes_target.py
    Per tier floor matrix, CLI command surface requires integration-with-fixtures.
    Proof (missing at integration-with-fixtures): tests/integration/test_hermes_target.py::TestHermesCompileE2E::test_compile_hermes_emits_agents_md -- proves: apm compile -t hermes routes through the agents compile family and writes AGENTS.md to the project root [governed-by-policy]

  • [recommended] _hermes_runtime_opted_in() double-gate (flag-enabled AND hermes-present) has no test at any tier at tests/integration/test_mcp_install_flow.py
    No test verifies: (a) flag-on + home-dir-absent + no-binary -> excluded from discovered runtimes; (b) flag-on + home-dir-exists -> included; (c) flag-off -> excluded regardless.
    Proof (missing at integration-with-fixtures): tests/integration/test_mcp_install_flow.py::TestHermesMCPOptIn::test_flag_on_no_presence_skips_mcp_write -- proves: APM does not write ~/.hermes/config.yaml on a host where Hermes is absent even when the experimental flag is enabled [secure-by-default]

  • [nit] _CROSS_TARGET_MAPS['hermes'] has no assertion; openclaw parallel entry has TestOpenclawCrossTargetMap at tests/integration/test_hermes_target.py
    Proof (missing at unit): tests/integration/test_hermes_target.py::TestHermesCrossTargetMap::test_cross_target_map_remaps_github_skills

Performance Expert

  • [recommended] _hermes_runtime_opted_in() is not memoized, leaving a latent shutil.which double-call risk at src/apm_cli/integration/mcp_integrator_install.py
    If a second call site is added with the flag ON and ~/.hermes/ absent, shutil.which('hermes') executes twice (5-200ms per call). One @lru_cache decorator fixes this permanently.
    Suggested: Add @functools.lru_cache(maxsize=None) to _hermes_runtime_opted_in().

  • [nit] Two-stage probe order (is_dir before find_runtime_binary) is correct -- stat before shutil.which at src/apm_cli/integration/mcp_integrator_install.py
    No change needed.

  • [nit] Flag-OFF fast path costs exactly 2 dict lookups -- zero I/O for the 99%+ of users without hermes enabled at src/apm_cli/integration/mcp_integrator_install.py
    Correctly implemented.

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Generated by PR Review Panel for issue #1726 · sonnet46 23.7M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 10, 2026
Address APM Review Panel needs_rework on PR microsoft#1726:

- BLOCKING (supply-chain-security): write ~/.hermes/config.yaml atomically
  with 0o600 via atomic_write_text + os.chmod, so the credential-bearing
  config is never left group/world-readable (parity with claude/codex/
  gemini/cursor). Tightens pre-existing loose files too.
- auth-expert: _load_document now raises on a malformed/non-mapping existing
  config (incl. yaml.YAMLError, previously uncaught); update_config refuses
  to overwrite and returns False instead of silently discarding native
  Hermes credentials (model-provider keys, Telegram tokens).
- docs: fix sidebar.order collision (6 -> 8) and stale --target all ->
  apm compile --all in the integration guide.

Tests: +6 unit (0o600 new/loose-file, refuse-on-malformed, non-mapping,
empty-file) and +9 integration (compile -t hermes emits AGENTS.md,
_hermes_runtime_opted_in double-gate x4, cross-target map x2).

Lint: ruff + pylint R0801 (10.00/10) + auth-signals all green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
auto-merge was automatically disabled June 10, 2026 19:41

Head branch was pushed to by a user without write access

@altaranenco

Copy link
Copy Markdown
Contributor Author

Addressed APM Review Panel needs_rework (commit ff588c5)

Thanks for the thorough pass. The blocking finding and the high-value recommendations are fixed; the remainder is itemized with disposition.

Blocking -- FIXED

supply-chain-security: world-readable credential file. update_config() now serializes via utils.yaml_io.yaml_to_str and writes through atomic_write_text(..., new_file_mode=0o600), then os.chmod(path, 0o600) so an existing looser file is tightened too (atomic write only sets mode on create). This matches the claude/codex/gemini/cursor contract.

Live proof on a real Hermes v0.16.0 deploy (Docker). Hermes' own installer ships config.yaml at 644; after an APM MCP write it is 600:

pre-existing config.yaml perms (Hermes installer): 644
apm install --target hermes --global:
    Successfully configured MCP server 'livetest-time' for Hermes
SECURITY ASSERTION: config.yaml mode=600  [+] PASS (0o600)
    mcp_servers.livetest-time present: 1

Recommended -- FIXED

  • auth-expert: malformed config silently discarded. _load_document() now raises on a non-mapping / unparseable existing file (incl. yaml.YAMLError, previously uncaught by except (OSError, ValueError)), and update_config() refuses the write (return False) instead of clobbering native Hermes credentials. Live:
    config.yaml is malformed YAML; refusing to overwrite. Fix or remove the file manually, then retry.
        [+] PASS malformed file untouched
        [+] PASS native credential preserved
    
  • test-coverage-expert (all three):
    • TestHermesCompileE2E::test_compile_hermes_emits_agents_md -- real apm compile -t hermes CliRunner invocation, asserts AGENTS.md written with instruction content.
    • TestHermesMCPOptIn (x4) -- _hermes_runtime_opted_in() double gate: flag-off skips regardless of presence; flag-on + no home + no binary skips; flag-on + home-dir opts in; flag-on + binary-only opts in.
    • TestHermesCrossTargetMap (x2) -- asserts _CROSS_TARGET_MAPS['hermes'] remaps .github/skills/ -> .agents/skills/.
  • doc-writer nits: sidebar.order 6 -> 8 (was colliding with copilot-app.md); stale --target all -> apm compile --all.

Already fixed in 49799aa (stale on bd4fa65)

  • auth-expert: for_scope() missing expanduser(). targets.py:399 already calls Path(env).expanduser().resolve(strict=False) for the hermes branch (same normalization as resolve_hermes_root()); skills + MCP roots cannot diverge on a tilde HERMES_HOME.

Deferred, with rationale (non-blocking)

  • performance-expert: @lru_cache on _hermes_runtime_opted_in(). Intentionally not memoized: it reads mutable process state (the experimental flag and ~/.hermes presence) that legitimately changes within a run (tests toggle the flag; a user may create the home dir mid-session). Caching would trade a rare double shutil.which (flag-ON + home-absent only) for a stale-result correctness bug. The flag-OFF fast path the panel verified is already zero-I/O.
  • auth nit: bare except Exception in configure_mcp_server. Kept deliberately: the network/registry fetch can raise many exception types and the handler must not interpolate the message (registry URLs may embed credentials). Documented in-line.
  • growth/doc recommendations (README hero, "why APM?" opener, tip callout, external-link verification): positioning polish, queued as a follow-up so this PR stays scoped to the security/correctness fixes.

Validation

ruff check + ruff format --check clean; pylint R0801 10.00/10; bash scripts/lint-auth-signals.sh clean. New: 6 unit + 9 integration tests; full hermes/targets/detection suites 244 passed, mcp-install + bundle suites 65 passed.

All live evidence is deterministic and secret-free (secrets stay host-local in a 0600 env file, injected via --env-file); nothing sensitive is committed.

@sergio-sisternes-epam sergio-sisternes-epam added this pull request to the merge queue Jun 10, 2026
Merged via the queue into microsoft:main with commit 23b0518 Jun 10, 2026
15 checks passed
sergio-sisternes-epam pushed a commit that referenced this pull request Jun 13, 2026
Sync the Stage 2 complexity/file-length refactor branch with main's 22
feature commits (Hermes #1726, Kiro IDE #1741, multi-host dep identity
#1735, same-repo remote path deps #1732, git_file_transport #1740,
revision pins #1738, marketplace sourceBase/source parity/inherit
description #1736/#1739/#1755, pack --archive .zip #1720, mcp optional
registry inputs #1734, and the v0.19.0/v0.20.0 releases).

Conflict resolution preserved both sides: main's new features ported
through the branch's extracted sibling modules, branch's tightened ruff
thresholds (max-statements=120, max-branches=40, max-complexity=35,
max-returns=8, max-args=12) and 800-line file limit retained.

All 7 CI-mirror lint gates pass; full unit suite green (17099 passed).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants