From 4aa9d31d9f7e457e9cd687debb48f468a3b07466 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 17:46:38 -0400 Subject: [PATCH] fix(ci): widen node workspace detection, refuse a silent pass (G-015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit nself-ci wired into nself-org/plugins ran only secrets:gitleaks and still reported "Overall: PASSED", because the node stack detector required a formal pnpm-workspace.yaml or package.json "workspaces" field to find member packages. plugins has 5 nested package.json files and neither declaration, so the gate verified zero lines of code while looking green. Detection: add a bounded fallback walk (discoverNestedMembers) that finds nested package.json files when no workspace is declared, skipping node_modules/dist/build/.git/etc. Declared workspaces (pnpm-workspace.yaml or package.json "workspaces") still take priority and run via `pnpm -r`; implicit members run individually since pnpm can't recurse into packages it doesn't know are related. Also fixes package.json "workspaces" support, which was previously dead code (loadPackageJSON only ever returns the "scripts" sub-object, so the old type assertion could never succeed). Silence: GateResult gains Substantive and Skipped fields. Run() now refuses to pass when every executed gate is non-substantive (secrets scan, gateway routing, eval) even though the old "zero gates" guard doesn't fire — this is the exact G-015 shape (gitleaks ran, node contributed nothing). Every script the node gate doesn't find gets an explicit Skipped gate naming the reason, so a run is never silently missing a check. Split gate_runners.go and gate_helpers.go to stay under the 300-line cap: node-specific runners move to gate_runners_node.go, workspace detection to gate_workspace.go. --- free/ci/cmd/main.go | 9 +- free/ci/internal/gate.go | 14 ++ free/ci/internal/gate_helpers.go | 88 ++++------- free/ci/internal/gate_helpers_test.go | 163 +++++++++++++++++++++ free/ci/internal/gate_run.go | 36 +++++ free/ci/internal/gate_run_test.go | 203 ++++++++++++++++++++++++++ free/ci/internal/gate_runners.go | 81 +++------- free/ci/internal/gate_runners_node.go | 111 ++++++++++++++ free/ci/internal/gate_workspace.go | 174 ++++++++++++++++++++++ 9 files changed, 762 insertions(+), 117 deletions(-) create mode 100644 free/ci/internal/gate_helpers_test.go create mode 100644 free/ci/internal/gate_run_test.go create mode 100644 free/ci/internal/gate_runners_node.go create mode 100644 free/ci/internal/gate_workspace.go diff --git a/free/ci/cmd/main.go b/free/ci/cmd/main.go index aefce08b..b4a4e223 100644 --- a/free/ci/cmd/main.go +++ b/free/ci/cmd/main.go @@ -363,11 +363,16 @@ func printResults(r *internal.Result) { for _, g := range r.Gates { mark := "PASS" - if !g.Passed { + switch { + case g.Skipped: + mark = "SKIP" + case !g.Passed: mark = "FAIL" } fmt.Printf(" %-30s %s (%s)\n", g.Name, mark, g.Elapsed.Round(time.Millisecond)) - if !g.Passed && g.Output != "" { + // Always show WHY for a skip or a failure — a silent SKIP/PASS line + // is exactly the shape of the G-015 bug this gate now refuses to be. + if (g.Skipped || !g.Passed) && g.Output != "" { for _, line := range strings.SplitAfter(g.Output, "\n") { fmt.Print(" ", line) } diff --git a/free/ci/internal/gate.go b/free/ci/internal/gate.go index c53a064e..5a674e61 100644 --- a/free/ci/internal/gate.go +++ b/free/ci/internal/gate.go @@ -53,6 +53,20 @@ type GateResult struct { Passed bool Output string Elapsed time.Duration + // Substantive is true when this gate actually verified repo code — a + // lint, typecheck, test, build, or static-analysis step that ran for + // real. Meta checks (secrets scan, gateway routing, eval) and steps that + // never executed (missing binary/script — see Skipped) are false, so + // Run() can tell "every real check was skipped" from "everything was + // verified" even though both cases have Passed=true. G-015 / SPORT: + // PLUGINS-CI-007 + Substantive bool + // Skipped is true when this entry represents a check that did NOT + // execute (missing script, missing binary, absent env var). Output + // always carries the reason. A skipped gate never fails the run by + // itself and never counts toward Substantive coverage. G-015 / SPORT: + // PLUGINS-CI-007 + Skipped bool } // Result is the overall gate run result. diff --git a/free/ci/internal/gate_helpers.go b/free/ci/internal/gate_helpers.go index 36507d69..5a1d430d 100644 --- a/free/ci/internal/gate_helpers.go +++ b/free/ci/internal/gate_helpers.go @@ -87,6 +87,38 @@ func extractJSONObject(json, key string) map[string]string { return result } +// extractJSONStringArray extracts a top-level JSON string-array field (e.g. +// "workspaces": ["packages/*", "apps/*"]) using the same simple string +// parsing as extractJSONObject — no external JSON library. Handles only a +// flat array of string literals, which is the shape package.json +// "workspaces" always takes in practice. +func extractJSONStringArray(json, key string) []string { + search := `"` + key + `"` + idx := strings.Index(json, search) + if idx < 0 { + return nil + } + start := strings.Index(json[idx:], "[") + if start < 0 { + return nil + } + start += idx + 1 + end := strings.Index(json[start:], "]") + if end < 0 { + return nil + } + block := json[start : start+end] + + var items []string + for _, part := range strings.Split(block, ",") { + part = strings.Trim(strings.TrimSpace(part), `"'`) + if part != "" { + items = append(items, part) + } + } + return items +} + // isGitRepo reports whether root is inside a git checkout. // // Purpose: Decide whether gitleaks can scan tracked content (respecting @@ -108,62 +140,6 @@ func isGitRepo(root string) bool { } } -// workspaceMembers returns the directories of a pnpm/npm workspace's member -// packages, or nil when root is not a workspace. -// -// Purpose: Let the Node gates see scripts that live in member packages -// rather than the root package.json. -// Inputs: root string — repo root -// Outputs: []string — absolute member directories containing a package.json -// Constraints: Handles pnpm-workspace.yaml globs and package.json "workspaces". -// Skips node_modules. Glob depth is whatever the pattern states. -func workspaceMembers(root string) []string { - var patterns []string - - if data, err := os.ReadFile(filepath.Join(root, "pnpm-workspace.yaml")); err == nil { - for _, line := range strings.Split(string(data), "\n") { - line = strings.TrimSpace(line) - if !strings.HasPrefix(line, "- ") { - continue - } - pat := strings.Trim(strings.TrimSpace(strings.TrimPrefix(line, "- ")), `'"`) - // Members outside the repo (sibling checkouts) are not ours to gate. - if pat != "" && !strings.HasPrefix(pat, "..") { - patterns = append(patterns, pat) - } - } - } - - if len(patterns) == 0 { - if ws, ok := loadPackageJSON(root)["workspaces"].([]interface{}); ok { - for _, w := range ws { - if str, ok := w.(string); ok { - patterns = append(patterns, str) - } - } - } - } - - seen := map[string]bool{} - var members []string - for _, pat := range patterns { - matches, err := filepath.Glob(filepath.Join(root, pat)) - if err != nil { - continue - } - for _, m := range matches { - if strings.Contains(m, "node_modules") || seen[m] { - continue - } - if fileExists(filepath.Join(m, "package.json")) { - seen[m] = true - members = append(members, m) - } - } - } - return members -} - // anyMemberHasScript reports whether at least one workspace member defines the // named script, so a gate is only added when it will actually do something. func anyMemberHasScript(members []string, script string) bool { diff --git a/free/ci/internal/gate_helpers_test.go b/free/ci/internal/gate_helpers_test.go new file mode 100644 index 00000000..e7551277 --- /dev/null +++ b/free/ci/internal/gate_helpers_test.go @@ -0,0 +1,163 @@ +// Regression tests for Node workspace member discovery. +// +// WHY these exist: nself-org/plugins declares neither a pnpm-workspace.yaml +// nor a package.json "workspaces" field, yet has 5 nested package.json files +// with real lint/typecheck/test/build scripts. The old workspaceMembers() +// only understood the two formal declarations, found nothing, and the node +// gate silently ran zero checks while gitleaks alone reported "PASSED" +// (G-015). These tests pin the fallback discovery that fixes it, plus the +// package.json "workspaces" field path, which the old code referenced via +// loadPackageJSON(root)["workspaces"] — a type assertion that could never +// succeed, because loadPackageJSON only ever returns the "scripts" object. +package internal + +import ( + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +func writePackageJSON(t *testing.T, dir, content string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + if err := os.WriteFile(filepath.Join(dir, "package.json"), []byte(content), 0o644); err != nil { + t.Fatalf("write package.json in %s: %v", dir, err) + } +} + +// TestDetectNodeWorkspace_ImplicitDiscoveryMatchesPluginsShape reproduces the +// exact nself-org/plugins layout: a root package.json with only an unrelated +// script ("ci:local"), no pnpm-workspace.yaml, no "workspaces" field, and +// nested package.json files at varying depths (mirrors ".workers/plugins-registry" +// and "free/feature-flags/sdk-ts"). Before the fix this produced workspaceKind +// == workspaceNone and zero members; it must now find both. +func TestDetectNodeWorkspace_ImplicitDiscoveryMatchesPluginsShape(t *testing.T) { + root := t.TempDir() + writePackageJSON(t, root, `{"version":"1.0.0","scripts":{"ci:local":"echo ok"}}`) + writePackageJSON(t, filepath.Join(root, ".workers", "plugins-registry"), + `{"name":"registry","scripts":{"typecheck":"tsc --noEmit","test":"node --test"}}`) + writePackageJSON(t, filepath.Join(root, "free", "feature-flags", "sdk-ts"), + `{"name":"sdk-ts","scripts":{"build":"tsc","test":"jest"}}`) + + ws := detectNodeWorkspace(root) + + if ws.kind != workspaceImplicit { + t.Fatalf("expected workspaceImplicit (no formal declaration, nested package.json present), got kind=%d members=%v", ws.kind, ws.members) + } + if len(ws.members) != 2 { + t.Fatalf("expected 2 implicit members, got %d: %v", len(ws.members), ws.members) + } + wantA := filepath.Join(root, ".workers", "plugins-registry") + wantB := filepath.Join(root, "free", "feature-flags", "sdk-ts") + if !slices.Contains(ws.members, wantA) { + t.Errorf("expected member %s, got %v", wantA, ws.members) + } + if !slices.Contains(ws.members, wantB) { + t.Errorf("expected member %s, got %v", wantB, ws.members) + } +} + +// TestDiscoverNestedMembers_SkipsDependencyAndBuildDirs asserts the fallback +// walk never descends into node_modules/dist/.git — a repo where every real +// package.json lives inside node_modules (typical after `pnpm install`) must +// not report thousands of vendored packages as workspace members. +func TestDiscoverNestedMembers_SkipsDependencyAndBuildDirs(t *testing.T) { + root := t.TempDir() + writePackageJSON(t, filepath.Join(root, "node_modules", "some-dep"), `{"name":"some-dep"}`) + writePackageJSON(t, filepath.Join(root, "dist"), `{"name":"build-output"}`) + writePackageJSON(t, filepath.Join(root, "real-app"), `{"name":"real-app","scripts":{"test":"true"}}`) + + members := discoverNestedMembers(root, implicitMemberMaxDepth) + + if len(members) != 1 { + t.Fatalf("expected exactly 1 real member (node_modules/dist must be skipped), got %d: %v", len(members), members) + } + if members[0] != filepath.Join(root, "real-app") { + t.Errorf("expected real-app, got %v", members) + } +} + +// TestDiscoverNestedMembers_RespectsMaxDepth ensures the walk does not surface +// a package.json buried deeper than the configured bound, keeping the +// fallback a scoped discovery rather than a full-repo crawl. +func TestDiscoverNestedMembers_RespectsMaxDepth(t *testing.T) { + root := t.TempDir() + deep := filepath.Join(root, "a", "b", "c", "d", "e", "f") + writePackageJSON(t, deep, `{"name":"too-deep"}`) + + members := discoverNestedMembers(root, 2) + + if len(members) != 0 { + t.Fatalf("expected 0 members beyond maxDepth, got %v", members) + } +} + +// TestDetectNodeWorkspace_DeclaredPnpmWorkspaceWins asserts a real +// pnpm-workspace.yaml still takes priority over the implicit walk and is +// resolved via its own glob semantics, unchanged from before this fix. +func TestDetectNodeWorkspace_DeclaredPnpmWorkspaceWins(t *testing.T) { + root := t.TempDir() + writePackageJSON(t, root, `{"name":"root"}`) + if err := os.WriteFile(filepath.Join(root, "pnpm-workspace.yaml"), []byte("packages:\n - 'packages/*'\n"), 0o644); err != nil { + t.Fatalf("write pnpm-workspace.yaml: %v", err) + } + writePackageJSON(t, filepath.Join(root, "packages", "a"), `{"name":"a","scripts":{"test":"true"}}`) + + ws := detectNodeWorkspace(root) + + if ws.kind != workspaceDeclared { + t.Fatalf("expected workspaceDeclared, got kind=%d", ws.kind) + } + if len(ws.members) != 1 || ws.members[0] != filepath.Join(root, "packages", "a") { + t.Fatalf("expected [packages/a], got %v", ws.members) + } +} + +// TestDeclaredWorkspacePatterns_PackageJSONWorkspacesField pins the fix for a +// previously-dead code path: the old workspaceMembers() read +// loadPackageJSON(root)["workspaces"], but loadPackageJSON only ever returns +// the "scripts" sub-object, so that type assertion could never succeed — +// package.json "workspaces" was silently never honoured. This must now work. +func TestDeclaredWorkspacePatterns_PackageJSONWorkspacesField(t *testing.T) { + root := t.TempDir() + writePackageJSON(t, root, `{"name":"root","workspaces":["apps/*","libs/*"]}`) + + patterns := declaredWorkspacePatterns(root) + + if !slices.Contains(patterns, "apps/*") || !slices.Contains(patterns, "libs/*") { + t.Fatalf("expected [\"apps/*\" \"libs/*\"], got %v", patterns) + } +} + +// TestDetectNodeWorkspace_NoPackageAnywhereIsNone asserts a plain single +// package (no nested package.json, no declaration) still resolves to +// workspaceNone rather than spuriously discovering itself. +func TestDetectNodeWorkspace_NoPackageAnywhereIsNone(t *testing.T) { + root := t.TempDir() + writePackageJSON(t, root, `{"name":"solo","scripts":{"test":"true"}}`) + + ws := detectNodeWorkspace(root) + + if ws.kind != workspaceNone { + t.Fatalf("expected workspaceNone for a repo with no nested members, got kind=%d members=%v", ws.kind, ws.members) + } +} + +// TestSkipReasonForScript_NamesTheScriptAndMemberCount asserts a skip reason +// is never a bare, unexplained line — it must name the script and say +// whether a workspace was even considered. +func TestSkipReasonForScript_NamesTheScriptAndMemberCount(t *testing.T) { + none := skipReasonForScript("lint", nodeWorkspace{kind: workspaceNone}) + if !strings.Contains(none, `"lint"`) || !strings.Contains(none, "no workspace members found") { + t.Errorf("workspaceNone reason missing detail: %q", none) + } + + withMembers := skipReasonForScript("build", nodeWorkspace{kind: workspaceImplicit, members: []string{"a", "b", "c"}}) + if !strings.Contains(withMembers, `"build"`) || !strings.Contains(withMembers, "3 workspace member") { + t.Errorf("workspaceImplicit reason missing member count: %q", withMembers) + } +} diff --git a/free/ci/internal/gate_run.go b/free/ci/internal/gate_run.go index b03d3d8c..21ce4c0f 100644 --- a/free/ci/internal/gate_run.go +++ b/free/ci/internal/gate_run.go @@ -112,6 +112,42 @@ func Run(cfg Config) (*Result, error) { return res, nil } + // 8. Reject a run where every gate that DID execute was non-substantive. + // + // G-015: (1) above only caught the case of literally zero gates. Wiring + // this gate into nself-org/plugins showed a second, worse shape of the + // same bug — gitleaks ran (1 gate, len(res.Gates) != 0) while the + // detected "node" stack contributed nothing, because its scripts live in + // nested member packages the old detector never looked at. The result + // printed "Overall: PASSED" having verified zero lines of code. A gate + // meant to be a required merge check must never look identical whether + // it checked everything or checked nothing, so this counts only gates + // that actually ran a lint/test/build/analyze step (Substantive && + // !Skipped) and refuses to pass when that count is zero, regardless of + // how many meta/skipped entries are present. + substantive := 0 + for _, g := range res.Gates { + if g.Substantive && !g.Skipped { + substantive++ + } + } + if substantive == 0 { + res.Passed = false + res.Gates = append(res.Gates, GateResult{ + Name: "gate:no-substantive-checks", + Passed: false, + Output: fmt.Sprintf( + "Detected stack(s) %s produced zero lint/test/build/analyze checks — "+ + "only non-code checks ran (e.g. secrets scan). See the SKIP entries "+ + "above for which checks were skipped and why.\n"+ + "Passing this would report \"Overall: PASSED\" without verifying any code.", + strings.Join(res.Stack, "+"), + ), + }) + res.Elapsed = time.Since(start) + return res, nil + } + res.Passed = true for _, g := range res.Gates { if !g.Passed { diff --git a/free/ci/internal/gate_run_test.go b/free/ci/internal/gate_run_test.go new file mode 100644 index 00000000..6eb4d3a3 --- /dev/null +++ b/free/ci/internal/gate_run_test.go @@ -0,0 +1,203 @@ +// Regression tests for Run()'s pass/fail aggregation — specifically the +// near-empty-run case at the heart of G-015. +// +// WHY these exist: wiring this gate into nself-org/plugins produced exactly +// this real output: +// +// Stacks: node +// secrets:gitleaks PASS (2-4s) +// Overall: PASSED +// +// One gate ran (gitleaks), so the pre-existing "zero gates" guard below never +// fired, and a required merge-gate status check reported PASSED having +// verified zero lines of code. These tests pin the fix: Run() must count only +// gates that actually verified something (Substantive && !Skipped) and must +// refuse to report an unqualified pass when that count is zero, however many +// non-substantive gates ran alongside it. +package internal + +import ( + "os" + "os/exec" + "strings" + "testing" +) + +// TestRun_GitleaksOnlyIsNotASilentPass reproduces the exact G-015 shape: a +// node stack whose only script is unrelated to lint/typecheck/test/build (no +// workspace members either), with gitleaks left enabled. Before this fix, +// Run() would report Passed=true off the strength of the secrets scan alone. +func TestRun_GitleaksOnlyIsNotASilentPass(t *testing.T) { + if _, err := exec.LookPath("gitleaks"); err != nil { + t.Skip("skip: gitleaks binary not found on PATH") + } + if _, err := exec.LookPath("git"); err != nil { + t.Skip("skip: git binary not found on PATH") + } + + root := t.TempDir() + // Mirrors nself-org/plugins' root package.json: a real script, but not + // one of the four the node gate checks for, and no nested member + // packages at all. + writePackageJSON(t, root, `{"version":"1.1.7","scripts":{"ci:local":"echo ok"}}`) + + runGit := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = root + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=nself-ci-test", "GIT_AUTHOR_EMAIL=ci-test@nself.org", + "GIT_COMMITTER_NAME=nself-ci-test", "GIT_COMMITTER_EMAIL=ci-test@nself.org", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + runGit("init", "-q", "-b", "main") + runGit("add", "package.json") + runGit("commit", "-q", "-m", "initial commit") + + result, err := Run(Config{RepoRoot: root, StepTimeout: 30}) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + + if result.Passed { + t.Fatalf("G-015 regression: a run where only gitleaks executed must not pass, got:\n%s", dumpGates(result)) + } + + foundGitleaks, gitleaksPassed := false, false + for _, g := range result.Gates { + if g.Name == "secrets:gitleaks" { + foundGitleaks, gitleaksPassed = true, g.Passed + } + } + if !foundGitleaks { + t.Fatalf("expected a secrets:gitleaks gate to have run, got:\n%s", dumpGates(result)) + } + if !gitleaksPassed { + t.Fatalf("expected the clean fixture repo to pass gitleaks (so the FAILED overall comes from the substantive-check guard, not a secrets false-positive):\n%s", dumpGates(result)) + } + + foundGuard := false + for _, g := range result.Gates { + if g.Name == "gate:no-substantive-checks" { + foundGuard = true + if g.Passed { + t.Errorf("gate:no-substantive-checks must itself be Passed=false") + } + } + } + if !foundGuard { + t.Fatalf("expected a gate:no-substantive-checks entry explaining the failure, got:\n%s", dumpGates(result)) + } +} + +// TestRun_RealScriptCountsAsSubstantive is the positive-path counterpart: once +// a real check runs (here, a trivial passing "test" script), the substantive +// guard must not block a genuine pass. +func TestRun_RealScriptCountsAsSubstantive(t *testing.T) { + pm := "pnpm" + if _, err := exec.LookPath(pm); err != nil { + pm = "npm" + if _, err := exec.LookPath(pm); err != nil { + t.Skip("skip: neither pnpm nor npm found on PATH") + } + } + + root := t.TempDir() + writePackageJSON(t, root, `{"name":"solo","version":"1.0.0","scripts":{"test":"node -e \"process.exit(0)\""}}`) + + result, err := Run(Config{RepoRoot: root, SkipGitleaks: true, StepTimeout: 30}) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + + if !result.Passed { + t.Fatalf("expected a genuine passing script to pass the gate, got:\n%s", dumpGates(result)) + } + + substantiveRan := false + for _, g := range result.Gates { + if g.Name == "node:test" && g.Substantive && !g.Skipped { + substantiveRan = true + } + } + if !substantiveRan { + t.Fatalf("expected node:test to be marked Substantive and not Skipped, got:\n%s", dumpGates(result)) + } +} + +// TestRun_NodeStackWithNoScriptsAnywhereFailsExplicitly covers a bare +// package.json with no scripts, no workspace, and no tsconfig.json: every one +// of the 4 checked scripts (lint/typecheck/test/build) resolves to an +// explicit Skipped gate with its own reason (never a bare silent gap), and +// the run as a whole fails via gate:no-substantive-checks rather than +// reporting PASSED. +// +// This also documents that the pre-existing "zero gates" branch in Run() is +// no longer reachable through the node runner specifically: runNodeGates now +// always emits a Skipped entry per unmatched script instead of an empty +// slice, which is the whole point — a silent gap must announce itself. The +// zero-gates branch still exists as defense-in-depth for a future stack with +// no runner wired in the switch. +func TestRun_NodeStackWithNoScriptsAnywhereFailsExplicitly(t *testing.T) { + root := t.TempDir() + writePackageJSON(t, root, `{"name":"empty","version":"1.0.0"}`) + + result, err := Run(Config{RepoRoot: root, SkipGitleaks: true, StepTimeout: 5}) + if err != nil { + t.Fatalf("Run() error: %v", err) + } + if result.Passed { + t.Fatalf("a repo with no substantive checks anywhere must not pass, got:\n%s", dumpGates(result)) + } + + wantSkipped := map[string]bool{"node:lint": false, "node:typecheck": false, "node:test": false, "node:build": false} + foundGuard := false + for _, g := range result.Gates { + if _, ok := wantSkipped[g.Name]; ok { + if !g.Skipped || g.Output == "" { + t.Errorf("%s: expected Skipped=true with a non-empty reason, got skipped=%v output=%q", g.Name, g.Skipped, g.Output) + } + wantSkipped[g.Name] = true + } + if g.Name == "gate:no-substantive-checks" { + foundGuard = true + } + } + for name, seen := range wantSkipped { + if !seen { + t.Errorf("expected a %s gate entry, got:\n%s", name, dumpGates(result)) + } + } + if !foundGuard { + t.Fatalf("expected gate:no-substantive-checks, got:\n%s", dumpGates(result)) + } +} + +func dumpGates(r *Result) string { + var b strings.Builder + for _, g := range r.Gates { + b.WriteString(" " + g.Name + " passed=") + if g.Passed { + b.WriteString("true") + } else { + b.WriteString("false") + } + b.WriteString(" substantive=") + if g.Substantive { + b.WriteString("true") + } else { + b.WriteString("false") + } + b.WriteString(" skipped=") + if g.Skipped { + b.WriteString("true") + } else { + b.WriteString("false") + } + b.WriteString("\n") + } + return b.String() +} diff --git a/free/ci/internal/gate_runners.go b/free/ci/internal/gate_runners.go index 5ac3ad04..5a4dfc5f 100644 --- a/free/ci/internal/gate_runners.go +++ b/free/ci/internal/gate_runners.go @@ -57,78 +57,38 @@ func gitleaksArgs(root, configFlag string, isRepo bool) []string { return args } +// markSubstantive flags each gate as substantive (a real lint/test/build/ +// analyze check) unless runStep already marked it Skipped — e.g. the tool +// binary was missing. A step that never executed a command must not count +// toward "something was verified" just because its caller intended it to. +// G-015 / SPORT: PLUGINS-CI-007 +func markSubstantive(gates []GateResult) { + for i := range gates { + if !gates[i].Skipped { + gates[i].Substantive = true + } + } +} + // runGoGates runs gofmt, go vet, and go test for a Go repo. func runGoGates(root string, timeout int, verbose bool) []GateResult { - return []GateResult{ + gates := []GateResult{ runStep("go:fmt", root, timeout, verbose, "gofmt", "-l", "."), runStep("go:vet", root, timeout, verbose, "go", "vet", "./..."), runStep("go:test", root, timeout, verbose, "go", "test", "-count=1", "-timeout", fmt.Sprintf("%ds", timeout), "./..."), } -} - -// runNodeGates runs pnpm lint, pnpm test, and pnpm build for a Node repo. -// Falls back to npm if pnpm is not present. -func runNodeGates(root string, timeout int, verbose bool) []GateResult { - pm := "pnpm" - if _, err := exec.LookPath("pnpm"); err != nil { - pm = "npm" - } - - pkg := loadPackageJSON(root) - - // A pnpm/npm workspace keeps its real scripts in member packages, not the - // root package.json. Reading only the root made whole repos run ZERO gates - // and still report PASSED. Recurse when the root has no script of its own - // but a member does. - if members := workspaceMembers(root); len(members) > 0 { - var gates []GateResult - for _, script := range []string{"lint", "typecheck", "test", "build"} { - if hasScript(pkg, script) { - gates = append(gates, runStep("node:"+script, root, timeout, verbose, pm, "run", script)) - continue - } - if anyMemberHasScript(members, script) { - // --if-present so members without the script are skipped rather - // than failing the whole recursive run. - gates = append(gates, runStep("node:"+script+" (workspace)", root, timeout, verbose, - pm, "-r", "--if-present", "run", script)) - } - } - if len(gates) > 0 { - return gates - } - } - - var gates []GateResult - if hasScript(pkg, "lint") { - gates = append(gates, runStep("node:lint", root, timeout, verbose, pm, "run", "lint")) - } - if hasScript(pkg, "typecheck") { - gates = append(gates, runStep("node:typecheck", root, timeout, verbose, pm, "run", "typecheck")) - } - if hasScript(pkg, "test") { - gates = append(gates, runStep("node:test", root, timeout, verbose, pm, "run", "test")) - } - if hasScript(pkg, "build") { - gates = append(gates, runStep("node:build", root, timeout, verbose, pm, "run", "build")) - } - - if len(gates) == 0 { - // No scripts found; at minimum run tsc if tsconfig.json exists. - if fileExists(filepath.Join(root, "tsconfig.json")) { - gates = append(gates, runStep("node:tsc", root, timeout, verbose, pm, "exec", "tsc", "--noEmit")) - } - } - + markSubstantive(gates) return gates } // runFlutterGates runs flutter analyze and flutter test. func runFlutterGates(root string, timeout int, verbose bool) []GateResult { - return []GateResult{ + gates := []GateResult{ runStep("flutter:analyze", root, timeout, verbose, "flutter", "analyze"), runStep("flutter:test", root, timeout, verbose, "flutter", "test", "--reporter", "compact"), } + markSubstantive(gates) + return gates } // runRustGates runs cargo clippy (deny warnings) and cargo test for a Rust crate. @@ -138,12 +98,14 @@ func runFlutterGates(root string, timeout int, verbose bool) []GateResult { // Outputs: []GateResult — clippy lint + unit test results // Constraints: clippy --deny warnings; cargo test --all-features; SPORT PLUGINS-CI-004 func runRustGates(root string, timeout int, verbose bool) []GateResult { - return []GateResult{ + gates := []GateResult{ runStep("rust:clippy", root, timeout, verbose, "cargo", "clippy", "--all-targets", "--all-features", "--", "--deny", "warnings"), runStep("rust:test", root, timeout, verbose, "cargo", "test", "--all-features"), } + markSubstantive(gates) + return gates } // runGatewayRoutingCheck verifies that the nself-ai-gateway on staging responds @@ -205,6 +167,7 @@ func runStep(name, root string, timeout int, verbose bool, cmd string, args ...s if _, err := exec.LookPath(cmd); err != nil { gr.Output = fmt.Sprintf("command not found: %s (skipped)", cmd) gr.Passed = true // Skip missing optional tools gracefully. + gr.Skipped = true gr.Elapsed = time.Since(start) return gr } diff --git a/free/ci/internal/gate_runners_node.go b/free/ci/internal/gate_runners_node.go new file mode 100644 index 00000000..8e58e84b --- /dev/null +++ b/free/ci/internal/gate_runners_node.go @@ -0,0 +1,111 @@ +package internal + +// Package internal — gate_runners_node.go +// +// Purpose: Run lint/typecheck/test/build for a Node repo, across the root +// package and any workspace members (declared or implicitly discovered — +// see gate_workspace.go), and explain in the output why any of those four +// checks never ran anywhere. +// Inputs: root string, timeout int, verbose bool +// Outputs: []GateResult — one per script that ran, plus one Skipped entry +// per script that ran nowhere, each carrying the reason in Output. +// Constraints: falls back to npm if pnpm is not on PATH. G-015 — +// nself-org/plugins has 5 nested package.json files and NEITHER a +// pnpm-workspace.yaml NOR a "workspaces" field, so the prior member +// detection found nothing and the gate ran only gitleaks while still +// reporting "Overall: PASSED". +// SPORT: PLUGINS-CI-007 + +import ( + "fmt" + "os/exec" + "path/filepath" +) + +// runNodeGates runs lint/typecheck/test/build for a Node repo. +func runNodeGates(root string, timeout int, verbose bool) []GateResult { + pm := "pnpm" + if _, err := exec.LookPath("pnpm"); err != nil { + pm = "npm" + } + + pkg := loadPackageJSON(root) + ws := detectNodeWorkspace(root) + + var gates []GateResult + ranAny := false + for _, script := range []string{"lint", "typecheck", "test", "build"} { + scriptGates := runNodeScript(pm, root, script, pkg, ws, timeout, verbose) + for _, g := range scriptGates { + if !g.Skipped { + ranAny = true + } + } + gates = append(gates, scriptGates...) + } + + // Nothing declared any of the four scripts anywhere, and there is no + // workspace to recurse into — last resort, run tsc directly if the repo + // at least declares a tsconfig.json. + if !ranAny && ws.kind == workspaceNone && fileExists(filepath.Join(root, "tsconfig.json")) { + gates = append(gates, runStep("node:tsc", root, timeout, verbose, pm, "exec", "tsc", "--noEmit")) + } + + markSubstantive(gates) + return gates +} + +// runNodeScript runs one script name (lint/typecheck/test/build) against the +// root package and, per ws.kind, its workspace members — returning a Skipped +// gate explaining why when the script exists nowhere. +func runNodeScript(pm, root, script string, pkg map[string]interface{}, ws nodeWorkspace, timeout int, verbose bool) []GateResult { + var gates []GateResult + + if hasScript(pkg, script) { + gates = append(gates, runStep("node:"+script, root, timeout, verbose, pm, "run", script)) + } + + switch ws.kind { + case workspaceDeclared: + // A real pnpm/npm workspace understands `-r`/`--workspaces` recursion. + // --if-present so members without the script are skipped rather than + // failing the whole recursive run. + if anyMemberHasScript(ws.members, script) { + gates = append(gates, runStep("node:"+script+" (workspace)", root, timeout, verbose, + pm, "-r", "--if-present", "run", script)) + } + case workspaceImplicit: + // No formal workspace declaration exists, so `pnpm -r` would not + // recurse into these directories at all — run each member on its own. + for _, member := range ws.members { + if !hasScript(loadPackageJSON(member), script) { + continue + } + rel, err := filepath.Rel(root, member) + if err != nil { + rel = member + } + gates = append(gates, runStep(fmt.Sprintf("node:%s (%s)", script, rel), member, timeout, verbose, pm, "run", script)) + } + } + + if len(gates) == 0 { + gates = append(gates, GateResult{ + Name: "node:" + script, + Passed: true, + Skipped: true, + Output: skipReasonForScript(script, ws), + }) + } + + return gates +} + +// skipReasonForScript explains why a given script never ran anywhere, so a +// SKIP entry is never a bare, unexplained line. G-015 / SPORT: PLUGINS-CI-007 +func skipReasonForScript(script string, ws nodeWorkspace) string { + if ws.kind == workspaceNone { + return fmt.Sprintf("skipped: no %q script in package.json and no workspace members found", script) + } + return fmt.Sprintf("skipped: no %q script in package.json or any of %d workspace member(s)", script, len(ws.members)) +} diff --git a/free/ci/internal/gate_workspace.go b/free/ci/internal/gate_workspace.go new file mode 100644 index 00000000..c4b5ad5c --- /dev/null +++ b/free/ci/internal/gate_workspace.go @@ -0,0 +1,174 @@ +package internal + +// Package internal — gate_workspace.go +// +// Purpose: Resolve a Node repo's workspace member packages so the node gate +// can see scripts that live in member packages instead of only the root +// package.json — declared (pnpm-workspace.yaml / package.json +// "workspaces") or, failing that, merely nested with no formal declaration +// at all. +// Inputs: root string — repo root +// Outputs: nodeWorkspace{kind, members} +// Constraints: A declared workspace always wins (respects the author's exact +// glob intent, including exclusions); the implicit walk is a fallback +// only, bounded to implicitMemberMaxDepth and skipping dependency/build/VCS +// directories, so it cannot balloon into a full-repo crawl. +// SPORT: PLUGINS-CI-007 (G-015) +// +// WHY this exists as a fallback at all: nself-org/plugins has 5 nested +// package.json files (e.g. ".workers/plugins-registry", +// "free/feature-flags/sdk-ts") and declares NEITHER a pnpm-workspace.yaml NOR +// a "workspaces" field. The prior workspaceMembers() understood only those +// two formal declarations, found nothing, and the node gate ran zero checks +// while gitleaks alone reported "Overall: PASSED" — the shape of bug G-015 +// closes. + +import ( + "os" + "path/filepath" + "strings" +) + +// workspaceKind distinguishes a formally declared pnpm/npm workspace — which +// supports `pnpm -r` / `npm --workspaces` recursion — from implicitly +// discovered member packages, which must be run one directory at a time +// since no workspace manifest tells the package manager they are related. +type workspaceKind int + +const ( + // workspaceNone: no pnpm-workspace.yaml, no "workspaces" field, and the + // fallback walk found no nested package.json either. + workspaceNone workspaceKind = iota + // workspaceDeclared: pnpm-workspace.yaml or package.json "workspaces" + // names the members explicitly. + workspaceDeclared + // workspaceImplicit: nothing declared a workspace, but nested + // package.json files were found by walking the tree. + workspaceImplicit +) + +// nodeWorkspace is the result of resolving a Node repo's workspace members. +type nodeWorkspace struct { + kind workspaceKind + members []string +} + +// implicitMemberMaxDepth bounds the fallback walk in discoverNestedMembers. +// 4 covers every case seen in nself-org repos (e.g. plugins' +// free//ts/package.json sits 3 directories below the repo root) +// without turning into an unbounded crawl of an entire monorepo. +const implicitMemberMaxDepth = 4 + +// skipNestedDirs are directories never worth descending into while +// discovering implicit workspace members: dependency trees, build output, +// and VCS metadata. Deliberately does NOT skip all dot-directories — plugins' +// own ".workers/plugins-registry" is a real member with real scripts. +var skipNestedDirs = map[string]bool{ + "node_modules": true, ".git": true, "dist": true, "build": true, + "out": true, "coverage": true, ".next": true, ".turbo": true, + ".cache": true, "vendor": true, "target": true, ".venv": true, +} + +// detectNodeWorkspace resolves a Node repo's workspace members. Declared +// patterns (pnpm-workspace.yaml or package.json "workspaces") always take +// priority over the implicit fallback walk. +func detectNodeWorkspace(root string) nodeWorkspace { + if patterns := declaredWorkspacePatterns(root); len(patterns) > 0 { + return nodeWorkspace{kind: workspaceDeclared, members: globMembers(root, patterns)} + } + if members := discoverNestedMembers(root, implicitMemberMaxDepth); len(members) > 0 { + return nodeWorkspace{kind: workspaceImplicit, members: members} + } + return nodeWorkspace{kind: workspaceNone} +} + +// declaredWorkspacePatterns reads pnpm-workspace.yaml globs, or failing that, +// package.json's "workspaces" array. Members outside the repo (sibling +// checkouts referenced via "..") are not ours to gate. +// +// The package.json path was previously dead code: workspaceMembers() read +// loadPackageJSON(root)["workspaces"], but loadPackageJSON only ever returns +// the "scripts" sub-object, so that type assertion could never succeed — a +// "workspaces" field was silently never honoured. This reads the raw file. +func declaredWorkspacePatterns(root string) []string { + if data, err := os.ReadFile(filepath.Join(root, "pnpm-workspace.yaml")); err == nil { + var patterns []string + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "- ") { + continue + } + pat := strings.Trim(strings.TrimSpace(strings.TrimPrefix(line, "- ")), `'"`) + if pat != "" && !strings.HasPrefix(pat, "..") { + patterns = append(patterns, pat) + } + } + return patterns + } + + data, err := os.ReadFile(filepath.Join(root, "package.json")) + if err != nil { + return nil + } + var patterns []string + for _, pat := range extractJSONStringArray(string(data), "workspaces") { + if !strings.HasPrefix(pat, "..") { + patterns = append(patterns, pat) + } + } + return patterns +} + +// globMembers expands declared workspace glob patterns to absolute member +// directories that actually contain a package.json. +func globMembers(root string, patterns []string) []string { + seen := map[string]bool{} + var members []string + for _, pat := range patterns { + matches, err := filepath.Glob(filepath.Join(root, pat)) + if err != nil { + continue + } + for _, m := range matches { + if strings.Contains(m, "node_modules") || seen[m] { + continue + } + if fileExists(filepath.Join(m, "package.json")) { + seen[m] = true + members = append(members, m) + } + } + } + return members +} + +// discoverNestedMembers walks root — bounded to maxDepth, skipping +// dependency/build/VCS directories — collecting every directory that holds +// its own package.json. Used only as a fallback when no pnpm-workspace.yaml +// or "workspaces" field declares members explicitly; see detectNodeWorkspace. +func discoverNestedMembers(root string, maxDepth int) []string { + var members []string + + var walk func(dir string, depth int) + walk = func(dir string, depth int) { + if depth > maxDepth { + return + } + entries, err := os.ReadDir(dir) + if err != nil { + return + } + for _, e := range entries { + if !e.IsDir() || skipNestedDirs[e.Name()] { + continue + } + sub := filepath.Join(dir, e.Name()) + if fileExists(filepath.Join(sub, "package.json")) { + members = append(members, sub) + } + walk(sub, depth+1) + } + } + walk(root, 1) + return members +}