test(e2e): add all-client LLM gateway e2e matrix + fix secret provider bugs#5116
test(e2e): add all-client LLM gateway e2e matrix + fix secret provider bugs#5116
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
4c01ff7 to
332b11f
Compare
6f87bb9 to
7ba80e7
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes two secrets-provider edge cases uncovered while building a new end-to-end test matrix for thv llm across all supported AI clients, and adds a comprehensive Docker-free e2e suite plus a standalone local mock environment to support ongoing testing and debugging.
Changes:
- Fix
thv secret provider environmentincorrectly triggering container-runtime checks by treatingsecretas informational. - Fix
GetSystemSecretsProviderto honorTOOLHIVE_SECRETS_PROVIDERby resolving provider type before enforcingSetupCompleted. - Add a 16-test e2e matrix for
thv llmacross all supported clients plus a standalone mock OIDC + mock gateway environment for manual testing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/thv/app/commands.go |
Marks secret as an informational command to avoid container-runtime gating. |
pkg/auth/secrets/secrets.go |
Adjusts provider selection flow so env var overrides are evaluated before SetupCompleted checks. |
test/e2e/cli_llm_all_clients_test.go |
Adds all-client e2e coverage for setup/teardown/token/proxy and key edge cases. |
test/llm-mock-env/main.go |
Adds a standalone mock OIDC + TLS mock gateway harness for manual CLI testing. |
cmd/thv-operator/controllers/virtualmcpserver_deployment.go |
Adds encoding/json import to support JSON marshaling used in deployment hashing logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5116 +/- ##
==========================================
+ Coverage 67.12% 67.18% +0.05%
==========================================
Files 597 597
Lines 60170 60172 +2
==========================================
+ Hits 40390 40425 +35
+ Misses 16706 16669 -37
- Partials 3074 3078 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7ba80e7 to
b5b371e
Compare
…r bugs Fixes two bugs discovered while building the new test suite: 1. Add "secret" to informationalCommands so thv secret provider environment does not require a container runtime to be available. 2. Fix GetSystemSecretsProvider to call GetProviderType() before checking SetupCompleted, so the TOOLHIVE_SECRETS_PROVIDER env var is respected when interactive secrets setup has not been completed. Adds test/e2e/cli_llm_all_clients_test.go with 16 e2e tests (label: llm && clients) covering all 6 supported LLM gateway clients, proxy DNS-rebinding protection, port conflict handling, and 4 edge cases. Tests use an in-process mock OIDC server and fake browser stubs — no Docker or real credentials needed. Adds test/llm-mock-env/ for manual end-to-end testing with a local mock OIDC + LLM gateway. Closes #5115 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b5b371e to
64d9405
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes two CLI secrets-provider regressions discovered while building a broader thv llm end-to-end suite, and adds an all-client e2e matrix to exercise the full LLM workflow across supported AI clients without requiring Docker or real credentials.
Changes:
- Fix
thv secret …being treated as non-informational by adding"secret"toinformationalCommands(avoids container-runtime checks). - Fix
GetSystemSecretsProviderto honorTOOLHIVE_SECRETS_PROVIDERby resolving provider type (with env override) before enforcing setup completion; add unit coverage for the regression. - Add a 16-test Ginkgo e2e suite covering
thv llmsetup/teardown, proxy behavior (including DNS rebinding + port conflict), token behavior, and end-to-end proxy token forwarding across all supported clients (plus supporting mocks/helpers).
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 |
|---|---|
cmd/thv/app/commands.go |
Marks secret as informational to avoid container-runtime gating. |
pkg/auth/secrets/secrets.go |
Refactors system provider selection to consult env override before setup guard (injectable for tests). |
pkg/auth/secrets/secrets_test.go |
Adds regression tests validating env override bypass and no-setup error behavior. |
test/e2e/helpers.go |
Introduces reusable fake-browser stub helper for headless OIDC completion. |
test/e2e/cli_llm_setup_test.go |
Switches to the shared fake-browser helper. |
test/e2e/llm_gateway_mock.go |
Adds an in-process mock OpenAI-compatible gateway for proxy forwarding tests. |
test/e2e/cli_llm_all_clients_test.go |
Adds the all-client thv llm e2e matrix suite and supporting helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
73c5ffb to
c69587b
Compare
c69587b to
018af3e
Compare
There was a problem hiding this comment.
Pull request overview
Adds a comprehensive Docker-free e2e matrix for thv llm across all supported AI clients, along with fixes to secrets-provider selection so thv secret provider environment works on hosts without a container runtime and TOOLHIVE_SECRETS_PROVIDER=environment is honored without prior setup.
Changes:
- Fix
thv secretbeing treated as non-informational (avoids container runtime checks). - Fix system secrets provider selection to consult env override before setup gating; add regression tests.
- Add/expand e2e infrastructure and a 16-test all-client
thv llmmatrix (fake browser, mock OIDC, mock LLM gateway, proxy/token cases).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/thv/app/commands.go |
Marks secret as informational to avoid container runtime checks. |
pkg/auth/secrets/secrets.go |
Refactors system provider selection to respect env override via injectable env/config. |
pkg/auth/secrets/secrets_test.go |
Adds regression tests for env override + setup gating; adds an additional ProcessSecret behavior test. |
pkg/auth/tokensource/tokensource.go |
Adds helper to compute the env var name for cached LLM access tokens. |
test/e2e/helpers.go |
Introduces reusable fake-browser PATH helper for headless OIDC flows. |
test/e2e/cli_llm_setup_test.go |
Switches to the shared fake-browser helper. |
test/e2e/llm_gateway_mock.go |
Adds a mock OpenAI-compatible gateway for proxy/token forwarding e2e tests. |
test/e2e/cli_llm_all_clients_test.go |
Adds the 16-test all-client e2e matrix (setup/teardown/proxy/token/edge cases). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR fixes two CLI/secrets-provider regressions uncovered while building a comprehensive Docker-free thv llm e2e suite, and adds a full all-client LLM gateway e2e matrix (plus CI wiring) to prevent future regressions.
Changes:
- Fix
thv secret ...being incorrectly treated as requiring a container runtime by addingsecretto the informational command set. - Fix
GetSystemSecretsProviderto honorTOOLHIVE_SECRETS_PROVIDER=environmenteven whenSetupCompletedis false, and add unit coverage for the regression. - Add a large e2e matrix for
thv llmacross all supported clients, including a mock LLM gateway + fake browser helpers, and add anllmslice to the e2e CI workflow.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/thv/app/commands.go |
Marks secret as informational so it won’t trigger container-runtime checks. |
pkg/auth/secrets/secrets.go |
Refactors system secrets provider selection to evaluate env override before setup guard (testable helper). |
pkg/auth/secrets/secrets_test.go |
Adds unit tests to cover env override bypass + “no setup, no env” behavior. |
pkg/auth/tokensource/tokensource.go |
Adds a canonical helper for computing the env-var name for cached LLM access tokens. |
test/e2e/helpers.go |
Adds reusable fake-browser PATH stubs to support headless OIDC flows in e2e. |
test/e2e/cli_llm_setup_test.go |
Switches to the shared fake-browser helper to reduce duplication/flakiness. |
test/e2e/llm_gateway_mock.go |
Adds a mock OpenAI-compatible gateway server used by proxy/token-forwarding tests. |
test/e2e/cli_llm_all_clients_test.go |
Introduces the 16-test all-client thv llm workflow matrix (setup/teardown, proxy behaviors, token flows, edge cases). |
.github/workflows/e2e-tests.yml |
Adds an llm matrix entry to run LLM-labeled e2e tests in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3c2c9d3 to
b5d0756
Compare
b5d0756 to
1306e35
Compare
Summary
Two bugs were found while building a new e2e test suite for `thv llm` commands across all supported AI clients. This PR ships the fixes and the full test suite together.
Bug 1 — `thv secret provider environment` fails without a container runtime
`"secret"` was missing from `informationalCommands` in `cmd/thv/app/commands.go`, so `thv secret` triggered the Docker availability check. This broke `thv secret provider environment` on machines without a running container runtime.
Bug 2 — `GetSystemSecretsProvider` ignores `TOOLHIVE_SECRETS_PROVIDER` env var
`pkg/auth/secrets/secrets.go` checked `SetupCompleted` before calling `GetProviderType()`. Since `GetProviderType()` is what reads the env var and allows the `environment` provider without interactive setup, the bypass was silently skipped.
New: `test/e2e/cli_llm_all_clients_test.go` — 16 e2e tests covering the full `thv llm` workflow across all 6 supported clients using an in-process mock OIDC server and fake browser stubs (no Docker, no real credentials).
New: `test/llm-mock-env/` — standalone helper for manual testing with a local mock OIDC + LLM gateway.
Fixes #5115
Type of change
Test plan
Ran the full new suite locally with Docker available and confirmed all 16 tests pass. Also reproduced both bugs on a machine without Docker running and confirmed the fixes resolve them.
API Compatibility
Changes
Does this introduce a user-facing change?
Yes — `thv secret` subcommands no longer fail when no container runtime is available. `thv llm token` and secrets-related flows now correctly honour `TOOLHIVE_SECRETS_PROVIDER=environment` without requiring interactive secrets setup first.
Special notes for reviewers
The 16 new e2e tests use `Label("cli", "llm", "clients", "e2e")`. Adding `{title: llm, label_filter: "llm && clients"}` to `.github/workflows/e2e-tests.yml` is a recommended follow-up to run them in CI — they are ~40s and fully Docker-free.
Large PR Justification
This is an isolated PR that mostly adds e2e testing to thv llm for different clients. Cannot be split.