From e2291051b79df32c1406e1202dab215836fcbe57 Mon Sep 17 00:00:00 2001 From: Molecule AI Infra Lead Date: Sun, 10 May 2026 09:42:03 +0000 Subject: [PATCH] [infra-lead-agent] fix(workspace-server): unmask compile errors hidden by SourceResolver redeclaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI on main is RED — `docker build` fails on `go build ./cmd/server` with "SourceResolver redeclared in this block" (issue surfaced after Gitea Actions runner Docker socket was fixed and the build actually progressed to the Go compile step). Once that one error was resolved, several more latent regressions surfaced — all of them masked for an unknown number of merges by the upstream `SourceResolver` collision. Smallest set of mechanical fixes to restore a green production build: 1. internal/plugins/drift_sweeper.go: rename the duplicate `SourceResolver` interface (Resolve+Schemes) to `RegistryResolver` and slim it to just `Schemes()` — the only method the sweeper uses. The per-scheme `SourceResolver` (Scheme+Fetch) in source.go remains the canonical fetcher interface implemented by GithubResolver + LocalResolver. 2. internal/plugins/drift_sweeper_test.go: drop the unused `Resolve` method on `stubResolver`, drop unused `database/sql` import, rename the interface-satisfaction test, point it at `RegistryResolver`. 3. internal/handlers/plugins.go: `Sources()` was returning `plugins.SourceResolver` but `h.sources` is the registry shape (`pluginSources`). Switch return type to `plugins.RegistryResolver` so the existing `pluginSources` value satisfies it via `Schemes()`. 4. internal/handlers/restart_signals.go: `rewriteForDocker` was declared as a package-level function but referenced `h.provisioner` — must be a method on `*WorkspaceHandler`. Convert to method, update both callers in `resolveAgentURLForRestartSignal`. 5. internal/handlers/restart_signals_test.go: remove the test-only shim that wrapped the old package-level `rewriteForDocker` (now redundant + collides with the production method). 6. internal/handlers/admin_plugin_drift.go: drop unused `context` import. 7. internal/router/router.go: move the `/admin/plugin-updates-pending` block from before `plgh` is declared to after — the previous placement was a forward reference inside the same `Setup` scope. 8. cmd/server/main.go: `router.Setup` expects a per-scheme `plugins.SourceResolver`, but main was passing the whole `*plugins.Registry`. Pass the shared `*GithubResolver` instance instead, while keeping it registered in the registry that drives the drift sweeper. Preserves the documented "shared resolver state" intent (one GithubResolver across both surfaces). Verified locally with the same command the publish workflow runs inside Docker: CGO_ENABLED=0 GOOS=linux go build -ldflags '...' -o /platform ./cmd/server Build is green. `go vet ./...` still flags pre-existing test-only issues in restart_signals_test.go and a few others that DO NOT block `docker build` (CI only runs `go build`, not `go test` / `go vet` for the publish workflow). Those should be cleaned up in a follow-up by Core-Platform; calling them out in the PR description. Owner note: workspace-server is Core-Platform's surface, but CI health on main is Infra Lead's charter and Release Manager has flagged release blocked. Outbound A2A delegate_task is currently failing across the platform with `AttributeError: 'str' object has no attribute 'get'`, so I couldn't hand this off to Core-Platform Lead in real time — landing the fix here under triage authority and tagging Core-Platform for review. Co-Authored-By: Claude Opus 4.7 --- workspace-server/cmd/server/main.go | 17 +++++++---- .../internal/handlers/admin_plugin_drift.go | 1 - .../internal/handlers/delegation_test.go | 1 - workspace-server/internal/handlers/plugins.go | 10 ++++++- .../internal/handlers/restart_signals.go | 11 +++++-- .../internal/handlers/restart_signals_test.go | 7 ++--- .../internal/plugins/drift_sweeper.go | 30 +++++++++++++------ .../internal/plugins/drift_sweeper_test.go | 19 ++++++------ workspace-server/internal/router/router.go | 23 ++++++++++---- 9 files changed, 78 insertions(+), 41 deletions(-) diff --git a/workspace-server/cmd/server/main.go b/workspace-server/cmd/server/main.go index 1d6ff911..a20cddb2 100644 --- a/workspace-server/cmd/server/main.go +++ b/workspace-server/cmd/server/main.go @@ -332,13 +332,18 @@ func main() { cronSched.SetChannels(channelMgr) // Router - // Plugin registry — created before Setup so the same registry is shared - // between the PluginsHandler (for installs) and the drift sweeper (for - // drift detection). github:// sources always work; local:// sources - // require a plugins/ dir on disk (nil in CP/SaaS mode). + // Plugin resolver + registry — the github:// resolver is shared between + // the PluginsHandler (for installs, via router.Setup → WithSourceResolver) + // and a local registry that the drift sweeper consumes (for drift + // detection). Sharing the resolver instance preserves the documented + // intent of unified resolver state (e.g. rate limiters, in-process + // caches) across both surfaces. local:// sources require a plugins/ + // dir on disk and live entirely inside PluginsHandler's internal + // registry, which is fine — drift detection is github-only by design. + githubResolver := plugins.NewGithubResolver() pluginRegistry := plugins.NewRegistry() - pluginRegistry.Register(plugins.NewGithubResolver()) - r := router.Setup(hub, broadcaster, prov, platformURL, configsDir, wh, channelMgr, memBundle, pluginRegistry) + pluginRegistry.Register(githubResolver) + r := router.Setup(hub, broadcaster, prov, platformURL, configsDir, wh, channelMgr, memBundle, githubResolver) // Plugin drift sweeper — periodic detection of upstream plugin version drift // (core#123). Scans workspace_plugins rows where tracked_ref != 'none', diff --git a/workspace-server/internal/handlers/admin_plugin_drift.go b/workspace-server/internal/handlers/admin_plugin_drift.go index 1082c1d6..3ceb1166 100644 --- a/workspace-server/internal/handlers/admin_plugin_drift.go +++ b/workspace-server/internal/handlers/admin_plugin_drift.go @@ -8,7 +8,6 @@ package handlers // POST /admin/plugin-updates/:id/apply — apply a queued drift update import ( - "context" "database/sql" "errors" "fmt" diff --git a/workspace-server/internal/handlers/delegation_test.go b/workspace-server/internal/handlers/delegation_test.go index 427e71b2..38c63206 100644 --- a/workspace-server/internal/handlers/delegation_test.go +++ b/workspace-server/internal/handlers/delegation_test.go @@ -1262,4 +1262,3 @@ func TestExecuteDelegation_CleanProxyResponse_Unchanged(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } -} diff --git a/workspace-server/internal/handlers/plugins.go b/workspace-server/internal/handlers/plugins.go index 78e182ba..dbb7e6ad 100644 --- a/workspace-server/internal/handlers/plugins.go +++ b/workspace-server/internal/handlers/plugins.go @@ -112,7 +112,15 @@ func (h *PluginsHandler) WithInstanceIDLookup(lookup InstanceIDLookup) *PluginsH // Sources returns the underlying plugin source registry. Used by main.go to // pass the same registry to the drift sweeper so both share resolver state. -func (h *PluginsHandler) Sources() plugins.SourceResolver { +// +// Returns plugins.RegistryResolver (the slim Schemes()-only interface the +// drift sweeper consumes), not plugins.SourceResolver — `h.sources` is the +// registry shape (Register/Resolve/Schemes), not the per-scheme fetcher +// shape (Scheme/Fetch). The previous return type was a leftover from +// before #1814 narrowed `h.sources` to the `pluginSources` interface and +// caused a compile error once the masking duplicate `SourceResolver` in +// `internal/plugins/drift_sweeper.go` was removed. +func (h *PluginsHandler) Sources() plugins.RegistryResolver { return h.sources } diff --git a/workspace-server/internal/handlers/restart_signals.go b/workspace-server/internal/handlers/restart_signals.go index 81cb9200..e6d67fd3 100644 --- a/workspace-server/internal/handlers/restart_signals.go +++ b/workspace-server/internal/handlers/restart_signals.go @@ -120,7 +120,7 @@ func (h *WorkspaceHandler) resolveAgentURLForRestartSignal(ctx context.Context, // Try Redis cache first. agentURL, err := db.GetCachedURL(ctx, workspaceID) if err == nil && agentURL != "" { - return rewriteForDocker(agentURL, workspaceID), nil + return h.rewriteForDocker(agentURL, workspaceID), nil } // Cache miss — fall back to DB. @@ -136,13 +136,18 @@ func (h *WorkspaceHandler) resolveAgentURLForRestartSignal(ctx context.Context, } agentURL = *urlNullable _ = db.CacheURL(ctx, workspaceID, agentURL) - return rewriteForDocker(agentURL, workspaceID), nil + return h.rewriteForDocker(agentURL, workspaceID), nil } // rewriteForDocker rewrites a 127.0.0.1 agent URL to the Docker-DNS form // when the platform is running inside a Docker container. When platform is // on the host (non-Docker), 127.0.0.1 IS the host and the original URL works. -func rewriteForDocker(agentURL, workspaceID string) string { +// +// Method receiver `h` is required to access h.provisioner; was previously +// declared as a package-level function which referred to an undefined `h` +// — compile error masked by the upstream `SourceResolver` redeclaration in +// internal/plugins/drift_sweeper.go. +func (h *WorkspaceHandler) rewriteForDocker(agentURL, workspaceID string) string { if platformInDocker && h.provisioner != nil { // Only rewrite if the URL points to localhost (the ephemeral port // binding the container published to the host). Internal Docker diff --git a/workspace-server/internal/handlers/restart_signals_test.go b/workspace-server/internal/handlers/restart_signals_test.go index d9278e2c..6f87bb85 100644 --- a/workspace-server/internal/handlers/restart_signals_test.go +++ b/workspace-server/internal/handlers/restart_signals_test.go @@ -324,7 +324,6 @@ func setupTestRedisWithURL(t *testing.T, url string) *miniredis.Miniredis { return mr } -// rewriteForDocker is exported from restart_signals.go so it can be tested here. -func (h *WorkspaceHandler) rewriteForDocker(agentURL, workspaceID string) string { - return rewriteForDocker(agentURL, workspaceID) -} +// (the previous test-only shim wrapping a package-level rewriteForDocker +// has been removed: production rewriteForDocker is now a *WorkspaceHandler +// method directly — see internal/handlers/restart_signals.go.) diff --git a/workspace-server/internal/plugins/drift_sweeper.go b/workspace-server/internal/plugins/drift_sweeper.go index 9b6399d5..6036010b 100644 --- a/workspace-server/internal/plugins/drift_sweeper.go +++ b/workspace-server/internal/plugins/drift_sweeper.go @@ -9,7 +9,8 @@ package plugins // 1. SELECTs workspace_plugins rows where tracked_ref != 'none' // AND installed_sha IS NOT NULL (skip pre-migration rows with NULL SHA). // 2. For each row, resolves the tracked ref to its current upstream SHA -// using the appropriate SourceResolver. +// using the appropriate per-scheme SourceResolver (via the +// RegistryResolver passed in at sweeper start). // 3. If the resolved SHA differs from installed_sha → drift detected. // 4. On drift, INSERT INTO plugin_update_queue (ON CONFLICT DO NOTHING so // a re-drift while a row is still pending is a no-op). @@ -61,10 +62,21 @@ const DriftSweepInterval = 1 * time.Hour // that handles Gitea instances on high-latency links. const ResolveRefDeadline = 60 * time.Second -// SourceResolver resolves plugin sources to installable directories. -// Satisfied by *Registry (which wraps GithubResolver + LocalResolver). -type SourceResolver interface { - Resolve(source Source) (SourceResolver, error) +// RegistryResolver is the slim shape of a plugin source-scheme registry that +// the drift sweeper depends on. It exists distinct from `SourceResolver` (the +// per-scheme fetcher interface in source.go) so the two type names don't +// collide inside this package — historically both were named `SourceResolver`, +// which broke `go build` (issue: docker build fails with "SourceResolver +// redeclared in this block"). Satisfied by *Registry, which holds the set of +// per-scheme resolvers and exposes the list of registered scheme prefixes. +// +// Only `Schemes()` is used by the sweeper today (to strip the scheme prefix +// from `source_raw` before handing the spec to GithubResolver). If the +// sweeper ever needs to dispatch via the registry (e.g. to support +// non-github schemes for drift detection), add the resolution method back +// here — but keep it returning `SourceResolver` so it stays compatible with +// `*Registry.Resolve`. +type RegistryResolver interface { Schemes() []string } @@ -74,7 +86,7 @@ type SourceResolver interface { // // Registers itself via atexits in cmd/server/main.go so the process // shuts down cleanly on SIGTERM. -func StartPluginDriftSweeper(ctx context.Context, resolver SourceResolver) { +func StartPluginDriftSweeper(ctx context.Context, resolver RegistryResolver) { if resolver == nil { log.Println("Plugin drift sweeper: resolver is nil — sweeper disabled") return @@ -107,7 +119,7 @@ func StartPluginDriftSweeper(ctx context.Context, resolver SourceResolver) { // sweepDriftOnce runs one full drift-detection cycle. // Errors are non-fatal — each row is handled independently so a single // slow row doesn't block the rest of the sweep. -func sweepDriftOnce(parent context.Context, resolver SourceResolver) { +func sweepDriftOnce(parent context.Context, resolver RegistryResolver) { ctx, cancel := context.WithTimeout(parent, 10*time.Minute) defer cancel() @@ -170,7 +182,7 @@ func sweepDriftOnce(parent context.Context, resolver SourceResolver) { // resolveLatestSHA resolves the tracked ref to its current upstream SHA. // Handles both github:// and local:// sources; local sources are skipped // (no meaningful upstream to drift against). -func resolveLatestSHA(ctx context.Context, resolver SourceResolver, sourceRaw, trackedRef string) (string, error) { +func resolveLatestSHA(ctx context.Context, resolver RegistryResolver, sourceRaw, trackedRef string) (string, error) { // Strip the scheme prefix to get the raw spec. // sourceRaw is stored as the full string, e.g. "github://owner/repo#tag:v1.0.0" spec := sourceRaw @@ -231,7 +243,7 @@ func queueDriftEntry(ctx context.Context, workspaceID, pluginName, trackedRef, c // ───────────────────────────────────────────────────────────────────────────── // SweepDriftOnceForTest exposes sweepDriftOnce for package-level testing. -func SweepDriftOnceForTest(parent context.Context, resolver SourceResolver) { +func SweepDriftOnceForTest(parent context.Context, resolver RegistryResolver) { sweepDriftOnce(parent, resolver) } diff --git a/workspace-server/internal/plugins/drift_sweeper_test.go b/workspace-server/internal/plugins/drift_sweeper_test.go index 3370dce1..6bafe4f5 100644 --- a/workspace-server/internal/plugins/drift_sweeper_test.go +++ b/workspace-server/internal/plugins/drift_sweeper_test.go @@ -2,20 +2,17 @@ package plugins import ( "context" - "database/sql" "errors" "testing" ) -// stubResolver is a SourceResolver that always returns a stub github resolver. +// stubResolver satisfies plugins.RegistryResolver — the slim shape the +// drift sweeper consumes (just `Schemes()`). Tests that dispatch by scheme +// build a per-scheme SourceResolver directly via NewGithubResolver(). type stubResolver struct { schemes []string } -func (s *stubResolver) Resolve(source Source) (SourceResolver, error) { - return NewGithubResolver(), nil -} - func (s *stubResolver) Schemes() []string { return s.schemes } func TestResolveRef_RejectsBareSpec(t *testing.T) { @@ -156,8 +153,10 @@ func TestPluginUpdateQueueRow_Struct(t *testing.T) { } } -// TestSourceResolverInterface_StubResolver verifies that a stub resolver -// satisfies the SourceResolver interface. -func TestSourceResolverInterface_StubResolver(t *testing.T) { - var _ SourceResolver = (*stubResolver)(nil) +// TestRegistryResolverInterface_StubResolver verifies that a stub resolver +// satisfies the RegistryResolver interface — the slim shape the drift +// sweeper consumes. (The per-scheme SourceResolver interface lives in +// source.go and is exercised by GithubResolver / LocalResolver tests.) +func TestRegistryResolverInterface_StubResolver(t *testing.T) { + var _ RegistryResolver = (*stubResolver)(nil) } diff --git a/workspace-server/internal/router/router.go b/workspace-server/internal/router/router.go index 585e4f7c..0e8f79e7 100644 --- a/workspace-server/internal/router/router.go +++ b/workspace-server/internal/router/router.go @@ -501,12 +501,10 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // Admin — plugin version-subscription drift queue (core#123). // List pending drift entries and apply approved updates. - { - driftH := handlers.NewAdminPluginDriftHandler(plgh) - adminAuth := r.Group("", middleware.AdminAuth(db.DB)) - adminAuth.GET("/admin/plugin-updates-pending", driftH.ListPending) - adminAuth.POST("/admin/plugin-updates/:id/apply", driftH.Apply) - } + // Moved below plgh declaration — was previously here but referenced + // `plgh` before its `:=` at the same function level, which started + // failing once the upstream `SourceResolver` redeclaration was fixed + // (the prior compile-time block was masking the forward reference). // Admin — test token minting (issue #6). Hidden in production via TestTokensEnabled(). // NOT behind AdminAuth — this is the bootstrap endpoint E2E tests and @@ -646,6 +644,19 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // unpack locally instead of going through Docker exec. wsAuth.GET("/plugins/:name/download", plgh.Download) + // Admin — plugin version-subscription drift queue (core#123). + // List pending drift entries and apply approved updates. + // Wired here (after plgh) rather than in the admin block above so the + // `plgh` reference resolves — the previous placement was a forward + // reference, masked by the upstream `SourceResolver` redeclaration in + // internal/plugins/drift_sweeper.go. + { + driftH := handlers.NewAdminPluginDriftHandler(plgh) + adminAuth := r.Group("", middleware.AdminAuth(db.DB)) + adminAuth.GET("/admin/plugin-updates-pending", driftH.ListPending) + adminAuth.POST("/admin/plugin-updates/:id/apply", driftH.Apply) + } + // Bundles — #164 + #165: both gated behind AdminAuth. // POST /bundles/import — CRITICAL: anon creation of arbitrary workspaces // with user-supplied config (system prompts, -- 2.45.2