Skip to content

Add --tls-skip-verify flag to thv llm setup, config set, and proxy start#5136

Merged
yrobla merged 1 commit intomainfrom
yolanda/llm-tls-fixes
Apr 30, 2026
Merged

Add --tls-skip-verify flag to thv llm setup, config set, and proxy start#5136
yrobla merged 1 commit intomainfrom
yolanda/llm-tls-fixes

Conversation

@yrobla
Copy link
Copy Markdown
Contributor

@yrobla yrobla commented Apr 30, 2026

Summary

Stores TLSSkipVerify in the LLM config section and threads it through to every layer:

  • pkg/llm/config.go: add TLSSkipVerify field to Config; SetOptions uses *bool so that --tls-skip-verify=false explicitly clears the stored value rather than being silently ignored as a zero value
  • pkg/llm/manage.go: SetFields applies the pointer, Show() prints a WARNING line when the flag is set
  • pkg/llm/setup.go: pass TLSSkipVerify through ToolApplyConfig; extract configureDetectedTools() and warnTLSSkipVerify() helpers (gocyclo)
  • pkg/llm/proxy/proxy.go: add Option / WithTLSSkipVerify() functional option; New() accepts variadic opts; sets InsecureSkipVerify on the upstream transport when the flag is true
  • pkg/client/llm_gateway.go: add TLSSkipVerify to LLMApplyConfig; add NodeTLSRejectUnauthorized case to llmValueForSpec(); extract applyLLMGatewayKeys / addLLMKey / removeLLMKey helpers (gocyclo); ClearWhenEmpty on LLMGatewayKeySpec removes the JSON key when the flag is cleared rather than leaving a stale "0" in the settings file
  • pkg/client/config.go: add NODE_TLS_REJECT_UNAUTHORIZED key spec (ClearWhenEmpty=true) to ClaudeCode and GeminiCli (the two direct-mode Node.js tools); proxy-mode tools (Cursor, VSCode, Xcode, …) are covered by WithTLSSkipVerify on the proxy transport
  • cmd/thv/app/llm.go: wire --tls-skip-verify into setup, config set, and proxy start; proxy start overrides the stored value when the flag is explicitly passed

Fixes #5123

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)

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change

Does this introduce a user-facing change?

Implementation plan

Approved implementation plan

Special notes for reviewers

@yrobla yrobla requested a review from Copilot April 30, 2026 06:57
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Apr 30, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 adds a --tls-skip-verify flag and persists it in the LLM config, then threads it through llm setup, llm config set, and llm proxy start so local/self-signed TLS gateways can be used during development.

Changes:

  • Persist TLSSkipVerify in pkg/llm.Config, apply it via config set/setup, and display warnings in config show.
  • Extend tool settings patching to conditionally write/remove NODE_TLS_REJECT_UNAUTHORIZED via ClearWhenEmpty.
  • Add proxy-side support via WithTLSSkipVerify() and thread the flag into thv llm proxy start.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/llm/config.go Adds TLSSkipVerify to persisted LLM config.
pkg/llm/manage.go Applies pointer-based TLSSkipVerify option; prints warning in Show().
pkg/llm/manage_test.go Adds unit tests for TLSSkipVerify set/clear behavior and display.
pkg/llm/setup.go Threads TLSSkipVerify into tool configuration; adds warning/helper extraction.
pkg/llm/proxy/proxy.go Adds Option pattern + WithTLSSkipVerify and variadic New().
pkg/client/config.go Adds NodeTLSRejectUnauthorized key spec support + ClearWhenEmpty, wires into Claude/Gemini direct-mode.
pkg/client/llm_gateway.go Adds conditional add/remove logic for tool settings keys (applyLLMGatewayKeys).
pkg/client/test_support.go Extends test helper to support ClearWhenEmpty per JSON pointer.
pkg/client/llm_gateway_test.go Adds tests validating NODE_TLS_REJECT_UNAUTHORIZED write/no-write/removal behavior.
cmd/thv/app/llm.go Wires --tls-skip-verify into config set, setup, and proxy start (override behavior).

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

Comment thread pkg/llm/proxy/proxy.go Outdated
Comment thread pkg/llm/proxy/proxy.go
Comment thread pkg/llm/setup.go Outdated
Comment thread cmd/thv/app/llm.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

❌ Patch coverage is 64.60177% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.35%. Comparing base (60636a0) to head (9b93cc3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/client/llm_gateway.go 53.48% 10 Missing and 10 partials ⚠️
pkg/llm/setup.go 62.79% 14 Missing and 2 partials ⚠️
pkg/llm/proxy/proxy.go 69.23% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5136      +/-   ##
==========================================
- Coverage   67.36%   67.35%   -0.01%     
==========================================
  Files         598      598              
  Lines       60493    60566      +73     
==========================================
+ Hits        40751    40796      +45     
- Misses      16658    16679      +21     
- Partials     3084     3091       +7     

☔ View full report in Codecov by Sentry.
📢 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.

Closes #5123

Stores TLSSkipVerify in the LLM config section and threads it through to
every layer:

- pkg/llm/config.go: add TLSSkipVerify field to Config; SetOptions uses
  *bool so that --tls-skip-verify=false explicitly clears the stored value
  rather than being silently ignored as a zero value
- pkg/llm/manage.go: SetFields applies the pointer, Show() prints a WARNING
  line when the flag is set
- pkg/llm/setup.go: pass TLSSkipVerify through ToolApplyConfig; extract
  configureDetectedTools() and warnTLSSkipVerify() helpers (gocyclo)
- pkg/llm/proxy/proxy.go: add Option / WithTLSSkipVerify() functional
  option; New() accepts variadic opts; sets InsecureSkipVerify on the
  upstream transport when the flag is true
- pkg/client/llm_gateway.go: add TLSSkipVerify to LLMApplyConfig; add
  NodeTLSRejectUnauthorized case to llmValueForSpec(); extract
  applyLLMGatewayKeys / addLLMKey / removeLLMKey helpers (gocyclo);
  ClearWhenEmpty on LLMGatewayKeySpec removes the JSON key when the flag
  is cleared rather than leaving a stale "0" in the settings file
- pkg/client/config.go: add NODE_TLS_REJECT_UNAUTHORIZED key spec
  (ClearWhenEmpty=true) to ClaudeCode and GeminiCli (the two direct-mode
  Node.js tools); proxy-mode tools (Cursor, VSCode, Xcode, …) are covered
  by WithTLSSkipVerify on the proxy transport
- cmd/thv/app/llm.go: wire --tls-skip-verify into setup, config set, and
  proxy start; proxy start overrides the stored value when the flag is
  explicitly passed
@yrobla yrobla force-pushed the yolanda/llm-tls-fixes branch from c72c8aa to 9b93cc3 Compare April 30, 2026 07:13
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 30, 2026
@yrobla yrobla requested a review from Copilot April 30, 2026 07:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a persisted --tls-skip-verify configuration for the LLM gateway workflow and threads it through setup/config/proxy so tools (direct mode) and the local reverse proxy (proxy mode) can optionally bypass upstream TLS verification for local development.

Changes:

  • Persist TLSSkipVerify in the LLM config and support explicit clearing via pointer-based SetOptions.
  • For direct-mode Node.js tools, conditionally write/remove NODE_TLS_REJECT_UNAUTHORIZED=0 using a ClearWhenEmpty key spec.
  • Add proxy functional option WithTLSSkipVerify() and wire the flag into thv llm setup, thv llm config set, and thv llm proxy start, with unit tests.

Reviewed changes

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

Show a summary per file
File Description
pkg/llm/config.go Adds TLSSkipVerify to persisted LLM config.
pkg/llm/manage.go Applies TLSSkipVerify via pointer option; surfaces a warning in Show().
pkg/llm/manage_test.go Adds coverage for TLSSkipVerify pointer semantics and Show() output.
pkg/llm/setup.go Threads TLSSkipVerify into tool configuration and prints mode-specific warnings.
pkg/llm/proxy/proxy.go Introduces functional options; implements WithTLSSkipVerify() by adjusting upstream transport TLS verification.
pkg/llm/proxy/proxy_test.go Adds a test validating default TLS verification failure vs. skip-verify success.
pkg/client/config.go Extends LLMGatewayKeySpec with ClearWhenEmpty; adds Node TLS key spec for Claude Code and Gemini CLI.
pkg/client/test_support.go Extends LLMTestEntry to support ClearWhenEmpty in tests.
pkg/client/llm_gateway.go Refactors key application into helpers; supports conditional remove when ClearWhenEmpty and value is empty.
pkg/client/llm_gateway_test.go Adds tests for writing/removing NODE_TLS_REJECT_UNAUTHORIZED based on TLSSkipVerify.
cmd/thv/app/llm.go Wires --tls-skip-verify into setup/config set/proxy start, including override behavior for proxy start.

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

Comment thread pkg/llm/proxy/proxy_test.go
@yrobla yrobla merged commit b094f75 into main Apr 30, 2026
50 checks passed
@yrobla yrobla deleted the yolanda/llm-tls-fixes branch April 30, 2026 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

thv llm setup: add --tls-skip-verify (and --tls-ca-cert) for local development

4 participants