diff --git a/README.md b/README.md index a8876bd..80480d7 100644 --- a/README.md +++ b/README.md @@ -32,10 +32,38 @@ If you run it against the top of the consolidated BlockNinja repo, it prints a h | `--orchestrator` | Scan the orchestrator backend only (resolved from the hardcoded consolidated layout), regardless of the positional target. | | `--plugin-dir [more…]` | Register one or more plugin roots (dirs with `plugin.mod`). Alias: `--plugin-dirs`. Consumes args until the next `--flag`. | | `--plugin-pages [more…]` | Register frontend source dirs directly as plugin page targets (for the frontend checks). Consumes args until the next `--flag`. | +| `--verbose` / `-v` | Print every check (including `OK`/`SKIP`), not just failures and warnings. | +| `--all-any` | The `any`-usage check (2e) defaults to only the **unstaged working-tree diff** (newly introduced `any`); `--all-any` scans every source file instead. | The CLI contract (`check-safety [--flags]`) is stable — the CMS `Makefile` `safety-check` / `install-safety-checker` targets shell into this directory and depend on it. +### Output + +Output is concise by design (it is usually fed back into an agent's context). Passing and +skipped checks print nothing; only `FAIL`/`WARN`/`ERR` checks print a ` ` +line with their findings indented beneath, followed by one tally line. A clean run is two lines: + +``` +check-safety /home/alex/src/blockninja/cms +32 checks: 20 ok 12 skip -> OK +``` + +A run with problems: + +``` +check-safety /home/alex/src/blockninja/cms +FAIL 15 1 err.Error() leak(s) to HTTP clients — log via slog.Error() and return a user-friendly message + internal/service/handler.go:27 http.Error(w, err.Error(), ...) — leaks internal error +WARN 2e 1 any usage in changed lines (showing 1, --all-any to scan all) + web/src/api.ts:12 [go] data: any + +32 checks: 27 ok 3 skip 1 warn 1 fail -> FAIL +``` + +Pass `--verbose` to see every check's status. Exit code is `1` on any `FAIL`, `2` on a hard +`ERR`, `0` otherwise (warnings do not fail the run). + ## Install globally ```bash diff --git a/any_usage.go b/any_usage.go index 2171a92..2248cfc 100644 --- a/any_usage.go +++ b/any_usage.go @@ -17,6 +17,15 @@ type anyUsageWarning struct { snippet string } +// lineKeeper decides whether an `any` at absPath:line should be reported. A nil +// keeper keeps everything (the --all-any full scan); the diff-scoped default +// keeps only lines present in the unstaged working-tree diff. +type lineKeeper func(absPath string, line int) bool + +func (k lineKeeper) keep(absPath string, line int) bool { + return k == nil || k(absPath, line) +} + var ( reTypeScriptAny = regexp.MustCompile(`\bany\b`) reTypeScriptAsAny = regexp.MustCompile(`\bas\s+any\b`) @@ -26,7 +35,7 @@ var ( reTypeScriptEqAny = regexp.MustCompile(`=\s*any(?:\b|[\s;])`) ) -func checkGoAnyUsage(root string) []anyUsageWarning { +func checkGoAnyUsage(root string, keep lineKeeper) []anyUsageWarning { var warnings []anyUsageWarning _ = filepath.Walk(root, func(path string, info os.FileInfo, err error) error { @@ -61,6 +70,9 @@ func checkGoAnyUsage(root string) []anyUsageWarning { continue } seen[position.Line] = true + if !keep.keep(path, position.Line) { + continue + } snippet := "" if position.Line-1 >= 0 && position.Line-1 < len(lines) { snippet = strings.TrimSpace(lines[position.Line-1]) @@ -166,7 +178,7 @@ func findAnyTypePositions(expr ast.Expr) []token.Pos { return positions } -func checkTypeScriptAnyUsage(root string) []anyUsageWarning { +func checkTypeScriptAnyUsage(root string, keep lineKeeper) []anyUsageWarning { var warnings []anyUsageWarning _ = filepath.Walk(root, func(path string, info os.FileInfo, err error) error { diff --git a/any_usage_test.go b/any_usage_test.go index 10fcdcb..986c53d 100644 --- a/any_usage_test.go +++ b/any_usage_test.go @@ -19,7 +19,7 @@ func Map() map[string]any { } `, 0644) - warnings := checkGoAnyUsage(root) + warnings := checkGoAnyUsage(root, nil) if len(warnings) < 3 { t.Fatalf("checkGoAnyUsage() returned %d warnings, want at least 3: %#v", len(warnings), warnings) } @@ -38,7 +38,7 @@ type Payload struct { } `, 0644) - warnings := checkGoAnyUsage(root) + warnings := checkGoAnyUsage(root, nil) if len(warnings) != 0 { t.Fatalf("checkGoAnyUsage() returned %d warnings, want 0: %#v", len(warnings), warnings) } @@ -50,7 +50,7 @@ func TestCheckTypeScriptAnyUsageFlagsAnyPatterns(t *testing.T) { export const fn = (value: any[]): any => value as any `, 0644) - warnings := checkTypeScriptAnyUsage(root) + warnings := checkTypeScriptAnyUsage(root, nil) if len(warnings) < 2 { t.Fatalf("checkTypeScriptAnyUsage() returned %d warnings, want at least 2: %#v", len(warnings), warnings) } @@ -63,7 +63,7 @@ const label = "any"; /* any */ `, 0644) - warnings := checkTypeScriptAnyUsage(root) + warnings := checkTypeScriptAnyUsage(root, nil) if len(warnings) != 0 { t.Fatalf("checkTypeScriptAnyUsage() returned %d warnings, want 0: %#v", len(warnings), warnings) } diff --git a/changedlines.go b/changedlines.go new file mode 100644 index 0000000..1526cf8 --- /dev/null +++ b/changedlines.go @@ -0,0 +1,135 @@ +package main + +import ( + "bufio" + "os/exec" + "path/filepath" + "strconv" + "strings" +) + +// diffIndex answers "is this file:line part of the unstaged working-tree diff?" +// It resolves each file to its enclosing git repo and, once per repo, parses +// `git diff --unified=0` (added/modified lines) plus untracked files (which count +// as fully changed). Everything is cached; files with no enclosing repo, or no +// matching change, are treated as unchanged. +type diffIndex struct { + repoOf map[string]string // dir -> git repo root ("" = none) + repos map[string]*repoDiff // repo root -> parsed diff +} + +type repoDiff struct { + changed map[string]map[int]bool // repo-relative slash path -> changed line set + untracked map[string]bool // repo-relative slash path (fully changed) +} + +func newDiffIndex() *diffIndex { + return &diffIndex{repoOf: map[string]string{}, repos: map[string]*repoDiff{}} +} + +// isChanged reports whether absPath:line lies in the unstaged working-tree diff. +func (d *diffIndex) isChanged(absPath string, line int) bool { + repo := d.repoRoot(filepath.Dir(absPath)) + if repo == "" { + return false + } + rd := d.repoDiff(repo) + rel, err := filepath.Rel(repo, absPath) + if err != nil { + return false + } + rel = filepath.ToSlash(rel) + if rd.untracked[rel] { + return true + } + if lines, ok := rd.changed[rel]; ok { + return lines[line] + } + return false +} + +// repoRoot walks up from dir to find the enclosing git repo root, cached. +func (d *diffIndex) repoRoot(dir string) string { + if root, ok := d.repoOf[dir]; ok { + return root + } + out, err := exec.Command("git", "-C", dir, "rev-parse", "--show-toplevel").Output() + root := "" + if err == nil { + root = strings.TrimSpace(string(out)) + } + d.repoOf[dir] = root + return root +} + +// repoDiff parses (and caches) the unstaged diff + untracked files for a repo. +func (d *diffIndex) repoDiff(repo string) *repoDiff { + if rd, ok := d.repos[repo]; ok { + return rd + } + rd := &repoDiff{changed: map[string]map[int]bool{}, untracked: map[string]bool{}} + + if out, err := exec.Command("git", "-C", repo, "diff", "--unified=0", "--no-color").Output(); err == nil { + parseUnifiedDiff(string(out), rd) + } + if out, err := exec.Command("git", "-C", repo, "ls-files", "--others", "--exclude-standard").Output(); err == nil { + for _, f := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if f != "" { + rd.untracked[filepath.ToSlash(f)] = true + } + } + } + + d.repos[repo] = rd + return rd +} + +// parseUnifiedDiff records added/modified line numbers per file from a +// `git diff --unified=0` payload. Only the new-file side (`+c,d`) matters. +func parseUnifiedDiff(diff string, rd *repoDiff) { + var cur string + sc := bufio.NewScanner(strings.NewReader(diff)) + sc.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) + for sc.Scan() { + line := sc.Text() + switch { + case strings.HasPrefix(line, "+++ b/"): + cur = strings.TrimPrefix(line, "+++ b/") + if _, ok := rd.changed[cur]; !ok { + rd.changed[cur] = map[int]bool{} + } + case strings.HasPrefix(line, "@@"): + start, count := parseHunkNewRange(line) + if start <= 0 || cur == "" { + continue + } + if rd.changed[cur] == nil { + rd.changed[cur] = map[int]bool{} + } + for i := 0; i < count; i++ { + rd.changed[cur][start+i] = true + } + } + } +} + +// parseHunkNewRange extracts (start, count) from the `+c,d` part of a hunk +// header like `@@ -a,b +c,d @@`. A missing `,d` means count 1. +func parseHunkNewRange(header string) (int, int) { + plus := strings.IndexByte(header, '+') + if plus < 0 { + return 0, 0 + } + rest := header[plus+1:] + if sp := strings.IndexByte(rest, ' '); sp >= 0 { + rest = rest[:sp] + } + start, count := 0, 1 + if comma := strings.IndexByte(rest, ','); comma >= 0 { + start, _ = strconv.Atoi(rest[:comma]) + count, _ = strconv.Atoi(rest[comma+1:]) + } else { + start, _ = strconv.Atoi(rest) + } + return start, count +} diff --git a/changedlines_test.go b/changedlines_test.go new file mode 100644 index 0000000..d79b2b6 --- /dev/null +++ b/changedlines_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +func TestParseHunkNewRange(t *testing.T) { + cases := []struct { + header string + wantStart int + wantCount int + }{ + {"@@ -1,2 +3,4 @@", 3, 4}, + {"@@ -0,0 +1 @@", 1, 1}, // no ,count on the + side → count 1 + {"@@ -5,0 +6,3 @@ func Foo() {", 6, 3}, // trailing context after @@ + {"@@ -1 +1 @@", 1, 1}, + {"not a hunk", 0, 0}, + } + for _, c := range cases { + start, count := parseHunkNewRange(c.header) + if start != c.wantStart || count != c.wantCount { + t.Errorf("parseHunkNewRange(%q) = (%d,%d), want (%d,%d)", c.header, start, count, c.wantStart, c.wantCount) + } + } +} + +func TestParseUnifiedDiffRecordsAddedLines(t *testing.T) { + diff := "diff --git a/foo.go b/foo.go\n" + + "--- a/foo.go\n" + + "+++ b/foo.go\n" + + "@@ -10,0 +11,2 @@\n" + + "+added one\n" + + "+added two\n" + rd := &repoDiff{changed: map[string]map[int]bool{}, untracked: map[string]bool{}} + parseUnifiedDiff(diff, rd) + + lines := rd.changed["foo.go"] + if !lines[11] || !lines[12] { + t.Fatalf("expected lines 11,12 changed, got %#v", lines) + } + if lines[10] || lines[13] { + t.Fatalf("unexpected extra changed lines: %#v", lines) + } +} + +func TestDiffIndexIsChangedTracksWorkingTreeEdits(t *testing.T) { + repo := t.TempDir() + runGit := func(args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", repo}, args...)...) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + runGit("init") + runGit("config", "user.email", "test@example.com") + runGit("config", "user.name", "test") + + committed := filepath.Join(repo, "committed.go") + if err := os.WriteFile(committed, []byte("package p\n\nvar a int\nvar b int\n"), 0o644); err != nil { + t.Fatal(err) + } + runGit("add", "committed.go") + runGit("commit", "-m", "init") + + // Unstaged edit: append a line to the committed file. + if err := os.WriteFile(committed, []byte("package p\n\nvar a int\nvar b int\nvar c int\n"), 0o644); err != nil { + t.Fatal(err) + } + // Untracked new file. + untracked := filepath.Join(repo, "new.go") + if err := os.WriteFile(untracked, []byte("package p\n\nvar d int\n"), 0o644); err != nil { + t.Fatal(err) + } + + idx := newDiffIndex() + + if idx.isChanged(committed, 3) { + t.Errorf("line 3 (unchanged, committed) should not be reported as changed") + } + if !idx.isChanged(committed, 5) { + t.Errorf("line 5 (the unstaged append) should be reported as changed") + } + if !idx.isChanged(untracked, 1) || !idx.isChanged(untracked, 3) { + t.Errorf("all lines of an untracked file should be reported as changed") + } +} + +func TestDiffIndexIsChangedOutsideRepoIsFalse(t *testing.T) { + dir := t.TempDir() // not a git repo + f := filepath.Join(dir, "x.go") + if err := os.WriteFile(f, []byte("package p\n"), 0o644); err != nil { + t.Fatal(err) + } + idx := newDiffIndex() + if idx.isChanged(f, 1) { + t.Errorf("files outside any git repo must not be reported as changed") + } +} diff --git a/check_anyusage.go b/check_anyusage.go index f21c36a..f74873c 100644 --- a/check_anyusage.go +++ b/check_anyusage.go @@ -1,49 +1,64 @@ package main -import "fmt" - func init() { register(Check{ Seq: 24, ID: "2e", Title: "Warn on any usage in Go and TypeScript", Run: func(ctx *ScanContext, rep *Reporter) { - // Check 2e: Warn on any usage in Go and TypeScript - fmt.Println("=== Check 2e: Warn on any usage in Go and TypeScript ===") + // By default only warn on `any` introduced in the unstaged working-tree + // diff; --all-any scans every source file. + var keep lineKeeper + if !ctx.allAny { + idx := newDiffIndex() + keep = idx.isChanged + } + var anyWarnings []anyUsageWarning for _, target := range ctx.backendTargets { - for _, w := range checkGoAnyUsage(target.root) { + for _, w := range checkGoAnyUsage(target.root, keep) { w.file = prefixDisplayPath(target.displayOrRoot(), w.file) anyWarnings = append(anyWarnings, w) } } for _, target := range ctx.pluginTargets { - for _, w := range checkGoAnyUsage(target.root) { + for _, w := range checkGoAnyUsage(target.root, keep) { w.file = prefixDisplayPath(target.display, w.file) anyWarnings = append(anyWarnings, w) } } for _, target := range collectESLintDirectiveTargets(ctx.repoRoot, ctx.frontendTargets) { - for _, w := range checkTypeScriptAnyUsage(target.dir) { + for _, w := range checkTypeScriptAnyUsage(target.dir, keep) { w.file = prefixDisplayPath(target.label, w.file) anyWarnings = append(anyWarnings, w) } } - if len(anyWarnings) > 0 { - fmt.Printf(" WARN: %d any usage warning(s):\n", len(anyWarnings)) - limit := min(len(anyWarnings), maxAnyWarningsPrinted) - for _, w := range anyWarnings[:limit] { - fmt.Printf(" %s:%d [%s] %s\n", w.file, w.line, w.lang, w.snippet) + + if len(anyWarnings) == 0 { + if ctx.allAny { + rep.OK("no any usage found") + } else { + rep.OK("no any usage in changed lines") } - if len(anyWarnings) > limit { - fmt.Printf(" ... %d additional warning(s) omitted\n", len(anyWarnings)-limit) - } - fmt.Println("\n Guidance: prefer concrete types, generics, typed maps/structs, or unknown-style narrowing patterns over any.") - } else { - fmt.Println(" OK: No any usage found in scanned Go or TypeScript sources") + return } - fmt.Println() + scope := "any usage" + if !ctx.allAny { + scope = "any usage in changed lines" + } + limit := min(len(anyWarnings), maxAnyWarningsPrinted) + extra := "" + if !ctx.allAny { + extra = ", --all-any to scan all" + } + rep.Warn("%d %s (showing %d%s)", len(anyWarnings), scope, limit, extra) + for _, w := range anyWarnings[:limit] { + rep.Findingf("%s:%d [%s] %s", w.file, w.line, w.lang, w.snippet) + } + if len(anyWarnings) > limit { + rep.Findingf("... %d more omitted", len(anyWarnings)-limit) + } }, }) } diff --git a/check_authprecedence.go b/check_authprecedence.go index 97603b4..1e3f6cb 100644 --- a/check_authprecedence.go +++ b/check_authprecedence.go @@ -1,15 +1,11 @@ package main -import "fmt" - func init() { register(Check{ Seq: 190, ID: "19", Title: "Bearer token precedence over cookies in auth middleware", Run: func(ctx *ScanContext, rep *Reporter) { - // Check 19: Bearer token precedence over cookies - fmt.Println("\n=== Check 19: Bearer token precedence over cookies in auth middleware ===") var authPrecViolations []authPrecedenceViolation for _, target := range ctx.backendTargets { for _, v := range checkAuthPrecedence(target.root) { @@ -24,15 +20,12 @@ func init() { } } if len(authPrecViolations) > 0 { - fmt.Printf(" FAIL: %d auth precedence violation(s):\n", len(authPrecViolations)) + rep.Fail("%d auth precedence violation(s) — check Bearer/Authorization header before cookies", len(authPrecViolations)) for _, v := range authPrecViolations { - fmt.Printf(" %s:%d %s\n", v.file, v.line, v.snippet) + rep.Findingf("%s:%d %s", v.file, v.line, v.snippet) } - fmt.Println("\n Fix: Check Bearer/Authorization header BEFORE cookies. Bearer is explicit; cookies are ambient and may be stale.") - rep.Fail() } else { - fmt.Println(" OK: Bearer token takes precedence over cookies in all auth extraction") - printPerTargetOKLines(pluginTargetLabels(ctx.pluginTargets)) + rep.OK("Bearer token takes precedence over cookies in all auth extraction") } }, }) diff --git a/check_bareimports.go b/check_bareimports.go index e6df900..b7cbe6a 100644 --- a/check_bareimports.go +++ b/check_bareimports.go @@ -1,15 +1,11 @@ package main -import "fmt" - func init() { register(Check{ Seq: 80, ID: "8", Title: "Import @block-ninja/api from subpaths only", Run: func(ctx *ScanContext, rep *Reporter) { - // Check 8: Import @block-ninja/api from subpaths only - fmt.Println("=== Check 8: Import @block-ninja/api from subpaths only ===") if len(ctx.frontendTargets) > 0 { var bareImports []bareImportViolation for _, target := range ctx.frontendTargets { @@ -19,21 +15,16 @@ func init() { } } if len(bareImports) > 0 { - fmt.Printf(" FAIL: %d bare import(s) from '@block-ninja/api' (naming conflicts):\n", len(bareImports)) + rep.Fail("%d bare import(s) from '@block-ninja/api' — import from subpaths (/types, /queries, /services)", len(bareImports)) for _, v := range bareImports { - fmt.Printf(" %s:%d %s\n", v.file, v.line, v.snippet) + rep.Findingf("%s:%d %s", v.file, v.line, v.snippet) } - fmt.Println("\n Fix: Import from subpaths: @block-ninja/api/types, /queries, or /services") - rep.Fail() } else { - fmt.Println(" OK: All @block-ninja/api imports use subpaths") - printPerTargetOKLines(frontendTargetLabels(ctx.frontendTargets)) + rep.OK("All @block-ninja/api imports use subpaths") } } else { - fmt.Println(" SKIP: no frontend sources found") + rep.Skip("no frontend sources found") } - - fmt.Println() }, }) } diff --git a/check_buildmodeplugin.go b/check_buildmodeplugin.go index fd293b9..9656ee9 100644 --- a/check_buildmodeplugin.go +++ b/check_buildmodeplugin.go @@ -1,7 +1,5 @@ package main -import "fmt" - // enableBuildmodePluginCheck is the per-check enable toggle. It ships DISABLED // (WO-WZ-009): the wasm plugin migration is still porting the fleet off `.so`, // so a `-buildmode=plugin` target is not yet a hard error. WO-WZ-017 flips this @@ -20,20 +18,15 @@ func init() { if !enableBuildmodePluginCheck { return // disabled until WO-WZ-017; emit no output } - fmt.Println("=== Check 29: No -buildmode=plugin targets in ported plugin repos ===") violations := checkBuildmodePlugin(ctx.resolvedPluginTargets) if len(violations) > 0 { - fmt.Printf(" FAIL: %d legacy .so build target(s) found:\n", len(violations)) + rep.Fail("%d legacy .so build target(s) found — build wasm with `ninja plugin build`", len(violations)) for _, v := range violations { - fmt.Printf(" %s:%d %s\n", v.file, v.line, v.snippet) + rep.Findingf("%s:%d %s", v.file, v.line, v.snippet) } - fmt.Println("\n Fix: build wasm with `ninja plugin build` (make build-wasm); drop the -buildmode=plugin target.") - rep.Fail() } else { - fmt.Println(" OK: No -buildmode=plugin targets found") - printPerTargetOKLines(pluginTargetLabels(ctx.resolvedPluginTargets)) + rep.OK("No -buildmode=plugin targets found") } - fmt.Println() }, }) } diff --git a/check_buttonautomation.go b/check_buttonautomation.go index a070a0a..2bd6828 100644 --- a/check_buttonautomation.go +++ b/check_buttonautomation.go @@ -1,15 +1,11 @@ package main -import "fmt" - func init() { register(Check{ Seq: 100, ID: "10", Title: "Button automation attributes", Run: func(ctx *ScanContext, rep *Reporter) { - // Check 10: Button automation attributes - fmt.Println("=== Check 10: Button automation attributes ===") if len(ctx.frontendTargets) > 0 { var buttonViolations []buttonViolation for _, target := range ctx.frontendTargets { @@ -19,22 +15,16 @@ func init() { } } if len(buttonViolations) > 0 { - fmt.Printf(" FAIL: %d