Skip to content

fix(acp): preserve explicit model providers - #84332

Open
khushal-jagota wants to merge 2 commits into
NousResearch:mainfrom
khushal-jagota:agent/hermes-acp-explicit-provider
Open

fix(acp): preserve explicit model providers#84332
khushal-jagota wants to merge 2 commits into
NousResearch:mainfrom
khushal-jagota:agent/hermes-acp-explicit-provider

Conversation

@khushal-jagota

Copy link
Copy Markdown

Summary

  • preserve every recognized explicit provider:model choice in Hermes ACP
  • keep provider auto-detection for unqualified model text and supported slash, custom, aggregator, and variant forms
  • add deterministic resolver-collision coverage and a no-turn session/set_model persistence regression

Root cause

ACP parsed an explicit provider correctly, then ran bare-model detection when the parsed provider matched the current provider. A catalog collision could therefore replace the provider that the client selected.

This builds on #59092 and preserves @BROCCOLO1D's original commit authorship.

Impact

ACP clients can select a provider-qualified model without Hermes silently routing the model through another provider. Unqualified selections retain automatic provider detection.

Validation

  • scripts/run_tests.sh tests/acp_adapter/ tests/hermes_cli/test_model_validation.py tests/hermes_cli/test_models.py — 84 passed
  • .venv/bin/ruff check . — passed
  • git diff --check — passed

@khushal-jagota
khushal-jagota marked this pull request as ready for review August 12, 2026 06:09
@khushal-jagota

Copy link
Copy Markdown
Author

Ready for maintainer review. This salvages #59092 with BROCCOLO1D's original commit and adds the real resolver-collision plus no-turn session/set_model regressions.

The fork workflows await maintainer approval:

Local verification passed 84 relevant tests, repository-wide Ruff checks, and git diff --check.

@alt-glitch alt-glitch added type/bug Something isn't working comp/acp Agent Communication Protocol adapter comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Aug 12, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #59092: both PRs prevent ACP session/set_model from re-detecting a recognized explicit provider:model selection and replacing it after parsing. #59092 is the earlier open canonical PR.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

PR: fix(acp): preserve explicit model providers

  1. Slash vs colon asymmetry (acp_adapter/server.py, _resolve_model_selection): parse_model_input_details treats any recognized provider:model as explicit regardless of the current provider, but the new slash branch only treats provider/model as explicit when the provider equals the current one. google:model while on anthropic switches provider; google/model while on anthropic falls through to detect_provider_for_model. Same user intent, different outcome. Either apply the colon semantics to the slash form too, or document why the slash form is current-provider-only.

  2. Slash support lands only in ACP, not the CLI (hermes_cli/models.py, parse_model_input): parse_model_input_details still only parses colons, so /model anthropic/model in the CLI won't get the slash handling the ACP resolver now has — two entry points, two syntaxes. If slash syntax is intended to be general, normalize it inside parse_model_input_details (a shared helper) so both callers get it and the duplicated slash logic in the ACP resolver disappears.

  3. is_known_provider_name depends on _KNOWN_PROVIDER_NAMES staying lowercase (models.py): the set is built from _PROVIDER_LABELS.keys() + _PROVIDER_ALIASES.keys() + {"openrouter", "custom"}; the new helper lowercases the input before membership, so any future label/alias key added with uppercase silently breaks lookups. A frozenset(name.lower() for ...) at construction would make the invariant structural. (The old inline check had the same property, so this is hardening, not a regression.)

  4. Good coverage: the collision fixture plus parametrized cases and the no-turn persistence test are exactly the regressions this needed. The openrouter_collision monkeypatch of a module-level function is clean.

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Aug 16, 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 comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants