Skip to content

fix: harden Fireworks provider integration and stabilize CI - #8632

Closed
JSRRosenbaum wants to merge 2 commits into
NousResearch:mainfrom
JSRRosenbaum:joseph/fireworks-hardening-ci-fixes
Closed

fix: harden Fireworks provider integration and stabilize CI#8632
JSRRosenbaum wants to merge 2 commits into
NousResearch:mainfrom
JSRRosenbaum:joseph/fireworks-hardening-ci-fixes

Conversation

@JSRRosenbaum

Copy link
Copy Markdown

Summary

  • harden Fireworks provider setup and model discovery
  • restore/fix provider model-selection flow used by setup/tests
  • stabilize CI around supply-chain audit comment behavior and provider ordering

Changes

  • block non-TTY API key entry instead of falling back to plain input()
  • add bounded pagination + explicit logging for Fireworks catalog fetches
  • sanitize/reject unsafe Fireworks model IDs before terminal display
  • restore/fix _setup_provider_model_selection()
  • import normalize_opencode_model_id from the correct module
  • preserve provider autodetect priority so Xiaomi doesn't get shadowed by Fireworks
  • make supply-chain audit reporting fork-safe by always writing findings to the job summary and making PR comments best-effort / same-repo only
  • add git to the Docker image for likely git-backed npm dependency resolution issues

Validation

Targeted local validation:

  • 181 passed
    • tests/hermes_cli/test_xiaomi_provider.py
    • tests/hermes_cli/test_api_key_providers.py
    • tests/hermes_cli/test_setup_model_selection.py
  • 166 passed
    • tests/hermes_cli/test_models.py
    • tests/cli/test_cli_provider_resolution.py::test_model_flow_custom_saves_verified_v1_base_url
    • tests/cli/test_cli_provider_resolution.py::test_model_flow_custom_requires_tty_for_api_key_entry
    • tests/agent/test_model_metadata.py
    • tests/agent/test_credential_pool.py

Notes

  • I did not find a blocking security issue in the Fireworks integration itself.
  • Local Docker repro was blocked by a Docker Hub timeout while pulling debian:13.4, so the Dockerfile fix is reasonable but not fully re-verified end-to-end here.
  • Issue [Feature]: Firework Pass Support #3510 was closed as not planned for Firework Pass support; this PR is framed around hardening and CI/test stability for the current integration path.

paraddox and others added 2 commits April 12, 2026 20:03
This squashes the Fireworks branch into one decision record on top of current main.
It keeps Fireworks aligned with the existing API-key provider seams across auth,
model catalogs, setup, CLI selection, runtime resolution, and the rebased test
baselines needed to stay compatible with current upstream behavior.

Constraint: Must preserve current main's provider architecture while keeping Fireworks usable end to end
Rejected: Keep four-commit branch history | unnecessary review noise after repeated rebases
Confidence: high
Scope-risk: moderate
Reversibility: clean
Directive: On future rebases, keep current main behavior and reapply only Fireworks-specific provider deltas
Tested: Fireworks provider pytest slice after squash
Not-tested: Full pytest suite; known upstream failures remain outside the Fireworks slice
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles labels Apr 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #8693 — same Fireworks provider hardening scope (API-key priority, model ID sanitization, pagination bounds, non-interactive stdin guard). Recommend consolidating.

@teknium1 teknium1 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.

Thanks for the hardening work. Current main has since landed Fireworks through a different provider-profile path, so this branch is no longer a clean salvage target.

Problems

  • hermes_cli/models.py:75 adds a Fire Pass router model. Current main deliberately uses PAYG /models/ IDs instead (plugins/model-providers/fireworks/__init__.py:25-33), and tests/hermes_cli/test_fireworks_provider.py:167-172 asserts Fireworks auxiliary models do not use /routers/. The later policy-alignment commit is 31152ae108e8.
  • The PR's registry/catalog wiring duplicates the current plugin-derived registration path in hermes_cli/auth.py:447-478 and the current picker entry in hermes_cli/models.py:1083.
  • The Docker and supply-chain hunks are also stale: remote main already installs git (Dockerfile:31) and has since redesigned the audit workflow (.github/workflows/supply-chain-audit.yml:37-160).

Suggested changes

  • If any CI behavior is retained, re-scope it against the current audit workflow rather than applying this snapshot.
  • Keep Fireworks changes within the current provider-profile architecture; do not restore Fire Pass routing without explicit maintainer direction.

Automated hermes-sweeper review.

Comment thread hermes_cli/models.py
return _add_forward_compat_models(list(DEFAULT_CODEX_MODELS))


FIREWORKS_FIRE_PASS_MODELS: list[str] = [

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.

Current main deliberately avoids Fire Pass router IDs: the landed Fireworks profile uses PAYG /models/ fallbacks, and its focused test requires the auxiliary model not to contain /routers/. Please do not reintroduce this router catalog without explicit maintainer approval.


permissions:
pull-requests: write
issues: write

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.

Please keep workflow permissions minimal. Current main's redesigned audit workflow retains pull-requests: write and contents: read; any added issues: write scope needs a concrete operation that cannot use the existing permission.

@teknium1 teknium1 added sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the thorough hardening work here. Closing both this and #8693: they were built against the Fireworks integration from PR #3564, which was never merged — the Fireworks support that eventually landed on main (July 2026) is the official provider plugin using the standard ProviderProfile path, so the bespoke catalog-fetch code these PRs harden (pagination bounds, model-ID sanitization) doesn't exist in the tree. I also verified the autodetect-priority concern empirically on current main: fireworks is auto-extended AFTER the static registry entries, so with both XIAOMI_API_KEY and FIREWORKS_API_KEY set, resolve_provider('auto') returns xiaomi — no shadowing. The non-TTY API-key guard also exists on main now. If you spot a hardening gap that reproduces against the current plugin-based integration, a fresh focused PR is very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants