From ac54f96e44b37eca05b54cb2f2760441cecdf222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 10 Aug 2026 12:18:44 +0200 Subject: [PATCH] test(backend): assert three mutation-surviving code paths (#2481) Co-authored-by: Kandev Agent --- .../internal/backendapp/adapters_test.go | 196 ++++++++++++++++++ .../internal/gateway/websocket/access_test.go | 196 ++++++++++++++++++ apps/backend/internal/worktree/config_test.go | 48 +++++ 3 files changed, 440 insertions(+) diff --git a/apps/backend/internal/backendapp/adapters_test.go b/apps/backend/internal/backendapp/adapters_test.go index 78445b1b02..fe39520310 100644 --- a/apps/backend/internal/backendapp/adapters_test.go +++ b/apps/backend/internal/backendapp/adapters_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "sync" "testing" @@ -45,6 +46,201 @@ func TestBuildLifecycleLaunchRequestForwardsRemoteContributions(t *testing.T) { } } +// distinctFiller populates struct fields with non-zero values that are unique +// per field, so a mapper that drops a field (or crosswires two of the same +// type) produces an observable difference rather than two matching zero +// values. Bool is the documented exception — see fill. +type distinctFiller struct{ n int } + +// fillStruct populates every exported field of the struct pointed at by ptr. +func (f *distinctFiller) fillStruct(t *testing.T, ptr any) { + t.Helper() + v := reflect.ValueOf(ptr).Elem() + typ := v.Type() + for i := 0; i < typ.NumField(); i++ { + field := typ.Field(i) + if !v.Field(i).CanSet() { + t.Fatalf("distinctFiller: %s.%s is unexported and cannot be covered", typ, field.Name) + } + f.fill(t, v.Field(i), field.Name) + } +} + +func (f *distinctFiller) fill(t *testing.T, v reflect.Value, name string) { + t.Helper() + f.n++ + switch v.Kind() { + case reflect.String: + v.SetString(fmt.Sprintf("%s-%d", name, f.n)) + case reflect.Bool: + // Always true, never counter-derived: bool has only two values and one + // of them is the zero value, so distinctness across several bool fields + // and drop-detection are mutually exclusive. Drop-detection wins — + // a field left unmapped reads back false and fails. The cost is that + // two crosswired bool fields would still compare equal. + v.SetBool(true) + case reflect.Int, reflect.Int64: + v.SetInt(int64(f.n)) + case reflect.Pointer: + // Deliberately shallow: a zero-value allocation is enough to catch a + // dropped pointer field (nil != non-nil). It does not catch a mapper + // that substitutes a freshly allocated zero value of the same type, + // since reflect.DeepEqual compares pointees rather than addresses. + v.Set(reflect.New(v.Type().Elem())) + case reflect.Slice: + slice := reflect.MakeSlice(v.Type(), 1, 1) + f.fill(t, slice.Index(0), name+"-elem") + v.Set(slice) + case reflect.Map: + m := reflect.MakeMap(v.Type()) + key := reflect.New(v.Type().Key()).Elem() + f.fill(t, key, name+"-key") + value := reflect.New(v.Type().Elem()).Elem() + f.fill(t, value, name+"-value") + m.SetMapIndex(key, value) + v.Set(m) + default: + t.Fatalf("distinctFiller: unsupported kind %s for field %s — extend the helper", v.Kind(), name) + } +} + +// assertSameNamedFieldsEqual fails unless src and dst expose the same number of +// exported fields and every same-named pair holds a DeepEqual value. Mapper +// tests use it so that dropping a field — or adding one to both sides of a +// mirrored type pair and forgetting to wire it — fails instead of passing +// silently the way a hand-written subset of field assertions would. +func assertSameNamedFieldsEqual(t *testing.T, label string, src, dst any) { + t.Helper() + srcVal, dstVal := reflect.ValueOf(src), reflect.ValueOf(dst) + srcType, dstType := srcVal.Type(), dstVal.Type() + if srcType.NumField() != dstType.NumField() { + t.Fatalf("%s: %s has %d fields but %s has %d — the mirrored types drifted", + label, srcType, srcType.NumField(), dstType, dstType.NumField()) + } + for i := 0; i < srcType.NumField(); i++ { + name := srcType.Field(i).Name + dstField := dstVal.FieldByName(name) + if !dstField.IsValid() { + t.Errorf("%s: %s has no field %s", label, dstType, name) + continue + } + want := srcVal.Field(i).Interface() + if got := dstField.Interface(); !reflect.DeepEqual(got, want) { + t.Errorf("%s: field %s = %#v, want %#v", label, name, got, want) + } + } +} + +// TestLifecycleWorkspaceFoldersMapsEveryField covers the launch path's folder +// projection. Every agent launch flows through it and a dropped field is +// silent — no panic, no error, just missing agent configuration. +func TestLifecycleWorkspaceFoldersMapsEveryField(t *testing.T) { + if got := lifecycleWorkspaceFolders(nil); got != nil { + t.Errorf("lifecycleWorkspaceFolders(nil) = %#v, want nil", got) + } + if got := lifecycleWorkspaceFolders([]orchestratorexecutor.WorkspaceFolderSpec{}); got != nil { + t.Errorf("lifecycleWorkspaceFolders(empty) = %#v, want nil", got) + } + + filler := &distinctFiller{} + folders := make([]orchestratorexecutor.WorkspaceFolderSpec, 2) + for i := range folders { + filler.fillStruct(t, &folders[i]) + } + + got := lifecycleWorkspaceFolders(folders) + if len(got) != len(folders) { + t.Fatalf("lifecycleWorkspaceFolders returned %d folders, want %d", len(got), len(folders)) + } + for i := range folders { + assertSameNamedFieldsEqual(t, fmt.Sprintf("workspace folder %d", i), folders[i], got[i]) + } +} + +// TestLifecycleRouteOverrideMapsEveryField pins the same all-fields contract on +// the sibling route-override mapper. +func TestLifecycleRouteOverrideMapsEveryField(t *testing.T) { + if got := lifecycleRouteOverride(nil); got != nil { + t.Errorf("lifecycleRouteOverride(nil) = %#v, want nil", got) + } + + override := &orchestratorexecutor.RouteOverride{} + (&distinctFiller{}).fillStruct(t, override) + + got := lifecycleRouteOverride(override) + if got == nil { + t.Fatal("lifecycleRouteOverride returned nil for a non-nil override") + } + assertSameNamedFieldsEqual(t, "route override", *override, *got) +} + +// TestLifecycleRepoLaunchSpecsMapsEveryField pins the all-fields contract on +// the multi-repo launch spec mapper — the widest of the three (19 fields). +func TestLifecycleRepoLaunchSpecsMapsEveryField(t *testing.T) { + if got := lifecycleRepoLaunchSpecs(nil); got != nil { + t.Errorf("lifecycleRepoLaunchSpecs(nil) = %#v, want nil", got) + } + if got := lifecycleRepoLaunchSpecs([]orchestratorexecutor.RepoSpec{}); got != nil { + t.Errorf("lifecycleRepoLaunchSpecs(empty) = %#v, want nil", got) + } + + filler := &distinctFiller{} + repos := make([]orchestratorexecutor.RepoSpec, 2) + for i := range repos { + filler.fillStruct(t, &repos[i]) + } + + got := lifecycleRepoLaunchSpecs(repos) + if len(got) != len(repos) { + t.Fatalf("lifecycleRepoLaunchSpecs returned %d specs, want %d", len(got), len(repos)) + } + for i := range repos { + assertSameNamedFieldsEqual(t, fmt.Sprintf("repo spec %d", i), repos[i], got[i]) + } +} + +// TestBuildLifecycleLaunchRequestWiresMappedCollections asserts the launch +// request actually carries the three mapper outputs. A mapper can be correct +// while the wiring in buildLifecycleLaunchRequest drops it. +func TestBuildLifecycleLaunchRequestWiresMappedCollections(t *testing.T) { + filler := &distinctFiller{} + folders := make([]orchestratorexecutor.WorkspaceFolderSpec, 2) + for i := range folders { + filler.fillStruct(t, &folders[i]) + } + repos := make([]orchestratorexecutor.RepoSpec, 2) + for i := range repos { + filler.fillStruct(t, &repos[i]) + } + override := &orchestratorexecutor.RouteOverride{} + filler.fillStruct(t, override) + + launchReq := buildLifecycleLaunchRequest(&orchestratorexecutor.LaunchAgentRequest{ + WorkspaceFolders: folders, + RouteOverride: override, + Repositories: repos, + }, "/workspace", "office-profile") + + if len(launchReq.WorkspaceFolders) != len(folders) { + t.Fatalf("WorkspaceFolders length = %d, want %d", len(launchReq.WorkspaceFolders), len(folders)) + } + for i := range folders { + assertSameNamedFieldsEqual(t, fmt.Sprintf("launch workspace folder %d", i), folders[i], launchReq.WorkspaceFolders[i]) + } + + if launchReq.RouteOverride == nil { + t.Fatal("RouteOverride was dropped from the launch request") + } + assertSameNamedFieldsEqual(t, "launch route override", *override, *launchReq.RouteOverride) + + if len(launchReq.Repositories) != len(repos) { + t.Fatalf("Repositories length = %d, want %d", len(launchReq.Repositories), len(repos)) + } + for i := range repos { + assertSameNamedFieldsEqual(t, fmt.Sprintf("launch repo spec %d", i), repos[i], launchReq.Repositories[i]) + } +} + func TestBuildLifecycleLaunchRequestCarriesMCPProviders(t *testing.T) { want := []string{"github", "gitlab"} got := buildLifecycleLaunchRequest(&orchestratorexecutor.LaunchAgentRequest{ diff --git a/apps/backend/internal/gateway/websocket/access_test.go b/apps/backend/internal/gateway/websocket/access_test.go index e84b211428..d277b209c6 100644 --- a/apps/backend/internal/gateway/websocket/access_test.go +++ b/apps/backend/internal/gateway/websocket/access_test.go @@ -4,15 +4,211 @@ import ( "context" "encoding/json" "errors" + "net/http" + "net/http/httptest" "testing" "time" + "github.com/gin-gonic/gin" + "github.com/kandev/kandev/internal/auth/authn" "github.com/kandev/kandev/internal/common/logger" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" ) +// connectionAuthResult captures what requireConnectionAuth did to one request: +// whether the chain continued, the identity the downstream handler saw, and the +// HTTP response. +type connectionAuthResult struct { + recorder *httptest.ResponseRecorder + nextCalled bool + identity authn.Identity + hasID bool +} + +// runConnectionAuth drives requireConnectionAuth through a real gin router so +// both halves of the contract are observable: c.Next() reaching the downstream +// handler, and AbortWithStatusJSON stopping the chain before it. +func runConnectionAuth(t *testing.T, policy AuthPolicy, query string, preset *authn.Identity) connectionAuthResult { + t.Helper() + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger: %v", err) + } + gateway := NewGateway(log) + gateway.SetAuthPolicy(policy) + + result := connectionAuthResult{recorder: httptest.NewRecorder()} + router := gin.New() + if preset != nil { + identity := *preset + router.Use(func(c *gin.Context) { + authn.SetOnGin(c, identity) + c.Next() + }) + } + router.GET("/ws", gateway.requireConnectionAuth(), func(c *gin.Context) { + result.nextCalled = true + result.identity, result.hasID = authn.FromGin(c) + c.Status(http.StatusOK) + }) + router.ServeHTTP(result.recorder, httptest.NewRequest(http.MethodGet, "/ws"+query, nil)) + return result +} + +// TestRequireConnectionAuth covers the 401 gate in front of WebSocket upgrades +// and the VS Code / port proxy routes. +// +// Must-not-regress: the enforced-and-anonymous row. Inverting the enforcement +// check (`policy.Enforced()` instead of `!policy.Enforced()`) turns this +// middleware into a total auth bypass precisely when auth IS enforced, and +// nothing else in the suite notices. +func TestRequireConnectionAuth(t *testing.T) { + gin.SetMode(gin.TestMode) + + const patToken = "kandev_pat_valid" + sessionIdentity := authn.Identity{UserID: "user-session", Role: authn.RoleMember, SessionID: "sess-1"} + tokenIdentity := authn.Identity{UserID: "user-pat", Role: authn.RoleMember, TokenID: "tok-1"} + + enforced := func() bool { return true } + // resolvedToken is shared by the ResolveToken stubs below and reset at the + // top of every subtest, so these subtests must stay sequential — do not add + // t.Parallel() here without giving each case its own capture. + var resolvedToken string + resolveValid := func(_ context.Context, token string) (authn.Identity, bool) { + resolvedToken = token + return tokenIdentity, true + } + resolveReject := func(_ context.Context, token string) (authn.Identity, bool) { + resolvedToken = token + return authn.Identity{}, false + } + + cases := []struct { + name string + policy AuthPolicy + query string + preset *authn.Identity + wantStatus int + wantNext bool + wantUserID string + wantToken string + }{ + { + name: "no enforcement hook passes through", + policy: AuthPolicy{}, + wantStatus: http.StatusOK, + wantNext: true, + }, + { + name: "enforcement disabled passes through", + policy: AuthPolicy{Enforced: func() bool { return false }, ResolveToken: resolveValid}, + wantStatus: http.StatusOK, + wantNext: true, + }, + { + name: "enforced without identity or token is rejected", + policy: AuthPolicy{Enforced: enforced, ResolveToken: resolveValid}, + wantStatus: http.StatusUnauthorized, + wantNext: false, + }, + { + name: "enforced with an already-resolved identity passes through", + policy: AuthPolicy{Enforced: enforced}, + preset: &sessionIdentity, + wantStatus: http.StatusOK, + wantNext: true, + wantUserID: sessionIdentity.UserID, + }, + { + // Branch order matters: an established identity must win over a + // query credential. Resolving ?token= first would let any caller + // overwrite the identity the HTTP auth middleware already + // authenticated — wantToken "" pins that ResolveToken never runs. + name: "enforced with an identity present ignores the query token", + policy: AuthPolicy{Enforced: enforced, ResolveToken: resolveValid}, + query: "?token=" + patToken, + preset: &sessionIdentity, + wantStatus: http.StatusOK, + wantNext: true, + wantUserID: sessionIdentity.UserID, + wantToken: "", + }, + { + name: "enforced with a valid query token passes through and sets the identity", + policy: AuthPolicy{Enforced: enforced, ResolveToken: resolveValid}, + query: "?token=" + patToken, + wantStatus: http.StatusOK, + wantNext: true, + wantUserID: tokenIdentity.UserID, + wantToken: patToken, + }, + { + name: "enforced with a rejected query token is rejected", + policy: AuthPolicy{Enforced: enforced, ResolveToken: resolveReject}, + query: "?token=" + patToken, + wantStatus: http.StatusUnauthorized, + wantNext: false, + wantToken: patToken, + }, + { + name: "enforced with a query token but no resolver is rejected", + policy: AuthPolicy{Enforced: enforced}, + query: "?token=" + patToken, + wantStatus: http.StatusUnauthorized, + wantNext: false, + }, + { + name: "enforced with an empty query token is rejected", + policy: AuthPolicy{Enforced: enforced, ResolveToken: resolveValid}, + query: "?token=", + wantStatus: http.StatusUnauthorized, + wantNext: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resolvedToken = "" + got := runConnectionAuth(t, tc.policy, tc.query, tc.preset) + + if got.recorder.Code != tc.wantStatus { + t.Errorf("status = %d, want %d (body %s)", got.recorder.Code, tc.wantStatus, got.recorder.Body.String()) + } + if got.nextCalled != tc.wantNext { + t.Errorf("downstream handler called = %v, want %v", got.nextCalled, tc.wantNext) + } + if resolvedToken != tc.wantToken { + t.Errorf("ResolveToken received %q, want %q", resolvedToken, tc.wantToken) + } + if tc.wantStatus == http.StatusUnauthorized { + assertAuthRequiredBody(t, got.recorder) + return + } + if tc.wantUserID != "" { + if !got.hasID { + t.Fatalf("downstream handler saw no identity, want user %q", tc.wantUserID) + } + if got.identity.UserID != tc.wantUserID { + t.Errorf("identity user = %q, want %q", got.identity.UserID, tc.wantUserID) + } + } + }) + } +} + +func assertAuthRequiredBody(t *testing.T, rec *httptest.ResponseRecorder) { + t.Helper() + var body map[string]string + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("401 body is not JSON (%q): %v", rec.Body.String(), err) + } + if body["error"] != "authentication required" { + t.Errorf("401 body error = %q, want %q", body["error"], "authentication required") + } +} + func newAccessTestHub(t *testing.T) *Hub { t.Helper() log, err := logger.NewFromZap(zap.NewNop()) diff --git a/apps/backend/internal/worktree/config_test.go b/apps/backend/internal/worktree/config_test.go index fb6b600a2c..1332c1a0ad 100644 --- a/apps/backend/internal/worktree/config_test.go +++ b/apps/backend/internal/worktree/config_test.go @@ -2,6 +2,54 @@ package worktree import "testing" +// TestConfigValidateBranchPrefix pins both halves of the default-application +// branch in Config.Validate: an empty prefix picks up DefaultBranchPrefix, and +// an explicitly configured prefix survives untouched. Inverting the condition +// (`!= ""`) breaks both and must fail here. +func TestConfigValidateBranchPrefix(t *testing.T) { + cases := []struct { + name string + given string + want string + }{ + {name: "empty prefix falls back to the default", given: "", want: DefaultBranchPrefix}, + {name: "explicit prefix is preserved", given: "kandev/", want: "kandev/"}, + {name: "explicit prefix without separator is preserved", given: "wip", want: "wip"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := &Config{BranchPrefix: tc.given} + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate() error = %v, want nil", err) + } + if cfg.BranchPrefix != tc.want { + t.Errorf("BranchPrefix = %q, want %q", cfg.BranchPrefix, tc.want) + } + }) + } +} + +// TestConfigValidateLeavesOtherFieldsAlone guards against Validate growing a +// side effect on fields it has no business defaulting. +func TestConfigValidateLeavesOtherFieldsAlone(t *testing.T) { + cfg := &Config{ + Enabled: true, + TasksBasePath: "/tmp/kandev-tasks", + BranchPrefix: "feat/", + FetchTimeoutSeconds: 11, + PullTimeoutSeconds: 22, + } + want := *cfg + + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate() error = %v, want nil", err) + } + if *cfg != want { + t.Errorf("Validate() mutated the config: got %+v, want %+v", *cfg, want) + } +} + func TestTaskDirSuffix(t *testing.T) { const id = "61ccfd2c-1121-4226-99ab-8d9a60a57e6e"