test(plugins): unblock TestResolveAndStage_NoInternalErrorsInHTTPErr (#1814) - #2166
Merged
Merged
Conversation
…1814) Closes the second of two skipped tests in workspace_provision_test.go that were blocked on interface refactors. The Broadcaster + CP provisioner halves landed in earlier #1814 cycles; this is the plugin-source-registry half. Refactor: - Add handlers.pluginSources interface with the 3 methods handler code actually calls (Register, Resolve, Schemes) - Compile-time assertion `var _ pluginSources = (*plugins.Registry)(nil)` catches future method-signature drift at build time - PluginsHandler.sources narrowed from *plugins.Registry to the interface; production wiring (NewPluginsHandler, WithSourceResolver) still passes *plugins.Registry — satisfies the interface Production fix (#1206 leak): - resolveAndStage's Fetch-failure path was interpolating err.Error() into the HTTP response body via `failed to fetch plugin from %s: %v`. Resolver errors routinely contain rate-limit text, github request IDs, raw HTTP body fragments, and (for local resolvers) file system paths — none has any business landing in a user's browser. - Body now carries just `failed to fetch plugin from <scheme>`; the status code already differentiates the failure shape (404 not found, 504 timeout, 502 generic). Full err detail stays in the server-side log line one statement above. Test: - 6 sub-tests covering every error path inside resolveAndStage: empty source, invalid format, unknown scheme, local path-traversal, unpinned github (PLUGIN_ALLOW_UNPINNED unset), Fetch failure with a leaky synthetic error - The Fetch-failure case plants 5 realistic leak markers in the resolver's error string (rate limit text, x-github-request-id, auth_token, ghp_-prefixed token, /etc/passwd path); the assertion fails if ANY appears in the response body - Table-driven so a future error path added to resolveAndStage gets one new row, not a copy-paste of the assertion logic Verification: - 6/6 sub-tests pass - Full workspace-server test suite passes (interface refactor is non-breaking; production caller paths unchanged) - go build ./... clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
requested a review
from hongmingwang-moleculeai
as a code owner
April 27, 2026 11:01
HongmingWang-Rabbit
enabled auto-merge
April 27, 2026 11:01
3 tasks
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
…ivity + Delegation + A2A handlers (CHUNK 1 + CHUNK 2)' (#2166) from fix/2151-chunk1-activity-delegation-a2a-integration-tests into main
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
…tion tests (#2166 blocker #2) **Step A — Go-level fail-closed** Extract a shared `requireIntegrationDBURL(t)` helper into `integration_helper_test.go` (build-tag: integration). The helper: - Returns $INTEGRATION_DB_URL when present - Calls `t.Fatalf` when the URL is empty AND any CI marker is set (`CI`, `GITHUB_ACTIONS`, or `GITEA_ACTIONS`), preventing a silent skip-to-green in CI - Calls `t.Skip` when the URL is empty AND no CI marker is set, preserving the local-dev ergonomics Update all three integration test files to use the shared helper: - delegation_ledger_integration_test.go - pending_uploads_integration_test.go - workspace_create_name_integration_test.go This closes the Go-level fail-open where a missing INTEGRATION_DB_URL in CI would cause every integration test to skip and report PASS. **Step C — Workflow bash preflight** Add a `Preflight — INTEGRATION_DB_URL must be present` step in `.gitea/workflows/handlers-postgres-integration.yml` immediately before the `go test` invocation. If the postgres-start step failed to export the variable, the preflight exits 1 with `::error::` so the job fails loud before the test binary can even start. **Step B — Workflow CoE mask** ALREADY FIXED in current main: both `detect-changes` and `integration` jobs have `continue-on-error: false` (lines 93 and 125). The context is already listed in `audit-force-merge.yml` REQUIRED_CHECKS_JSON for `main`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the second of two skipped tests in
workspace_provision_test.gothat were blocked on interface refactors. With this PR, both skipped tests called out by #1814 are unblocked and #1814 can close.Bundled in this PR
Two tightly-coupled changes — the test cannot pass without the production fix; the production fix needs the interface to be exercised in tests:
1. Interface refactor (testability)
handlers.pluginSourcesinterface with the 3 methods handler code calls:Register,Resolve,Schemesvar _ pluginSources = (*plugins.Registry)(nil)catches future method-signature drift at build timePluginsHandler.sourcesnarrowed from*plugins.Registryto the interfaceNewPluginsHandler,WithSourceResolver) still passes*plugins.Registry, which satisfies the interface — zero behavior change2. Production leak fix (#1206)
resolveAndStage's Fetch-failure path was interpolatingerr.Error()into the HTTP response body:Resolver errors routinely contain:
x-github-request-id=…)None has any business landing in a user's browser. Body now carries just
failed to fetch plugin from <scheme>; the status code (404 / 504 / 502) already differentiates the failure shape. Full err detail stays in the server-sidelog.Printfone statement above.Test
TestResolveAndStage_NoInternalErrorsInHTTPErris now a 6-subtest table covering every error path insideresolveAndStage:"""not a valid uri""weirdscheme://x""local://../etc/passwd""github://owner/repo""github://owner/repo#v1.0"The Fetch-failure case plants 5 realistic leak markers in the synthetic resolver error:
rate limit,x-github-request-id,auth_token,ghp_INTERNAL_DETAIL,/etc/passwd. The assertion fails if ANY appears in the response body. Each subtest checks status code + body-leak-marker scan independently.Pre-fix the Fetch-failure subtest fails because the body contains all 5 markers verbatim. Post-fix it passes because the body is just
"failed to fetch plugin from github".Verification
go test ./...passes (interface refactor is non-breaking)go build ./...clean#1814 status after this lands
Both originally-skipped tests are unblocked:
TestProvisionWorkspaceCP_NoInternalErrorsInBroadcast— landed in PR test(provisioner): unblock TestProvisionWorkspaceCP_NoInternalErrorsInBroadcast (#1814) #2164 (CPProvisionerAPI interface)TestResolveAndStage_NoInternalErrorsInHTTPErr— this PR (pluginSources interface)🤖 Generated with Claude Code