Skip to content

Bound model catalog response reads - #54840

Open
ooiuuii wants to merge 1 commit into
NousResearch:mainfrom
ooiuuii:fix/models-response-cap
Open

Bound model catalog response reads#54840
ooiuuii wants to merge 1 commit into
NousResearch:mainfrom
ooiuuii:fix/models-response-cap

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a shared 16 MiB bounded reader for live model-catalog JSON responses in hermes_cli/models.py
  • route OpenRouter/Nous recommendation and pricing fetches, Anthropic/Copilot/custom /models probes, and LM Studio model discovery through the bounded reader
  • keep oversized or malformed catalog responses on the existing fallback/cache paths

Fixes #54838

Validation

  • uv run --extra dev python -m pytest tests\hermes_cli\test_models.py tests\hermes_cli\test_model_validation.py tests\hermes_cli\test_api_key_providers.py -q --basetemp .pytest-tmp-models-response-cap (332 passed)
  • git diff --check
  • autoreview helper unavailable locally (autoreview, agent-autoreview, and codex-autoreview not found)

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Jun 29, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — Bounds model catalog response reads to 16MB. Adds two helper functions (_read_model_catalog_json, _read_model_catalog_text) that replace raw resp.read() calls across 10 call sites in models.py. Tests update mock read() to accept size parameter and verify bounded reads. The oversized-response fallback to static snapshot is well-tested.

@ooiuuii
ooiuuii force-pushed the fix/models-response-cap branch from d90e19a to 44a5531 Compare June 29, 2026 22:23
@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

Security evidence:

  • trust boundary: remote/provider-controlled model catalog and pricing responses entering CLI model discovery and validation.
  • source/sink/invariant: the live catalog and pricing response reads in hermes_cli/models.py now go through bounded helpers, with JSON reads requesting cap + 1 bytes and falling back when an oversized body is detected.
  • current-main reproduction: a run-root probe imported the current-main hermes_cli/models.py and reproduced the existing OpenRouter catalog path calling the response with read(-1).
  • PR-head or patch-replay validation: the same probe imported the run-owned patch replay, lowered the catalog cap to 8 bytes, and confirmed the live OpenRouter catalog path called read(9) and fell back to the curated list on oversized data.
  • positive/negative cases: the new bounded-read and oversized-fallback tests cover the changed OpenRouter path, and the adjacent model catalog, validation, and API-key provider tests passed.
  • residual bypass search: the replayed hermes_cli/models.py has no remaining direct response .read() calls outside the two bounded helpers, and the changed live catalog call sites route through those helpers.
  • reviewer validation: CodeRabbit completed with no findings in the clean-pass flow.

I reviewed the meaningful PR patch replayed onto current GitHub main because the submitted branch has a stale/conflict setup advisory; that replay applied cleanly and validated, but the local replay by itself does not prove the submitted branch will keep merging cleanly if main moves again.

Signed: GPT-5.5-xhigh in Codex

@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 focused hardening work. The underlying issue remains on current main: hermes_cli/models.py:1410 still calls resp.read() before decoding the live OpenRouter catalog.

Problems

  • The OpenRouter picker also calls get_curated_openrouter_models() first (hermes_cli/models.py:1397-1401), but its remote manifest fetch remains an unbounded resp.read() in hermes_cli/model_catalog.py:125-136. The shared bound should cover that catalog source too.
  • Current main now uses _urlopen_model_catalog_request() (hermes_cli/models.py:33) after commit 6e75ba7fa. The PR diff patches raw urllib.request.urlopen calls and mocks that old seam, so the implementation and tests need to be ported to the current wrapper while preserving the redirect-security behavior.

Suggested changes

  • Put the bounded JSON/text reader in a shared hermes_cli location and apply it to both models.py and model_catalog.py remote catalog reads.
  • Retarget tests to _urlopen_model_catalog_request() and add an oversized-manifest fallback test for model_catalog.py.

Automated hermes-sweeper review.

Comment thread hermes_cli/models.py
_openrouter_catalog_cache: list[tuple[str, str]] | None = None


def _read_model_catalog_json(resp) -> Any:

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 make this bounded reader available to hermes_cli/model_catalog.py too. fetch_openrouter_models() first fetches that remote curated manifest, and _fetch_manifest() still performs an unbounded resp.read() on current main.


monkeypatch.setattr(_models_mod, "_openrouter_catalog_cache", None)
with patch(
"hermes_cli.model_catalog.get_curated_openrouter_models",

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 reaches this response through _urlopen_model_catalog_request() (hermes_cli/models.py:33), not raw urllib.request.urlopen; port this mock to that wrapper seam when salvaging the test.

@teknium1 teknium1 added 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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound live model catalog response reads

5 participants