Skip to content

Fail LLM setup when callback port is busy - #6432

Open
jerm-dro wants to merge 2 commits into
mainfrom
jerm-dro/01M0X4AKVD2MW0E2T938A3SD37
Open

Fail LLM setup when callback port is busy#6432
jerm-dro wants to merge 2 commits into
mainfrom
jerm-dro/01M0X4AKVD2MW0E2T938A3SD37

Conversation

@jerm-dro

Copy link
Copy Markdown
Collaborator

Summary

  • OAuth providers require callback URLs to match their registered redirect URIs. Silently replacing a busy callback port with a random one causes authentication to fail at the provider and hides the local port conflict.
  • Require the LLM authentication flow to keep its requested callback port and return an actionable error when that port is occupied.
  • Preserve automatic port fallback for OAuth consumers that support dynamically selected callback ports.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Does this introduce a user-facing change?

thv llm setup now fails immediately when its requested OIDC callback port is occupied. The error tells users to stop the process using that port or choose another redirect URI registered with their identity provider using thv llm setup --callback-port <port>.

Special notes for reviewers

Strict callback-port behavior is opt-in at the shared OAuth layer and enabled for the LLM token source only, so existing OAuth flows that support dynamic callback ports retain their fallback behavior.

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Aug 25, 2026
@jerm-dro
jerm-dro requested a lite review from Copilot August 25, 2026 22:47

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

This PR makes the OAuth/OIDC callback-port handling stricter for thv llm setup so that when a pre-registered redirect URI requires a specific callback port, the setup fails immediately (instead of silently falling back to a random port that will break provider redirect-URI matching). The strict behavior is implemented as an opt-in flag at the shared OAuth layer and enabled for the LLM token source only, preserving existing fallback behavior for other OAuth consumers (e.g., registry auth).

Changes:

  • Introduces a typed CallbackPortInUseError and propagates it through OAuth flow creation and LLM setup for actionable user-facing errors.
  • Adds RequireExactCallbackPort plumbing from pkg/auth/tokensourcepkg/auth/oauth and enables it for LLM tokens.
  • Adds/extends unit tests to cover strict “port in use” failure vs. fallback behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/networking/port.go Adds CallbackPortInUseError and uses it from callback-port validation.
pkg/llm/tokensource.go Enables strict callback-port behavior for LLM OIDC token source.
pkg/llm/setup.go Detects callback-port conflicts and returns a remediation-focused error message.
pkg/llm/setup_test.go Adds tests for callback-port-in-use failures (pre-login and during login).
pkg/auth/tokensource/tokensource.go Adds RequireExactCallbackPort to OIDC params and passes it into oauth config.
pkg/auth/oauth/flow.go Enforces strict callback-port behavior when requested port is busy.
pkg/auth/oauth/flow_test.go Adds tests for strict failure vs. fallback when the requested callback port is occupied.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/auth/oauth/flow.go
Comment thread pkg/llm/setup.go
@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.84%. Comparing base (7c9c55e) to head (310a229).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/auth/oauth/flow.go 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6432      +/-   ##
==========================================
+ Coverage   77.81%   77.84%   +0.03%     
==========================================
  Files         760      761       +1     
  Lines       73133    73204      +71     
==========================================
+ Hits        56908    56987      +79     
+ Misses      16220    16212       -8     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants