feat(agent): add Upstage Solar as a model provider (internal review)#1
Open
minchang wants to merge 4254 commits into
Open
feat(agent): add Upstage Solar as a model provider (internal review)#1minchang wants to merge 4254 commits into
minchang wants to merge 4254 commits into
Conversation
minchang
force-pushed
the
feat/upstage-solar-provider
branch
20 times, most recently
from
cf599d5 to
4317c2d
Compare
…sResearch#47437 - derive the compact_rows projection from SCHEMA_SQL (parse once, cache) instead of a hardcoded column list: the original NousResearch#47437 list was cut against a June schema and silently dropped session_key/chat_id/chat_type/ thread_id/display_name/origin_json/expiry_finalized/git_branch/ git_repo_root/compression_failure_* — including desktop sidebar fields. Schema-derived means declaratively reconciled new columns are included automatically; only system_prompt is excluded. - guard test pinning the schema<->projection contract (mutation-verified: dropping a column from the projection fails it) - wire compact_rows=(not full) into /api/sessions and /api/profiles/sessions so the SQL projection pairs with the API-level field strip (?full=1 still returns complete rows end-to-end) - pass compact_rows at the remaining hot list callers: /api/status active count, _session_latest_descendant fallback, /api/sessions/stats by-source - thread compact_rows through the compression-tip projection (_get_session_rich_row) so projected tips can't reintroduce the blob - add pagination tests for get_messages (NousResearch#60347 shipped none): paging order, offset-past-end, active-flag interaction; add tip-projection compact test - AUTHOR_MAP entries for mahdiwafy + CodeForgeNet (plain emails)
Review finding: get_messages(offset=N) with no limit dropped the OFFSET entirely. SQLite requires a LIMIT clause for OFFSET, so emit LIMIT -1 (unbounded) when only offset is given. Regression test added.
tui_gateway session.list/most_recent now pass compact_rows=True (NousResearch#47437 salvage); the keyword-only fake signatures in test_tui_gateway_server.py rejected the new kwarg and CI slice 6/8 failed with TypeError. Other list_sessions_rich fakes use **kwargs and are unaffected.
Rebase reconciliation with NousResearch#60884: _count_status_active_sessions (from NousResearch#58238) now passes compact_rows=True (this branch's NousResearch#47437 projection), so the fake asserts both.
…t httpx.ResponseNotRead When an API error carries an httpx.Response whose body was consumed via iter_bytes() during streaming error handling (e.g. GeminiAPIError from agent/gemini_native_adapter.py), accessing .text raises httpx.ResponseNotRead. The secondary exception replaced the real, already-computed provider error (429 free-tier quota guidance) with the generic 'Attempted to access streaming response content' message on every turn. Guard the .text access so it degrades to an empty snippet and falls through to the str(error) fallback, which carries the full original message. Mirrors the existing guards in agent/error_classifier.py::_extract_error_body() and agent/gemini_native_adapter.py::gemini_http_error(). Fixes NousResearch#59769 Salvaged from PR NousResearch#59868 (guard + regression test); the unrelated desktop Ctrl-C fix bundled in that PR was intentionally dropped and is triaged separately.
Gateway froze the fallback chain at process start while cron reloads it per job, so a chain configured after hermes gateway was running never reached messaging sessions. Refresh from disk on agent create and when reusing a cached agent. Fixes NousResearch#60955. (cherry picked from commit b64e715)
Pin reload + cached-agent apply helpers for NousResearch#60955 so a mid-uptime fallback chain change reaches messaging sessions without a restart. (cherry picked from commit fafb341)
Follow-ups on the NousResearch#60987 salvage (review pass): - _refresh_fallback_model: keep last known-good chain on transient config.yaml read/parse failure (user mid-edit, torn write) — only a successful read that lacks the key clears the chain. Previously a refresh error wiped a cached agent's working fallback for the turn. - Move the cached-agent refresh+apply OUTSIDE the agent-cache lock: config.yaml read is disk I/O and the idle-sweep watcher contends on that lock (same reasoning as NousResearch#52197). Per-session turn serialization keeps the post-lock apply safe. - _apply_fallback_chain_to_agent: clear _unavailable_fallback_keys when chain content actually changes, so an entry re-configured mid-uptime (e.g. credentials added) is retried instead of staying suppressed for the cached agent's lifetime; no-op refreshes keep the memo. - Tests: cwd-independent source pin (Path(__file__) anchor), pin the reuse-path apply call, + regression tests for last-known-good, memo clear-on-change, memo keep-on-unchanged (mutation-verified).
Reapply the endpoint-aware preflight after request and execution middleware so no override can reintroduce a connection-scoped ID.
Exercise request and execution middleware replacements through the real conversation loop and assert the provider payload is sanitized.
… in fetch_models fetch_models() sends Authorization: Bearer <api_key> plus any default_headers (x-api-key etc.) via urllib.request.urlopen, and urllib's redirect handler forwards every header when following a 3xx — including to a different host. A catalog endpoint (or a compromised/misconfigured proxy in front of it) answering with a redirect to another origin therefore received the provider API key. Install an HTTPRedirectHandler that drops authorization, x-api-key, api-key, x-goog-api-key and cookie when the redirect target hostname differs from the original request, mirroring the pattern already used in skills/creative/comfyui/scripts/_common.py. Same-host redirects keep credentials so legitimate path-level redirects still work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g credentials Review feedback: a same-host redirect to a different port can land on a different service, which must not inherit the provider API key. Compare (scheme, hostname, effective port) — with 80/443 defaults — instead of hostname alone, and add a two-server regression test for the same-host/different-port case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`hermes -t web chat` silently dropped the toolset filter (and the same hold true for `-m`, `--provider`, `--tui`, `--dev` placed before `chat`). Reported in NousResearch#28780 for `-t/--toolsets`; the others are sibling failures with the same root cause. Root cause: the chat subparser re-declared these flags with `default=None` (or `default=False` for store_true) on top of the matching top-level parser flags. When argparse dispatches into the subparser it shares the namespace via `dest`, so the subparser's default overwrites whatever the top-level parser parsed before the subcommand. `-s/--skills`, `-r/-c/-w`, `--yolo`, and `--pass-session-id` already use `default=argparse.SUPPRESS` for exactly this reason — the chat-subparser action becomes a no-op unless the user explicitly passes the flag after `chat`, and the parent value survives. Reproduction (origin/main, before fix): >>> parser.parse_known_args(["-t", "web", "chat"]).toolsets None >>> parser.parse_known_args(["chat", "-t", "web"]).toolsets 'web' After fix: >>> parser.parse_known_args(["-t", "web", "chat"]).toolsets 'web' >>> parser.parse_known_args(["chat", "-t", "web"]).toolsets 'web' Sibling flags fixed in the same commit because they share the exact same argparse pattern bug — verified via a new contract test that scans every chat-subparser action whose `dest` is also on the top-level parser and asserts `default is argparse.SUPPRESS`. The test fails on origin/main listing all five offenders and passes after this fix. Test additions in tests/hermes_cli/test_argparse_flag_propagation.py: - TestChatSubparserInheritedValueFlags exercising real `_parser` build (not the hand-rolled replica) so it catches future drift. - Parametrized before-chat / after-chat cases for `-t`, `--toolsets`, `-m`, `--model`, `--provider`. - Negative case: passing none of the flags leaves attrs at the top-level parser's `None` default (SUPPRESS does not remove existing attrs). - Combined case: all three value flags before `chat` simultaneously. - store_true cases for `--tui` / `--dev`. - Contract test asserting every shared-`dest` flag on chat uses SUPPRESS. Fixes NousResearch#28780.
* fix(agent): persist truthful tool effect dispositions * fix(agent): preserve successful siblings during orphan recovery * fix(agent): narrow effect dispositions to none and unknown
Bundle Fireworks AI as a first-class BYOK provider across the CLI, web/TUI, and desktop onboarding. - New model-provider plugin with attribution headers (HTTP-Referer / X-Title) so Fireworks can attribute Hermes traffic; PAYG-safe default aux + fallback models (accounts/fireworks/models/...), IDs tracking fw-ai/fireconnect. - Registered in CANONICAL_PROVIDERS so it appears in the CLI/web/TUI pickers. - Alias wiring (fireworks-ai, fw) into both CLI resolvers. - First-class wiring: OPTIONAL_ENV_VARS, HERMES_OVERLAYS (FIREWORKS_BASE_URL override), doctor env hints. Live catalog + model_metadata are auto-derived. - doctor: treat Fireworks' native slash-form IDs (accounts/fireworks/...) as valid, not aggregator vendor prefixes, so it no longer tells Fireworks users to switch to openrouter or drop the prefix. - picker: plugin providers with no static curated list now lead with their profile fallback_models, so the default is an agentic chat model instead of whatever the live catalog returns first (Fireworks listed an image model, flux-*, ahead of its chat models). - Desktop onboarding: Fireworks as a RECOMMENDED hero card with the official Fireworks logomark and a brand-purple badge, routing to the BYOK key form; i18n in en/ja/zh/zh-hant. - Tests: profile contract, first-class wiring (both resolvers, overlay, config, doctor incl. the slash-form regression, aux headers, credentials), discovery spot-check, and a live smoke test driven through the Hermes runtime. Fire Pass (fpk_) support is coming soon; the future wiring is kept as a commented-out scaffold in the plugin.
The `sessions` table records only the initial (model, billing_provider) for a session, so when a user switches models mid-session (via `/model` or programmatically) every token — including the switched model's — is attributed to the first model. Insights/billing reports then hide the cost of the new model entirely (e.g. a session that started on deepseek and switched to opus shows $0 for opus). Add a `session_model_usage` table keyed (session_id, model, billing_provider) that accumulates each per-API-call delta under the model active at the time of the call. `update_token_counts()` is the single chokepoint every per-call delta flows through (CLI, gateway, cron, delegated, codex), so recording there captures accurate attribution on every platform. Only the incremental path records — the gateway's `absolute=True` summary overwrite is skipped to avoid double-counting cumulative totals that can't be split per model. When a call omits the model, it falls back to the session's recorded model, matching the existing COALESCE-from-session summary behaviour. Insights `_compute_model_breakdown` now aggregates tokens and cost from `session_model_usage`, so a switched session splits correctly across models, with a defensive fallback to the per-session aggregate for any session lacking usage rows. A v17 migration backfills one usage row per existing token-bearing session from its aggregate totals (idempotent via INSERT OR IGNORE), validated lossless against a 1.3 GB production DB. Tests: per-model recording, mid-session split, model fallback, absolute no-double-count, v17 backfill, and an insights-level switch breakdown. Fixes NousResearch#51607. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Preserve deletability, route identity, stored costs, aggregate reconciliation, and zero-usage Codex route accounting on top of the salvaged per-model usage work.
🚨 CRITICAL Supply Chain Risk DetectedThis PR contains a pattern that has been used in real supply chain attacks. A maintainer must review the flagged code carefully before merging. 🚨 CRITICAL: Install-hook file added or modifiedThese files can execute code during package installation or interpreter startup. Files: Scanner only fires on high-signal indicators: .pth files, base64+exec/eval combos, subprocess with encoded commands, or install-hook files. Low-signal warnings were removed intentionally — if you're seeing this comment, the finding is worth inspecting. |
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
20 participants
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.
Internal review before upstream
Internal review PR within
UpstageAI/hermes-agentbefore we submit tonousresearch/hermes-agent. (Upstream PR NousResearch#41697 was opened then closed to revise here first.)What
Adds Upstage Solar as a model provider so it appears in the
hermes model/hermes setupprovider picker. Solar exposes an OpenAI-compatible chat-completions endpoint (https://api.upstage.ai/v1), so this uses Hermes' documented fast path — a self-registeringProviderProfileplugin. No transport changes.solar-pro(rolling alias for the latest Solar Pro).fallback_models): only the agentic Solar Pro models (solar-pro,solar-pro3). When a key is set, the live/v1/modelscatalog lists everything available.Changes
plugins/model-providers/upstage/—ProviderProfile+plugin.yaml. Nameupstage, aliassolar,UPSTAGE_API_KEY(+ optionalUPSTAGE_BASE_URL).default_aux_modelleft empty so auxiliary side tasks (compression, vision, memory) fall back to the user's main model. Every downstream layer auto-wires from the registry.agent/model_metadata.py— context-window fallbacks (/v1/modelsomitscontext_length). Versioned ids resolve exactly;solar-procarries the 128K Pro context as the rolling-alias catch-all for futuresolar-pro*releases..env.example— documentsUPSTAGE_API_KEY/UPSTAGE_BASE_URL.solar-prodefault.Review feedback applied
solar-pro(= latest Solar Pro), led infallback_models[0].solar-miniremoved from the default/fallback model list. It's also not pinned as the aux model —default_aux_modelis left empty so background tasks use the main model, avoiding a silent 404 when Mini is deprecated (no committed replacement).solar-pro2dropped from promotion; descriptions no longer enumerate a fixed model list, so better models surface automatically via the live catalog.solar-pro(128K) instead of a baresolar(32K) entry — futuresolar-pro4inherits the Pro context.Still to confirm before upstreaming
solar-pro/solar-pro3are the exact native slugs returned byhttps://api.upstage.ai/v1/models(vs OpenRouter's dashedsolar-pro-3).fallback_modelsonly matters offline — the live catalog overrides it when a key is set.solar-minias aux modelsolar-pro), so no dependency on Mini.reasoning_effortwiring). Confirm Solar Pro doesn't require echoingreasoning_contentback on later turns (the DeepSeek/Kimi trap); if it does, we'll add abuild_api_kwargs_extrasoverride.https://console.upstage.ai/api-keys.feat(agent).Testing
594+ passed across provider/plugin/runtime/CLI/parity/api-key suites;
ruffclean. Verified the picker shows"Upstage Solar", the default resolves tosolar-pro, and a futuresolar-pro4inherits the Pro context window.Platforms tested: macOS.
🤖 Generated with Claude Code