Skip to content

fix: align doctor with auth-store and enabled tools - #51644

Open
shokirunicus wants to merge 3 commits into
NousResearch:mainfrom
shokirunicus:fix/doctor-auth-store-toolsets
Open

fix: align doctor with auth-store and enabled tools#51644
shokirunicus wants to merge 3 commits into
NousResearch:mainfrom
shokirunicus:fix/doctor-auth-store-toolsets

Conversation

@shokirunicus

Copy link
Copy Markdown

Summary

  • treat logged-in auth-store/OAuth providers as configured in doctor without requiring raw API keys in .env
  • limit doctor auth-store probes to auth-backed provider types so API-key/AWS SDK credential chains are not triggered
  • filter Tool Availability diagnostics to CLI-enabled toolsets and ignore the local .install_method stamp

Tests

  • git diff --check HEAD
  • venv/bin/python -m py_compile hermes_cli/doctor.py tests/hermes_cli/test_doctor.py
  • HOME=/Users/purpl uv run --extra dev ruff check hermes_cli/doctor.py tests/hermes_cli/test_doctor.py
  • HOME=/Users/purpl uv run --extra dev python -m pytest tests/hermes_cli/test_doctor.py -q -o 'addopts=' (70 passed)
  • hermes doctor (All checks passed)
  • changed-line secret scan (hits=0)
  • added-line absolute path scan (hits=0)
  • independent read-only code review (PASS)

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have labels Jun 24, 2026

@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 auth-store doctor investigation. The underlying OAuth-without-.env problem remains on current main: hermes_cli/doctor.py:704-715 only checks .env provider hints before treating the configuration as missing.

Problems

  • Current main intentionally filters only the final missing-API-key summary, while retaining tool-availability rows (hermes_cli/doctor.py:2195-2200, commit 6b21a935). The PR instead filters both displayed lists at proposed hermes_cli/doctor.py:2148-2151; this needs an explicit UX decision during salvage.
  • The added delegation regression test does not reach its new fallback: proposed tests/tools/test_delegate.py:389 passes a non-empty model, which the existing expression already uses. The real dispatcher passes resolved creds["model"] at tools/delegate_tool.py:2522.
  • .install_method is already ignored on main at .gitignore:121-124.

Suggested changes

  • Preserve the auth-store doctor fix, but reconcile the tool-row behavior with 6b21a935.
  • Drop the unrelated delegation change unless a reachable falsey-model path can be demonstrated and tested through the real credential-resolution flow.

Automated hermes-sweeper review.

Comment thread hermes_cli/doctor.py
available, unavailable = check_tool_availability()
available, unavailable = _apply_doctor_tool_availability_overrides(available, unavailable)

enabled_toolsets = _doctor_enabled_toolsets_for_platform("cli")

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 filters only the final missing-API-key summary (commit 6b21a935), while retaining all tool-availability warning rows. This filters the displayed rows as well; please reconcile that UX change with the current-main behavior rather than applying it incidentally.

goal="Use fallback route",
context=None,
toolsets=None,
model="high-output-child-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.

This supplies a non-empty model, so both the old and new callback-label expressions select it before reaching the added fallback. The test passes on pre-change code; either demonstrate a reachable falsey-model flow through credential resolution or drop the dead branch.

Comment thread .gitignore
# Install-method stamp written into the managed checkout root by the installer.
# It is local runtime state that lets Hermes distinguish git/docker/pip installs;
# never commit the operator-specific stamp value.
.install_method

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.

This entry is already present on current main at .gitignore:121-124; omit this hunk when salvaging the PR.

@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 labels Jul 15, 2026
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have 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.

3 participants