diff --git a/.github/workflows/backend-tests.yml b/.github/workflows/backend-tests.yml index 9f878fac6f..24c46b7d60 100644 --- a/.github/workflows/backend-tests.yml +++ b/.github/workflows/backend-tests.yml @@ -253,21 +253,68 @@ jobs: apps/backend/test-results-${{ matrix.shard }}.json retention-days: 30 + test_ambient_env: + name: Backend (ambient integration env) + runs-on: ubuntu-latest + # test_shards runs in the same clean container and never sets these vars, + # so it cannot catch a package that starts reading them again — the class + # of bug that shipped alongside internal/gitlab's host-trust tests (they + # only passed because CI never set KANDEV_GITLAB_HOST/GITLAB_HOST/ + # GITLAB_TOKEN). This job poisons them with obviously-fake .invalid-TLD + # values so both GitLab-facing packages are forced to prove they stay + # hermetic instead of accidentally depending on an unset ambient env. + container: ghcr.io/kdlbs/kandev-ci:build-latest + defaults: + run: + shell: bash + working-directory: apps/backend + + steps: + - name: Checkout code + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + with: + persist-credentials: false + + - name: Mark workspace safe for git + shell: bash + working-directory: ${{ github.workspace }} + run: git config --global --add safe.directory "$GITHUB_WORKSPACE" + + - name: Cache Go modules + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: | + ~/go/pkg/mod + ~/.cache/go-build + key: go-ambient-env-${{ runner.os }}-${{ hashFiles('apps/backend/go.mod', 'apps/backend/go.sum') }} + restore-keys: | + go-ambient-env-${{ runner.os }}- + go-${{ runner.os }}- + + - name: Run GitLab-facing packages under a poisoned host environment + env: + KANDEV_GITLAB_HOST: https://gitlab.ambient-ci.invalid + GITLAB_HOST: https://gitlab.ambient-ci.invalid + GITLAB_TOKEN: glpat-ambient-ci-not-a-real-token + run: go test -race ./internal/gitlab/... ./internal/agentctl/server/process/... + test: name: Run Backend Tests runs-on: ubuntu-latest - needs: [static_checks, test_shards] + needs: [static_checks, test_shards, test_ambient_env] if: ${{ always() }} steps: - name: Require backend checks to pass env: STATIC_CHECKS_RESULT: ${{ needs.static_checks.result }} TEST_SHARDS_RESULT: ${{ needs.test_shards.result }} + TEST_AMBIENT_ENV_RESULT: ${{ needs.test_ambient_env.result }} run: | set -euo pipefail - if [[ "${STATIC_CHECKS_RESULT}" != "success" || "${TEST_SHARDS_RESULT}" != "success" ]]; then + if [[ "${STATIC_CHECKS_RESULT}" != "success" || "${TEST_SHARDS_RESULT}" != "success" || "${TEST_AMBIENT_ENV_RESULT}" != "success" ]]; then echo "Static checks: ${STATIC_CHECKS_RESULT}" echo "Test shards: ${TEST_SHARDS_RESULT}" + echo "Ambient env tests: ${TEST_AMBIENT_ENV_RESULT}" exit 1 fi diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 97a7271d13..e837a4e109 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -233,6 +233,7 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - **Filesystem safety checks:** Carry the original `os.Lstat` `FileInfo` (or opened handle) through every decision; do not re-stat a validated path, which reopens a TOCTOU window before side effects. Test mismatches before writes or manager commands. - **Git indexed-environment tests:** When a test supplies `GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_n`, and `GIT_CONFIG_VALUE_n`, clear every inherited indexed key first and restore it with `t.Cleanup`. `internal/gitconfigenv` ignores indexes past the count; uncleared parent indexes silently alter the block instead of failing loudly. - **Derived environment contracts:** Drive derived values through the production producer/wiring seam instead of manually seeding keys; pair consumer/process coverage with a producer-boundary assertion so missing publication fails. + - **GitLab host-trust tests:** `internal/gitlab`'s `TestMain` in `goleak_test.go` calls the scrub in `env_hermetic_test.go`; `internal/agentctl/server/process`'s `TestMain` clears ambient `KANDEV_GITLAB_HOST`/`GITLAB_HOST`/`GITLAB_TOKEN`. `test_ambient_env` runs both poisoned to catch a regression neither package's clean-env CI shard could catch. - **Full test output:** For local full-suite pass/fail validation, prefer plain `go test -race ./...`. `go test -json ./...` can emit very large JSONL streams; if a wrapper or tracing tool truncates the stream mid-record, downstream JSON parsing may fail even when Go tests passed. Use JSON output mainly for CI artifacts or test-report tooling that explicitly requires it. - **Private executable helpers:** When library code starts `os.Executable()` in a private helper mode, do not rely only on a command package's dispatch or package-specific `TestMain`: an importing package's test binary can be the executable and recursively run tests. Use either an explicitly marked private env-and-argument trampoline before normal test entry, or inject a dedicated helper executable. Cover it from a different importing package, not only the helper package. @@ -245,7 +246,7 @@ Every long-running goroutine must have a single owner with explicit start and st - **E2E reset invariant:** `seedData`/backend are worker-scoped, so any workspace-scoped state a global poller reads (for example `github_review_watches`) must be deleted in `cmd/kandev/e2e_reset.go` before task deletion — otherwise the poller recreates rows mid-reset and later tests see duplicates. Add a `Delete...ByWorkspace` cascade when introducing a new poller-backed entity. - **Cancellation:** the goroutine selects on `ctx.Done()` (or `stopCh`) in every long wait. Never use `time.Sleep` in a retry/backoff loop — use `time.NewTimer` inside a `select` that also watches the shutdown signal (see `lifecycle.StreamManager.sleepOrStop`). - **Detached helpers:** event handlers and short-lived `go func()` calls in `internal/orchestrator/` and `internal/agent/runtime/lifecycle/` must accept a cancellable context (or check the owning type's shutdown signal) and return promptly when it fires. -- **Leak testing:** packages that spawn goroutines add `goleak.VerifyTestMain(m)` in a per-package `TestMain`. New packages of this kind must follow suit. When a third-party background goroutine genuinely can't be drained, suppress it with `goleak.IgnoreTopFunction(...)` and leave a comment explaining why. Currently instrumented: `internal/gateway/websocket/`, `internal/agent/runtime/lifecycle/`, `internal/agentctl/server/process/`, `internal/orchestrator/`, `internal/github/`, `internal/jira/`, `internal/linear/`, `internal/integrations/healthpoll/`. +- **Leak testing:** packages that spawn goroutines add `goleak.VerifyTestMain(m)` in a per-package `TestMain`. New packages of this kind must follow suit. When a third-party background goroutine genuinely can't be drained, suppress it with `goleak.IgnoreTopFunction(...)` and leave a comment explaining why. Currently instrumented: `internal/gateway/websocket/`, `internal/agent/runtime/lifecycle/`, `internal/agentctl/server/process/`, `internal/orchestrator/`, `internal/github/`, `internal/gitlab/`, `internal/jira/`, `internal/linear/`, `internal/integrations/healthpoll/`. ## Backups diff --git a/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go b/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go index 23bc7906f3..a86b4b883c 100644 --- a/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go +++ b/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go @@ -330,3 +330,35 @@ exit 8 t.Fatalf("result = %+v", result) } } + +// TestMainScrubsAmbientGitLabEnvironment guards the TestMain hermeticity +// scrub in testmain_test.go. If that scrub is ever removed, this fails +// loudly instead of letting the parent shell's GitLab host/token variables +// silently re-arm GitLab host-trust test failures. +func TestMainScrubsAmbientGitLabEnvironment(t *testing.T) { + for _, name := range []string{gitLabHostEnv, legacyGitLabHostEnv, gitLabTokenEnv} { + if _, ok := os.LookupEnv(name); ok { + t.Fatalf("%s is set during tests; TestMain must clear it", name) + } + } +} + +// TestGitOperatorEnvironmentValueReachesGitLabHostDetection covers the +// os.Environ() fallback in environmentValues (git.go:117-133): a GitOperator +// built with a nil environment provider (NewGitOperator(dir, log, nil), as +// every CreatePR test in this package does) reads KANDEV_GITLAB_HOST from +// the ambient process environment, and that value reaches GitLab remote-host +// validation the same way git.go:1183 wires it into parseGitLabRepoInfo. +func TestGitOperatorEnvironmentValueReachesGitLabHostDetection(t *testing.T) { + t.Setenv(gitLabHostEnv, "https://gitlab.selfhosted.example") + op := NewGitOperator(t.TempDir(), newTestLogger(t), nil) + + matched, err := parseGitLabRepoInfo("git@gitlab.selfhosted.example:group/widgets.git", op.environmentValue(gitLabHostEnv)) + if err != nil || matched == nil || matched.Origin != "https://gitlab.selfhosted.example" { + t.Fatalf("parseGitLabRepoInfo(matching remote) = %+v, %v", matched, err) + } + + if _, err := parseGitLabRepoInfo("git@gitlab.com:group/widgets.git", op.environmentValue(gitLabHostEnv)); err == nil { + t.Fatal("gitlab.com remote unexpectedly accepted when ambient KANDEV_GITLAB_HOST names a different host") + } +} diff --git a/apps/backend/internal/agentctl/server/process/testmain_test.go b/apps/backend/internal/agentctl/server/process/testmain_test.go index b1538d994d..e6ebfb8ded 100644 --- a/apps/backend/internal/agentctl/server/process/testmain_test.go +++ b/apps/backend/internal/agentctl/server/process/testmain_test.go @@ -26,6 +26,7 @@ import ( const kandevTestFixtureEnv = "KANDEV_TEST_FIXTURE" const gitFetchRaceRealGitEnv = "KANDEV_GIT_FETCH_RACE_REAL_GIT" const gitFetchRaceWorktreeEnv = "KANDEV_GIT_FETCH_RACE_WORKTREE" +const legacyGitLabHostEnv = "GITLAB_HOST" // TestMain branches into fixture-binary mode when the activation env var // is set; otherwise it runs the test suite normally — wrapped in goleak so @@ -36,6 +37,16 @@ func TestMain(m *testing.M) { runFixture(spec) return } + // Tests build GitOperator with a nil environment (NewGitOperator(dir, log, + // nil)), which falls back to os.Environ() (environmentValues in git.go). + // An inherited KANDEV_GITLAB_HOST / GITLAB_HOST / GITLAB_TOKEN then leaks + // into GitLab remote-host detection and PR-provider auth, failing tests that + // assume these are unset. Scrub them here so the suite is hermetic regardless + // of the parent shell's environment; tests that need a specific value set it + // explicitly via t.Setenv. + _ = os.Unsetenv(gitLabHostEnv) + _ = os.Unsetenv(legacyGitLabHostEnv) + _ = os.Unsetenv(gitLabTokenEnv) goleak.VerifyTestMain(m) } diff --git a/apps/backend/internal/gitlab/service_config_test.go b/apps/backend/internal/gitlab/service_config_test.go index 67f53bd6e8..eadd403af1 100644 --- a/apps/backend/internal/gitlab/service_config_test.go +++ b/apps/backend/internal/gitlab/service_config_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http/httptest" "reflect" + "strings" "sync" "testing" "time" @@ -625,3 +626,60 @@ func assertWorkingWorkspaceConfig(t *testing.T, store *Store, secrets *configTes t.Fatalf("preserved secret = %q, want %q", gotToken, wantToken) } } + +func TestTrustedEnvironmentTokenHostPrecedence(t *testing.T) { + tests := []struct { + name string + kandevHost string + gitlabHost string + startupHost string + want string + }{ + {name: "kandev host wins over gitlab host and startup host", kandevHost: "https://gitlab.kandev.example", gitlabHost: "https://gitlab.legacy.example", startupHost: "https://gitlab.startup.example", want: "https://gitlab.kandev.example"}, + {name: "gitlab host wins over startup host when kandev host unset", gitlabHost: "https://gitlab.legacy.example", startupHost: "https://gitlab.startup.example", want: "https://gitlab.legacy.example"}, + {name: "startup host used when neither env var is set", startupHost: "https://gitlab.startup.example", want: "https://gitlab.startup.example"}, + {name: "unparseable kandev host yields empty trusted host", kandevHost: "ftp://gitlab.kandev.example", startupHost: "https://gitlab.startup.example", want: ""}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(envKandevGitLabHost, test.kandevHost) + t.Setenv(envGitLabHost, test.gitlabHost) + if got := trustedEnvironmentTokenHost(test.startupHost); got != test.want { + t.Fatalf("trustedEnvironmentTokenHost(%q) = %q, want %q", test.startupHost, got, test.want) + } + }) + } +} + +// TestEnvironmentTokenTrustsSelfHostedOriginOverStartupHost covers the +// self-hosted deployment path: KANDEV_GITLAB_HOST names the trusted origin, +// which may differ from the startup host NewService was constructed with. +// This is the behaviour the ambient ci-machine env was accidentally +// exercising, and the mirror image of +// TestEnvironmentTokenRejectsUntrustedWorkspaceHostBeforeClientConstruction. +func TestEnvironmentTokenTrustsSelfHostedOriginOverStartupHost(t *testing.T) { + t.Setenv(envKandevGitLabHost, "https://gitlab.selfhosted.example") + t.Setenv(secretNameToken, "glpat-selfhosted-secret") + store := newTestStore(t) + seedWorkspace(t, store, "workspace-a") + service := NewService("https://gitlab.startup.example", NewNoopClient(DefaultHost), AuthMethodNone, nil, newTestLogger(t)) + service.SetStore(store) + service.SetWorkspaceSecretStore(&configTestSecrets{values: make(map[string]string)}) + service.workspaceClientFn = func(_ context.Context, cfg *GitLabConfig, token string) (Client, error) { + return NewMockClient(cfg.Host), nil + } + + accepted := service.TestConfigForWorkspace(t.Context(), "workspace-a", &SetConfigRequest{ + Host: "https://gitlab.selfhosted.example", AuthMethod: AuthMethodEnvironment, + }) + if !accepted.OK { + t.Fatalf("self-hosted origin rejected: %#v", accepted) + } + + rejected := service.TestConfigForWorkspace(t.Context(), "workspace-a", &SetConfigRequest{ + Host: "https://gitlab.startup.example", AuthMethod: AuthMethodEnvironment, + }) + if rejected.OK || !strings.Contains(rejected.Error, "environment credential host") { + t.Fatalf("startup host unexpectedly accepted: %#v", rejected) + } +}