feat(providers): registration seam — a provider can ship its own client from outside the tree (part 2 of the #74743 split) - #40
Conversation
૮ >ﻌ< ა ci reviewrunning on 6bfcf62 — fix(providers): discover a provider plugin installed by `her Still running 2 jobs: ❌ Job failuresCheck contributors / check-attribution · View jobJob Check contributors / check-attribution failed.
|
37972be to
7e823b8
Compare
|
Added a third commit — While checking how the plugin index actually installs things I found that the two parts of the documented path don't meet:
and Verified before the fix: So That matters here beyond correctness: an index listing plus one-command install is exactly the distribution path this whole split is for. Without this commit, parts 1 and 2 land and Junie still can't be installed the documented way. 12 new tests. Discovery imports only Regression: |
``create_openai_client`` was a hardcoded if-ladder: copilot-acp builds an ACP
stdio shim, gemini builds a native client, everything else gets an
``openai.OpenAI``. There was no extension point, so a provider whose wire
protocol is not OpenAI-over-HTTP could only be added by editing this function —
which is exactly why an ACP provider cannot ship outside this tree today, even
though ``providers/__init__.py`` has discovered out-of-tree profiles from
``~/.hermes/plugins/model-providers/`` and pip entry points for a while.
``ProviderProfile.create_client(**client_kwargs)`` closes that gap. It returns
``None`` by default, so every provider that wants the standard client is
unaffected and the existing ladder still runs as the fallback. copilot-acp is
migrated onto it — its hardcoded branch is gone and its profile supplies the
client in three lines, which is the same three lines an external package writes.
Resolution goes by provider name first, then by ``base_url`` prefix, so a
runtime configured only by URL still reaches its profile — matching what the
replaced ``startswith("acp://copilot")`` branch did. A profile that raises is
logged and skipped: a third-party plugin can fail to provide a client, but it
cannot take the turn down.
Also replaces the two ``isinstance`` checks in ``agent/auxiliary_client.py``
that mean "this client is complete, do not wrap it" with capability flags the
client class declares — ``HERMES_SKIP_TRANSPORT_WRAP`` and
``HERMES_SKIP_ASYNC_WRAP``, mirroring ``SUPPORTS_HERMES_TOOL_CALLS`` in
``background_review.py``. Two in-tree consumers (the ACP shim and the Gemini
native client), an out-of-tree client is covered by the same declaration, and
the hot path no longer imports those modules just to type-test.
Co-Authored-By: Junie <junie@jetbrains.com>
An external-process provider is an agent CLI Hermes drives over stdio rather than an HTTP endpoint. Three things about it were spelled out for one vendor, and each was a hard stop for any other: * ``resolve_provider()`` gates on ``PROVIDER_REGISTRY``. Its auto-extend from ``providers/`` covered api-key providers only, so an external-process profile never entered it and ``hermes -m <that provider>`` died with "Unknown provider" before a client was ever built. * ``resolve_runtime_provider()`` keyed the external-process branch on the literal ``"copilot-acp"``, so anything else silently fell through to the OpenRouter default instead of its own runtime. * ``resolve_external_process_provider_credentials()`` hardcoded the binary (``copilot``), the argv (``--acp --stdio``), the env var names and the placeholder api_key — so a third-party provider would have been handed another vendor's CLI. Now the profile carries what only the provider knows — ``process_command``, ``process_args``, ``process_command_env_vars``, ``process_args_env_var`` — and the three core paths key on ``auth_type == "external_process"`` instead of a name. copilot-acp's values move into its profile verbatim, so ``HERMES_COPILOT_ACP_COMMAND`` / ``COPILOT_CLI_PATH`` / ``HERMES_COPILOT_ACP_ARGS`` and its ``copilot-acp`` api_key placeholder behave exactly as before; the new tests assert that alongside the out-of-tree case at every step. The error for a missing binary now names the provider and its own env override instead of telling every user to install GitHub Copilot CLI. Co-Authored-By: Junie <junie@jetbrains.com>
…ns install` `hermes plugins install owner/repo` — and the plugin index behind `hermes plugins search` — clones into `$HERMES_HOME/plugins/<name>/`, flat, one directory per plugin. Provider discovery only ever scanned `$HERMES_HOME/plugins/model-providers/<name>/`. Nothing joined the two. `PluginManager` does not close the gap either: it classifies `kind: model-provider` and deliberately skips importing it, because provider lifecycle is owned by `providers/__init__.py` — which was not looking in the directory the installer writes to. So the documented install path half-worked. The CLI reported success, wrote its install metadata, and the provider silently did not exist: `hermes -m <it>` said "Unknown provider" and `/model` never listed it. Verified before the fix — a plugin at `~/.hermes/plugins/<name>/` was NOT FOUND while the identical plugin at `~/.hermes/plugins/model-providers/<name>/` was discovered. Discovery now also walks the flat directory, importing only entries whose manifest declares `kind: model-provider`. Everything else there belongs to `PluginManager`, which owns its lifecycle and consent flow — importing it here would run third-party code behind its back, so the tests assert we don't (with fixtures that register on import, since a fixture that merely raised would be swallowed by `_import_plugin_dir` and prove nothing). Manifests are parsed with PyYAML when present and a line scan otherwise, so provider discovery gains no hard dependency; an unreadable manifest is skipped rather than allowed to blank the registry. Co-Authored-By: Junie <junie@jetbrains.com>
ea4075e to
6bfcf62
Compare
1 participant
TL;DR
This is the last piece of core work between us and "Junie installs into stock Hermes with
pip install". It adds no Junie code and no vendor names — it removes the three places where core hardcodes one vendor's ACP provider, so any provider can be registered from outside the tree.copilot-acpis migrated onto the new seam as the in-tree proof and its behaviour is unchanged, asserted step by step.Background — why this exists
Upstream rejected the Junie integration twice (NousResearch#69207 closed, NousResearch#74743 blocked), both times for the same reason:
AGENTS.md's June 2026 policy forbids new third-party-product plugins in the tree.copilot-acppredates that policy (March 2026) and is grandfathered; Junie is not. The agreed path (teknium1 on NousResearch#74743) is a three-part split:What was actually broken
I built a throwaway out-of-tree ACP provider (
acme-acp) in~/.hermes/plugins/model-providers/and walked it forward until it broke, three times. Each fix is keyed on a capability, never on a name.1. There was nowhere to put a client.
create_openai_client()inagent/agent_runtime_helpers.pyis a hardcoded if-ladder —copilot-acp→ ACP stdio shim,gemini→ native client, everything else →openai.OpenAI. No extension point. This is the blocker:providers/__init__.pyhas discovered out-of-tree profiles from~/.hermes/plugins/and pip entry points for a while, but a discovered profile could never supply a transport.→
ProviderProfile.create_client(**client_kwargs), returningNoneby default. Everything that wants the standard client is untouched and the existing ladder stays as the fallback. Resolution is by provider name first, then bybase_urlprefix, matching what the replacedstartswith("acp://copilot")branch did. A profile that raises is logged and skipped — a third-party plugin can fail to provide a client, it cannot take the turn down.2.
Unknown providerbefore anything was built.resolve_provider()gates onPROVIDER_REGISTRYinhermes_cli/auth.py. That registry already auto-extends itself fromproviders/— but only forauth_type == "api_key"providers with env vars. An external-process profile has neither, so it was skipped andhermes -m <provider>died at the gate.→ Auto-extend now also absorbs
external_processprofiles, aliases included.3. The provider was handed someone else's CLI.
resolve_external_process_provider_credentials()hardcoded the binary (copilot), the argv (--acp --stdio), the env var names (HERMES_COPILOT_ACP_*), the api_key placeholder and the error text. Separately,resolve_runtime_provider()keyed its external-process branch on the literal"copilot-acp", so any other provider fell through to the OpenRouter default instead of its own runtime.→ The profile now carries what only the provider knows —
process_command,process_args,process_command_env_vars,process_args_env_var— and both core paths key onauth_type == "external_process". copilot's values moved into its profile verbatim.Bonus: two
isinstance(CopilotACPClient)checks inagent/auxiliary_client.pymeaning "this client is complete, don't wrap it" became capability flags the client class declares —HERMES_SKIP_TRANSPORT_WRAPandHERMES_SKIP_ASYNC_WRAP, mirroringSUPPORTS_HERMES_TOOL_CALLSfrom part 1. Two in-tree consumers (the ACP shim andGeminiNativeClient), an out-of-tree client is covered by the same declaration, and the hot path no longer imports those modules just to type-test.What this means for Junie
Today Junie-over-ACP only runs for someone who installs our fork. That is the whole distribution problem: upstream is at 237k stars and ~7k commits/month, our
junie-acp-v2branch is 6248 commits behind and drifting, and every week of that gap is rebase debt we own forever.After this merges upstream, Junie's entire footprint in Hermes core becomes zero lines, and the whole integration is this, in our own repository:
Concretely that buys us:
pip installonto stock Hermes. No fork, no patched core.hermes_cli/plugin_index.py+hermes_cli/data/plugin_index.jsonis the real promotion surface — five entries today, no providers among them. Junie would be the first, and teknium1 already committed to promoting it.Compare with NousResearch#74743, where the same integration meant a
junie-acpliteral pasted next to thecopilot-acpliteral in 11hermes_clifiles — which is precisely what got it blocked.Still to do after this, none of it blocked by upstream: the client loses the ~470 lines that moved into core in part 1, and the three findings from the first review that were never about shape still apply to the standalone package —
hermes_subprocess_env(inherit_credentials=True)instead ofos.environ.copy(), behavioural settings inconfig.yamlinstead ofHERMES_JUNIE_ACP_*, andSKILL.mdunder their format standards.Verification
End to end, out of tree. A provider registered from
~/.hermes/plugins/model-providers/with zero core edits, walked through the real resolution path:29 new tests, and
copilot-acpis asserted alongside the out-of-tree case at every step — same binary, same argv, same api_key placeholder, same env vars, same client class.Mutation-checked. Reverting each production change fails the corresponding test. This caught a real gap on the first pass: my tests exercised the helper rather than
create_openai_clientitself, so disabling the call site stayed green. Fixed by adding tests against the real entry point.Regression:
scripts/run_tests.sh tests/hermes_cli/ tests/agent/ tests/run_agent/ tests/providers/→ 14793 passed, 5 failed. All five reproduce identically on a cleanupstream/mainworktree (port 9119 occupied on this machine, launchd/Linux-desktop specifics, a tmpdir-cleanup flake intest_codex_app_server_persist). No test fails here that does not fail onmain.Review notes — where I'd push back on myself
_profile_for_base_urlscans every registered profile. Only reached when the provider name did not resolve, so it is off the hot path, but it is O(providers) with no cache. Worth a look.create_clientas a method vs aclient_factoryfield. teknium1's wording was "client_factory/ provider-registration hook". I chose a method becausefetch_modelsnext to it is a method and every profile is written as a subclass. If upstream wants the field form, it is a small change — flagging it before they do.PROVIDER_REGISTRYare stored by reference (PROVIDER_REGISTRY[alias] is PROVIDER_REGISTRY[name]). That mirrors the existing api-key branch directly above, so it is consistent, but it does mean a mutation through an alias is visible through the canonical name.HERMES_SKIP_*naming. Negative-sense flags.SUPPORTS_HERMES_TOOL_CALLSfrom part 1 is positive-sense. Consistent with neither perfectly; I picked the phrasing that reads correctly at the call site.