Conversation
548c4b7 to
86b51c6
Compare
86b51c6 to
e741864
Compare
e741864 to
adca902
Compare
…eplus-providers # Conflicts: # tests/providers/test_plugin_discovery.py
…eplus-providers # Conflicts: # hermes_cli/main.py
kaluluosi
left a comment
There was a problem hiding this comment.
Review: PR #32549 — Add Volcengine and BytePlus providers
Thanks for contributing this — it's great to see Volcengine/BytePlus support coming to Hermes! The plugin structure is on the right track, but the implementation goes against the current plugin architecture in a few ways. Here's a detailed review with references to official docs.
1. Provider plugins should be self-contained — zero core-file edits
The official authoring guide (website/docs/developer-guide/model-provider-plugin.md, lines 72-84) explicitly states that after dropping the two plugin files, all integrations auto-wire with no other edits:
| Integration | What it gets |
|---|---|
| Credential resolution | Populated from profile |
--provider CLI flag |
Works automatically |
hermes model picker |
Appears automatically |
hermes doctor |
Health check auto-wired |
hermes setup |
Setup wizard auto-wired |
| URL reverse-mapping | Hostname → provider auto-detection |
| Auxiliary model | Uses default_aux_model |
| Runtime resolution | Correct base_url, api_key, api_mode |
This PR adds manual edits to several of these auto-wired layers:
hermes_cli/providers.py(+41 lines) — should be auto-wired from the profilehermes_cli/models.py(+49 lines) — should be auto-wiredhermes_cli/main.py(+6 lines) — should be auto-wiredagent/model_metadata.py(+9 lines) — should be auto-wired
Each of these is listed in the official guide as something that "auto-wires with no other edits." The manual interventions are unnecessary and create maintenance burden.
2. hermes_cli/ark_providers.py should not exist
The new shared module (hermes_cli/ark_providers.py, 112 lines) is imported by the plugins. This creates a dependency from plugins → core code, which breaks the plugin isolation principle.
The correct approach is to put all constants (base URLs, model lists, default models) directly in each plugin's __init__.py. ProviderProfile accepts all of these as constructor arguments — there's no need for a shared module. Compare the existing built-in plugins like novita (plugins/model-providers/novita/__init__.py, 28 lines) or zai — they are fully self-contained.
3. StaticArkProviderProfile subclass is unnecessary
ProviderProfile (defined in providers/base.py) already provides every field used here. Subclassing it adds indirection without adding value. All four plugins should instantiate ProviderProfile directly, same as every other built-in provider plugin.
4. The correct structure (for reference)
Each provider should be a self-contained directory with exactly two files, per AGENTS.md lines 786-809 and website/docs/developer-guide/model-provider-plugin.md lines 27-70:
plugins/model-providers/volcengine-coding-plan/
├── __init__.py # ProviderProfile + register_provider
└── plugin.yaml # kind: model-provider
No core-file edits. No shared modules. Here's a minimal example matching the official guide:
# __init__.py
from providers import register_provider
from providers.base import ProviderProfile
profile = ProviderProfile(
name="volcengine-coding-plan",
aliases=("volcengine_coding_plan", "volcengine-coding"),
display_name="Volcengine Coding Plan",
description="Volcengine Ark Coding Plan — Subscription Plan",
signup_url="https://console.volcengine.com/ark",
env_vars=("VOLCENGINE_CODING_PLAN_API_KEY", "VOLCENGINE_CODING_PLAN_BASE_URL"),
base_url="https://ark.cn-beijing.volces.com/api/coding/v3",
auth_type="api_key",
supports_health_check=False,
default_aux_model="doubao-seed-2.0-pro",
fallback_models=(
"doubao-seed-code",
"deepseek-v3.2",
"doubao-seed-2.0-code",
"doubao-seed-2.0-pro",
),
)
register_provider(profile)5. Also missing: Volcengine Agent Plan
Volcengine's official docs for Hermes Agent integration point to the Agent Plan endpoint (https://ark.cn-beijing.volces.com/api/plan/v3) — a subscription-based multimodal inference service with its own API key tier, separate from both Standard Ark and Coding Plan. This PR covers Coding Plan and Standard, but not Agent Plan. A fifth plugin for volcengine-agent-plan would complete the Volcengine provider family.
Summary
The plugin skeleton is correct, but the implementation is ~500 lines heavier than it needs to be. Removing the core-file edits and the shared module would make this a clean, maintainable contribution that follows the official plugin architecture.
There was a problem hiding this comment.
This file should not exist. Provider plugins must be self-contained — all constants (base URLs, model lists, default models) belong directly in each plugin's __init__.py. ProviderProfile accepts them all as constructor arguments. See website/docs/developer-guide/model-provider-plugin.md lines 27-70 for the correct directory structure: exactly two files per plugin, no shared core modules. Importing from a core module (hermes_cli.*) breaks the plugin isolation principle — plugins are discovered through lazy scanning, not through core imports.
There was a problem hiding this comment.
These manual edits to hermes_cli/main.py should not be necessary. The --provider CLI flag auto-wires from the registered profile. See official guide website/docs/developer-guide/model-provider-plugin.md integration table (lines 72-84), row 2: --provider CLI flag — "Accepts provider name." No core code changes needed to support a new provider.
There was a problem hiding this comment.
This custom setup flow should not be necessary. Per website/docs/developer-guide/model-provider-plugin.md integration table (lines 72-84), row 5: hermes setup — ACME_API_KEY appears in OPTIONAL_ENV_VARS and the setup wizard automatically. The plugin system handles credential collection and model selection without per-provider flow code. Same as novita, zai, stepfun — none of them added a custom setup flow.
There was a problem hiding this comment.
These manual edits to hermes_cli/models.py should not be necessary. The hermes model picker and CANONICAL_PROVIDERS auto-populate from registered ProviderProfile instances — model lists come from {base_url}/models (or fallback_models when live fetch fails), and provider ordering is handled by the plugin discovery system. See official guide integration table (lines 72-84), row 3: hermes model picker — "Appears in CANONICAL_PROVIDERS, model list fetched automatically." Existing built-in providers (novita, zai, stepfun, gmi) added zero lines to this file.
| # Finally, resolve bundled/user provider plugins. This keeps the | ||
| # provider-profile plugin system in sync with the newer provider resolver | ||
| # used for config validation and setup status display. | ||
| try: |
There was a problem hiding this comment.
This bridge from the plugin system to the legacy ProviderDef should not be necessary. The provider plugin system (providers/__init__.py) is the intended source of truth for model providers since v0.15. Rather than converting ProviderProfile → ProviderDef, the caller should use get_provider_profile() directly. Adding a translation layer to the old system just prolongs the dual-system maintenance burden. See AGENTS.md lines 786-809: provider plugins are the recommended path — the legacy hermes_cli/providers.py system exists only for back-compat and should not grow.
| @@ -27,16 +28,16 @@ def _collect_masked_input( | |||
| while True: | |||
| ch = read_char() | |||
| if ch == "": | |||
| write("\n") | |||
| write(_TERMINAL_NEWLINE) | |||
| raise EOFError | |||
| if ch in _ENTER_CHARS: | |||
| write("\n") | |||
| write(_TERMINAL_NEWLINE) | |||
| return "".join(value) | |||
| if ch == "\x03": | |||
| write("\n") | |||
| write(_TERMINAL_NEWLINE) | |||
| raise KeyboardInterrupt | |||
| if ch in _EOF_CHARS: | |||
| write("\n") | |||
| write(_TERMINAL_NEWLINE) | |||
There was a problem hiding this comment.
These line-ending changes (\n → \r\n) appear to be an unintentional Windows line-ending conversion — they are unrelated to the Volcengine/BytePlus provider feature. Please revert this file. This is another sign that the PR touches too many files: an editor config leak in an unrelated module (secret_prompt.py) ended up in the diff.
|
Thanks for the detailed provider implementation and the setup/test coverage. This is an automated hermes-sweeper review. The PR adds four new bundled Volcengine/BytePlus model-provider integrations under
Closed as not-planned per standing maintainer policy ( |
Summary
Fixes #29331
This adds first-class Volcengine Ark and BytePlus ModelArk support to Hermes. Each provider has two explicit setup paths:
The four paths use separate credentials and base URL overrides so a standard API key is never silently reused for a subscription plan key, even when both products are under the same vendor.
Details
/modelsfor these providers during setup.VOLCENGINE_BASE_URL,VOLCENGINE_CODING_PLAN_BASE_URL,BYTEPLUS_BASE_URL, andBYTEPLUS_CODING_PLAN_BASE_URL.Model catalogs
Direct API-key endpoints use provider-specific defaults as a starting point, but still let users enter any deployment/model ID manually:
doubao-seed-2-0-pro-260215seed-2-0-pro-260328Subscription Plan endpoints use static curated pickers because these plans expose a known coding catalog and should not depend on live
/modelsdiscovery during setup.Volcengine Subscription Plan defaults to
doubao-seed-2.0-proand includes:doubao-seed-codedeepseek-v3.2doubao-seed-2.0-codedoubao-seed-2.0-prodoubao-seed-2.0-liteminimax-m2.7glm-5.1kimi-k2.6deepseek-v4-prodeepseek-v4-flashBytePlus Subscription Plan defaults to
dola-seed-2.0-proand includes:bytedance-seed-codeglm-4.7gpt-oss-120bkimi-k2.5dola-seed-2.0-prodola-seed-2.0-litedola-seed-2.0-codeglm-5.1deepseek-v4-prodeepseek-v4-flashTests
./scripts/run_tests.sh tests/providers/test_plugin_discovery.py tests/hermes_cli/test_ark_providers.py tests/hermes_cli/test_api_key_providers.py tests/hermes_cli/test_prompt_api_key.py tests/hermes_cli/test_secret_prompt.py tests/hermes_cli/test_setup_model_provider.py tests/hermes_cli/test_models.py tests/hermes_cli/test_model_switch_custom_providers.py tests/hermes_cli/test_setup_ollama_cloud_force_refresh.py tests/hermes_cli/test_model_provider_persistence.py -- -q.venv/bin/ruff check hermes_cli/main.py hermes_cli/model_setup_flows.py hermes_cli/ark_providers.py tests/hermes_cli/test_ark_providers.py tests/hermes_cli/test_prompt_api_key.py tests/hermes_cli/test_secret_prompt.py tests/providers/test_plugin_discovery.pygit diff --cached upstream/main --checkGitHub CI is passing on the current PR head, including the split test matrix, e2e, lint, Nix, Docker, attribution, history, and supply-chain checks.