Skip to content

fix(xai): scan the primary model.default slot in the retirement migration/doctor check - #55737

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/xai-retirement-scan-model-default
Closed

fix(xai): scan the primary model.default slot in the retirement migration/doctor check#55737
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/xai-retirement-scan-model-default

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

find_retired_xai_refs (hermes_cli/xai_retirement.py) powers both hermes migrate xai and the hermes doctor xAI-retirement check. It scanned principal, auxiliary.<slot>, delegation, tts.xai, and plugins.image_gen.xai — but never the top-level model block, which is the single most important slot: the primary active model.

runtime_provider._get_model_config is the schema source-of-truth here — it resolves the active model from model.default, accepting model.model as an alias only when default is absent. So a user with the canonical config:

model:
  provider: xai
  default: grok-3

got a false "No retired xAI models in config — nothing to migrate" from both commands after the May 15, 2026 grok retirement, even though their primary model was retired.

This PR adds a model-block scan (placed first, since it is the primary slot) that mirrors _get_model_config's precedence: model.default is canonical, model.model is consulted only when default is absent.

Mirrors runtime_provider._get_model_config precedence: model.default > model.model.

Related Issue

Code-originated; found by cross-checking the scanner against _get_model_config. No filed issue.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/xai_retirement.py: scan the top-level model block in find_retired_xai_refs, emitting model.default / model.model issues per the resolver precedence. Docstring slot list updated.
  • tests/hermes_cli/test_xai_retirement.py: add TestFindRetiredModelBlock (default slot, alias slot, default-precedence-over-alias, valid-not-flagged) and extend test_full_trap_config to include the model.default slot.

The apply side needs no change: _walk_to_parent already resolves the 2-segment model.default / model.model paths, so hermes migrate xai --apply rewrites them (and adds the reasoning_effort sibling) correctly — verified end-to-end against a sample config.yaml.

How to Test

uv run --with pytest python3 -m pytest tests/hermes_cli/test_xai_retirement.py tests/hermes_cli/test_migrate_xai.py -v

Before the fix, find_retired_xai_refs({"model": {"provider": "xai", "default": "grok-3"}}) returned []; after, it returns one issue at model.defaultgrok-4.3. The four new TestFindRetiredModelBlock tests fail before the production change and pass after.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the touched test suites (test_xai_retirement.py, test_migrate_xai.py, test_doctor.py) and they pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — updated the find_retired_xai_refs docstring slot list
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A (no new keys)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) — or N/A (pure dict-walking logic)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…tion/doctor check

find_retired_xai_refs scanned principal, auxiliary, delegation, tts.xai and
plugins.image_gen.xai — but never the top-level model block, which is the
PRIMARY active-model slot. runtime_provider._get_model_config resolves the
active model from model.default (with model.model as the accepted alias), so
a user with the canonical model: {provider: xai, default: grok-3} got a false
'nothing to migrate' from both hermes migrate xai and hermes doctor after the
May 15, 2026 grok retirement.

Add a model-block scan (placed first, since it is the primary slot) mirroring
_get_model_config's precedence: model.default is canonical, model.model is
consulted only when default is absent. apply_migration's _walk_to_parent
already resolves the 2-segment model.default / model.model paths, so the
apply side rewrites them (and adds the reasoning_effort sibling) unchanged.
Copilot AI review requested due to automatic review settings June 30, 2026 15:42

Copilot AI 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.

Pull request overview

Fixes a gap in the xAI retirement scanner (find_retired_xai_refs) used by both hermes migrate xai and the hermes doctor xAI-retirement check, ensuring it also detects retired models configured in the primary active-model slot (model.default, with model.model as fallback alias).

Changes:

  • Extend find_retired_xai_refs to scan the top-level model block, mirroring runtime_provider._get_model_config precedence (model.default > model.model).
  • Update the scanner docstring slot list to include the primary model.default slot.
  • Add/extend unit tests to cover model.default, model.model alias behavior, and default-precedence over alias.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
hermes_cli/xai_retirement.py Adds scanning of the primary model.default slot (and model.model alias) to prevent false “nothing to migrate” results.
tests/hermes_cli/test_xai_retirement.py Adds targeted coverage for the model-block scan and updates the full-config trap test accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/hermes_cli/test_xai_retirement.py Outdated
Comment on lines +170 to +171
issues = find_retired_xai_refs(cfg)
assert len(issues) == 5
assert len(issues) == 6
Per Copilot review: asserting only the issue count is brittle and breaks
whenever slots are added/removed. Assert the exact set of trapped config
paths instead — more robust and more informative.
@briandevans

Copy link
Copy Markdown
Contributor Author

@copilot Addressed in commit 9e166fb: test_full_trap_config now asserts the exact set of trapped config paths (auxiliary.vision.model, delegation.model, model.default, plugins.image_gen.xai.model, principal.model, tts.xai.model) instead of just the issue count, so it stays informative if slots change.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard provider/xai xAI (Grok) area/config Config system, migrations, profiles labels Jun 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this overlaps with open #31614, which already adds the same model.default / model.model scan to find_retired_xai_refs in hermes_cli/xai_retirement.py and is broader (it also fixes apply-side provider-prefix preservation and root-level model: xai/grok-3 strings) and earlier. #31614 looks like the canonical superset here; this PR is a focused subset of the same gap. Both extend the merged retirement feature #29277 — flagging the cluster for a maintainer pick (not closing either).

@briandevans

Copy link
Copy Markdown
Contributor Author

Thanks @alt-glitch — agreed, #31614 is the broader change here: it also adds apply-side provider-prefix preservation (x-ai//xai/) and the root-level model: xai/grok-3 string case, which this PR intentionally doesn't touch. This one is the focused detection subset — it extends find_retired_xai_refs to the primary model.default/model.model slot only, and is currently green (all required checks pass).

Worth flagging the asymmetry for the maintainer pick: #31614 is the superset but has been idle since 2026-05-24 (~39d, unmerged); this PR is fresh and CI-clean. Happy either way — if maintainers prefer #31614 as the canonical fix I'll close this in its favor, or if the apply-side widening stalls, this green subset closes the most common gap (the active model.default slot) in the meantime. No objection to #31614 landing first.

@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 isolating the canonical model.default omission. Current main confirms the scanner skips the top-level model block (hermes_cli/xai_retirement.py:92-120) even though runtime resolution uses model.default (hermes_cli/runtime_provider.py:281-299).

Problems

  • The new branch only handles mapping-shaped model values. Runtime still accepts a legacy scalar root model (model: xai/grok-3) at hermes_cli/runtime_provider.py:298-299; it would remain undetected and therefore unapplied. Open #31614 already covers that sibling shape and root-path application.

Suggested changes

  • Either add scalar-root detection, root-path application, and regression coverage, or keep this as the narrower canonical-dict fix for a maintainer choice alongside #31614.

Automated hermes-sweeper review.

# Primary active-model slot. Mirror runtime_provider._get_model_config:
# model.default is canonical, model.model is the accepted alias (only
# consulted when default is absent).
model = config.get("model")

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.

Completeness: runtime still treats scalar model: xai/grok-3 as the active model (hermes_cli/runtime_provider.py:298-299), but this dict-only branch leaves it undetected. Please either scan/apply that root path too or explicitly leave it to the broader #31614 implementation.

@briandevans

Copy link
Copy Markdown
Contributor Author

Closing to keep my open queue focused and reviewable. This has sat ~2 weeks without any human review interest, and I would rather maintain a small set of actively-reviewed PRs than a large standing backlog. The analysis and branch remain available if anyone wants to pick this up — happy to reopen if it is useful.

@teknium1 teknium1 added 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 area/install-update Installer, updater, packaging, wheels, doctor 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 area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists provider/xai xAI (Grok) 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants