Skip to content

fix(auth): re-resolve Kimi pool base URLs - #18694

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/kimi-credential-base-url-5908
Open

LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/kimi-credential-base-url-5908

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • normalize loaded kimi-coding credential-pool entries with the current API key prefix rules
  • keep explicit KIMI_BASE_URL overrides authoritative
  • add regression coverage for stale Moonshot base_url values on sk-kimi keys

Fixes #5908

Verification

  • scripts/run_tests.sh tests/hermes_cli/test_api_key_providers.py::TestKimiCodeCredentialPoolAutoDetect tests/hermes_cli/test_api_key_providers.py::TestKimiCodeCredentialAutoDetect
  • scripts/run_tests.sh tests/hermes_cli/test_api_key_providers.py
  • git diff --check origin/main...HEAD

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists area/auth Authentication, OAuth, credential pools provider/kimi Kimi / Moonshot labels May 2, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for targeting the persisted Kimi pool case. The stale-entry premise remains valid on current main: hermes_cli/auth.py:1340-1345 returns stored entries unchanged, agent/credential_pool.py:2342-2355 rehydrates them, and hermes_cli/runtime_provider.py:409 uses the selected entry’s URL.

Problems

  • hermes_cli/auth.py:962 overwrites every Kimi entry’s stored base_url. Current runtime deliberately treats a non-default pool URL as explicit (hermes_cli/runtime_provider.py:476-485), so this would replace intentionally configured proxy/custom endpoints.
  • The current read_credential_pool() has profile/global per-provider fallback semantics at hermes_cli/auth.py:1301-1345; the proposed hunk predates that implementation and must be integrated with it.

Suggested changes

  • Normalize only missing or known-stale default URLs, while preserving non-default stored URLs absent an explicit KIMI_BASE_URL override.
  • Add a pool-to-runtime regression test, plus preservation coverage for a non-default pool URL.

Automated hermes-sweeper review.

Comment thread hermes_cli/auth.py
or item.get("api_key")
or ""
).strip()
item["base_url"] = _resolve_kimi_base_url(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This rewrites every persisted Kimi endpoint, including a deliberate non-default proxy/custom URL. Current runtime treats non-default pool URLs as explicit (hermes_cli/runtime_provider.py:476-485); please limit rewriting to missing or known-stale default URLs, while retaining explicit endpoint values unless KIMI_BASE_URL is set.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026
@DavidMetcalfe

Copy link
Copy Markdown
Contributor

@LeonSGP43 — thanks for starting the initial work on this! Following up on the sweeper's keep_open review, the requested changes were: (1) preserving custom/non-default stored URLs, (2) integrating with the current profile/global fallback semantics in read_credential_pool, and (3) adding pool-to-runtime plus custom-URL preservation regression tests.

Since this PR hasn't seen updates since May (and the test check has been failing), I implemented those changes in #83537 (addressing #5908) so the fix doesn't stall. If you'd like to continue with this PR instead, the review feedback is still actionable and the tests from #83537 can be cherry-picked over — otherwise we can proceed with #83537.

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 P2 Medium — degraded but workaround exists provider/kimi Kimi / Moonshot sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

kimi-coding credential pool base_url not re-resolved from key prefix on load

4 participants