diff --git a/CHANGELOG.md b/CHANGELOG.md index 995639e..3b68803 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,20 @@ and the project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/v ## [Unreleased] +### Added +- **Deterministic flaky-test detector (ADR-0022).** A static, provider-free + pre-pass scans the added lines of changed test files for high-precision + flakiness anti-patterns — hard-coded sleeps / fixed waits (`time.Sleep`, + `Thread.sleep`, `Task.Delay`, `asyncio.sleep`, `*.waitForTimeout`, numeric + `cy.wait`, `usleep`, `sleep()`) and unseeded randomness (`Math.random`, + Python `random.*`, Go `math/rand`) — and merges them into the structured + findings, so they render, count toward `--fail-on`, and `--copy` like any + other finding. Deterministic and reproducible: no model call, no JSON-schema + change (the findings contract stays v1). On by default for API/mock + providers; skip per-run with `--no-flaky` or persistently with + `review.flaky: false`. Localized (en/tr). CLI-tool-backed plain-text + providers are unaffected for now. + ## [1.6.0] - 2026-06-13 ### Added diff --git a/README.md b/README.md index 1d91bcf..b16942e 100644 --- a/README.md +++ b/README.md @@ -244,8 +244,22 @@ read project files beyond the diff to ground the review; see below), `--allow-secrets` (acknowledge a flagged credential in the diff), `--no-cost-check` (skip cost preflight), `--show-prompt` (print the exact system + user prompt that would be sent, -then exit — no provider call, no cost; honours `--output`), `--color`. See -`commitbrief --help`. +then exit — no provider call, no cost; honours `--output`), `--no-flaky` +(skip the flaky-test detector below), `--color`. See `commitbrief --help`. + +### Flaky-test detection (deterministic, ADR-0022) + +Before the model is called, a **static pre-pass** scans the added lines of any +changed **test** files for high-precision flakiness anti-patterns — hard-coded +sleeps / fixed waits (`time.Sleep`, `Thread.sleep`, `Task.Delay`, +`asyncio.sleep`, `*.waitForTimeout`, numeric `cy.wait`, `usleep`, `sleep()`) +and unseeded randomness (`Math.random`, Python `random.*`, Go `math/rand`). +Matches merge into the normal findings, so they render, count toward +`--fail-on`, and `--copy` like any other finding — but they are **deterministic +and reproducible**: no model call, no JSON-schema change. On by default for the +API/mock providers; turn it off per-run with `--no-flaky` or persistently with +`review.flaky: false`. CLI-tool-backed plain-text providers are unaffected for +now. ### `commitbrief commit` @@ -493,6 +507,8 @@ command: commit: type: plain # default --type for `commitbrief commit` (plain|conventional|conventional+body|gitmoji|subject+body) generate: 1 # default --generate (number of message alternatives) +review: + flaky: true # deterministic flaky-test detector pre-pass (ADR-0022); --no-flaky overrides per-run ``` ### Default command (`command.default`) diff --git a/internal/cli/config.go b/internal/cli/config.go index 8d63ed7..67949a1 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -225,12 +225,23 @@ func configFieldGet(cfg *config.Config, path string) (string, error) { return "", fmt.Errorf("config: unknown field %q in command (allowed: default)", parts[1]) } + case "review": + if len(parts) != 2 { + return "", fmt.Errorf("config: %q must be review.", path) + } + switch parts[1] { + case "flaky": + return strconv.FormatBool(cfg.Review.Flaky), nil + default: + return "", fmt.Errorf("config: unknown field %q in review (allowed: flaky)", parts[1]) + } + case "version": // Read-only via get; explicitly rejected by configFieldSet. return strconv.Itoa(cfg.Version), nil default: - return "", fmt.Errorf("config: unknown top-level field %q (allowed: provider, providers.*, output.*, cache.*, guard.*, cost.*, command.*, version)", parts[0]) + return "", fmt.Errorf("config: unknown top-level field %q (allowed: provider, providers.*, output.*, cache.*, guard.*, cost.*, command.*, review.*, version)", parts[0]) } } @@ -392,11 +403,27 @@ func configFieldSet(cfg *config.Config, path, value string) error { } return nil + case "review": + if len(parts) != 2 { + return fmt.Errorf("config: %q must be review.", path) + } + switch parts[1] { + case "flaky": + b, err := parseConfigBool(value) + if err != nil { + return fmt.Errorf("config: review.flaky: %w", err) + } + cfg.Review.Flaky = b + default: + return fmt.Errorf("config: unknown field %q in review (allowed: flaky)", parts[1]) + } + return nil + case "version": return errors.New("config: version is managed by migrations and cannot be set manually") default: - return fmt.Errorf("config: unknown top-level field %q (allowed: provider, providers.*, output.*, cache.*, guard.*, cost.*, command.*)", parts[0]) + return fmt.Errorf("config: unknown top-level field %q (allowed: provider, providers.*, output.*, cache.*, guard.*, cost.*, command.*, review.*)", parts[0]) } } diff --git a/internal/cli/flaky_merge_test.go b/internal/cli/flaky_merge_test.go new file mode 100644 index 0000000..a93842c --- /dev/null +++ b/internal/cli/flaky_merge_test.go @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package cli + +import ( + "testing" + + "github.com/CommitBrief/commitbrief/internal/render" +) + +func TestMergeFlaky(t *testing.T) { + llm := []render.Finding{ + {Severity: render.SeverityHigh, File: "a.go", Line: 10, Title: "llm-1"}, + {Severity: render.SeverityLow, File: "b.go", Line: 20, Title: "llm-2"}, + } + flaky := []render.Finding{ + // same file:line as llm-1 → dropped (the model already covered the line) + {Severity: render.SeverityMedium, File: "a.go", Line: 10, Title: "flaky-dup"}, + // unique line → kept + {Severity: render.SeverityMedium, File: "c.go", Line: 5, Title: "flaky-new"}, + } + + got := mergeFlaky(llm, flaky) + if len(got) != 3 { + t.Fatalf("len = %d, want 3 (2 llm + 1 unique flaky): %+v", len(got), got) + } + // Every LLM finding is preserved, in order, ahead of the flaky ones. + if got[0].Title != "llm-1" || got[1].Title != "llm-2" { + t.Errorf("LLM findings not preserved in order: %+v", got[:2]) + } + for _, f := range got { + if f.Title == "flaky-dup" { + t.Errorf("flaky finding at an LLM-occupied line should be dropped: %+v", f) + } + } + if got[2].Title != "flaky-new" { + t.Errorf("unique flaky finding should be appended last; got %+v", got[2]) + } +} + +func TestMergeFlaky_Empty(t *testing.T) { + llm := []render.Finding{{Severity: render.SeverityInfo, File: "a.go", Line: 1, Title: "x"}} + // No flaky findings → llm returned unchanged (the common + plain-text path). + if got := mergeFlaky(llm, nil); len(got) != 1 || got[0].Title != "x" { + t.Errorf("mergeFlaky(llm, nil) should return llm unchanged; got %+v", got) + } + // No LLM findings (clean review) + flaky present → flaky surfaced. + flaky := []render.Finding{{Severity: render.SeverityMedium, File: "t_test.go", Line: 3, Title: "f"}} + if got := mergeFlaky(nil, flaky); len(got) != 1 || got[0].Title != "f" { + t.Errorf("mergeFlaky(nil, flaky) should return flaky; got %+v", got) + } +} diff --git a/internal/cli/review.go b/internal/cli/review.go index ad2664c..dd6daae 100644 --- a/internal/cli/review.go +++ b/internal/cli/review.go @@ -12,6 +12,7 @@ import ( "io" "os" "path/filepath" + "strconv" "strings" "time" @@ -21,6 +22,7 @@ import ( "github.com/CommitBrief/commitbrief/internal/clipboard" "github.com/CommitBrief/commitbrief/internal/config" "github.com/CommitBrief/commitbrief/internal/diff" + "github.com/CommitBrief/commitbrief/internal/flaky" "github.com/CommitBrief/commitbrief/internal/git" "github.com/CommitBrief/commitbrief/internal/guard" "github.com/CommitBrief/commitbrief/internal/ignore" @@ -233,6 +235,16 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er return showPromptOutput(cmd, p) } + // Deterministic flaky-test pre-pass (ADR-0022). Static, provider-free + // findings from the filtered diff, merged into the structured results in + // both the cache-hit and fresh paths below. Skipped for plain-text/CLI + // providers (their output isn't structured) and when disabled via + // review.flaky=false or --no-flaky. + var flakyFindings []render.Finding + if app.Config.Review.Flaky && !global.noFlaky && !plainText { + flakyFindings = flaky.New(app.Catalog).Detect(parsed) + } + model := app.Config.Providers[app.Config.Provider].Model if model == "" { model = prov.DefaultModel() @@ -289,6 +301,7 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er case cache.FormatJSON, "": findings, _ = render.ParseFindings(entry.Result.Content) } + findings = mergeFlaky(findings, flakyFindings) if entry.Result.Format == cache.FormatPlainText { // CLI-emitted output: stream the cached body verbatim to // stdout instead of going through the cards renderer. @@ -407,6 +420,7 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er case cache.FormatMarkdownFallback: _, _ = fmt.Fprintln(cmd.ErrOrStderr(), app.Catalog.T("review.degraded")) } + findings = mergeFlaky(findings, flakyFindings) respModel := model meta := render.Meta{ @@ -466,6 +480,31 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er return applyFailOn(cmd, app, findings) } +// mergeFlaky appends deterministic flaky-test findings (ADR-0022) to the LLM +// findings, skipping any whose (file, line) the LLM already reported so the +// same line isn't surfaced twice. Every LLM finding is preserved; flaky +// findings are additive. Returns llm unchanged when there are no flaky +// findings — the common case, and the plain-text/CLI path where the detector +// never ran. +func mergeFlaky(llm, flakyFindings []render.Finding) []render.Finding { + if len(flakyFindings) == 0 { + return llm + } + seen := make(map[string]struct{}, len(llm)) + for _, f := range llm { + seen[f.File+":"+strconv.Itoa(f.Line)] = struct{}{} + } + out := make([]render.Finding, 0, len(llm)+len(flakyFindings)) + out = append(out, llm...) + for _, f := range flakyFindings { + if _, dup := seen[f.File+":"+strconv.Itoa(f.Line)]; dup { + continue + } + out = append(out, f) + } + return out +} + // emitPlainText streams a CLI-provider's already-formatted output // verbatim. We don't run it through the cards renderer or glamour — // the host CLI's output is the final form the user wants to see, and diff --git a/internal/cli/root.go b/internal/cli/root.go index b95670f..9daadb1 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -31,6 +31,7 @@ type globalFlags struct { compact bool allowSecrets bool noCostCheck bool + noFlaky bool copy bool suggestCommit bool commitType string // commit: --type ; "" → commit.type config → "plain" @@ -95,6 +96,7 @@ func newRootCmd() *cobra.Command { flags.BoolVar(&global.compact, "compact", false, "one-line per finding (dense review output)") flags.BoolVar(&global.allowSecrets, "allow-secrets", false, "bypass the pre-send secret scanner (use with care)") flags.BoolVar(&global.noCostCheck, "no-cost-check", false, "skip the pre-send cost estimate prompt") + flags.BoolVar(&global.noFlaky, "no-flaky", false, "skip the deterministic flaky-test detector (ADR-0022)") flags.BoolVar(&global.copy, "copy", false, "copy findings (severity, path, title, description) to the system clipboard via OSC 52 + native tool") flags.BoolVar(&global.suggestCommit, "suggest-commit", false, "after the review, suggest a Conventional Commit message for the staged diff (requires --staged; prints to stdout; not with --json/--markdown/--output)") flags.StringVar(&global.failOn, "fail-on", "", "exit 1 if any finding meets/exceeds severity (critical|high|medium|low|info|any|none)") diff --git a/internal/config/config.go b/internal/config/config.go index 44deee7..bc5356e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,16 @@ type Config struct { Cost CostConfig `yaml:"cost"` Command CommandConfig `yaml:"command"` Commit CommitConfig `yaml:"commit"` + Review ReviewConfig `yaml:"review"` +} + +// ReviewConfig toggles review-time behaviors that aren't pre-send guards. +// Flaky enables the deterministic static flaky-test detector (ADR-0022): a +// provider-free pre-pass that flags timing/randomness anti-patterns in +// changed test files and merges them into the structured findings. On by +// default; precedence is --no-flaky > review.flaky config > built-in (true). +type ReviewConfig struct { + Flaky bool `yaml:"flaky"` } // CommitConfig sets defaults for the `commit` command (ADR-0019) so a repo diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 89a2103..21092d6 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -31,5 +31,8 @@ func Default() *Config { Type: "plain", Generate: 1, }, + Review: ReviewConfig{ + Flaky: true, + }, } } diff --git a/internal/flaky/flaky.go b/internal/flaky/flaky.go new file mode 100644 index 0000000..be5eaf7 --- /dev/null +++ b/internal/flaky/flaky.go @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +// Package flaky is CommitBrief's deterministic, static flaky-test +// anti-pattern detector (ADR-0022). Unlike the LLM review path it produces +// reproducible findings from the diff alone: it scans the added lines of +// changed test files for high-precision anti-patterns (hard-coded sleeps, +// unseeded randomness, …) and emits standard render.Finding values — no +// JSON-schema change, no provider call. Recall is intentionally secondary +// to precision: a noisy commit-stage gate erodes trust. +package flaky + +import ( + "strings" + + "github.com/CommitBrief/commitbrief/internal/diff" + "github.com/CommitBrief/commitbrief/internal/i18n" + "github.com/CommitBrief/commitbrief/internal/render" +) + +// Detector turns a parsed diff into deterministic flaky-test findings. The +// catalog localizes finding text to the resolved output language (ADR-0021); +// the Severity value stays the English wire vocabulary, as for LLM findings. +type Detector struct { + cat *i18n.Catalog +} + +// New returns a Detector that localizes finding text via cat. +func New(cat *i18n.Catalog) *Detector { return &Detector{cat: cat} } + +// Detect scans the added lines of every changed test file in parsed and +// returns the matched anti-patterns as findings. The returned slice is +// non-nil only when there are findings; order follows file → hunk → line. +func (d *Detector) Detect(parsed diff.Diff) []render.Finding { + var out []render.Finding + for _, f := range parsed.Files { + if f.Binary || f.Mode == diff.ModeDeleted { + continue + } + if !isTestFile(f.Path) { + continue + } + lang := detectLang(f.Path) + for _, h := range f.Hunks { + // new-file line cursor: context and added lines advance it, + // deleted lines do not (standard unified-diff walk). + line := h.NewStart + for _, l := range h.Lines { + switch l.Kind { + case diff.LineContext: + line++ + case diff.LineAdd: + out = append(out, d.scanLine(f.Path, lang, line, l.Text)...) + line++ + case diff.LineDel: + // does not advance the new-file cursor + } + } + } + } + return out +} + +// scanLine evaluates one added line against every applicable rule. A line may +// match more than one distinct rule, but a single rule matches a line at most +// once (regexp alternation collapses internal duplicates). +func (d *Detector) scanLine(path, lang string, line int, text string) []render.Finding { + var out []render.Finding + for _, r := range rules { + if !r.appliesTo(lang) || !r.pattern.MatchString(text) { + continue + } + out = append(out, render.Finding{ + Severity: r.severity, + File: path, + Line: line, + Title: d.cat.T(r.titleKey), + Description: d.cat.T(r.descKey), + Suggestion: d.cat.T(r.sugKey), + Language: lang, + Snippet: "+" + text, + }) + } + return out +} + +// isTestFile reports whether path looks like a test file by convention across +// the languages CommitBrief commonly reviews. Conservative on directories so +// non-test fixtures rarely match; the rules themselves are the second filter. +func isTestFile(path string) bool { + p := strings.ToLower(toSlash(path)) + b := base(p) + + switch { + case strings.HasSuffix(b, "_test.go"): + return true + case strings.HasSuffix(b, "_test.py"), strings.HasPrefix(b, "test_") && strings.HasSuffix(b, ".py"): + return true + case strings.HasSuffix(b, "_spec.rb"), strings.HasSuffix(b, "_test.rb"): + return true + case strings.HasSuffix(b, "test.java"), strings.HasSuffix(b, "tests.java"): + return true + case strings.HasSuffix(b, "test.cs"), strings.HasSuffix(b, "tests.cs"): + return true + case strings.HasSuffix(b, "test.php"): + return true + case containsAny(b, ".test.", ".spec."): + return true + } + + for _, seg := range strings.Split(p, "/") { + switch seg { + case "__tests__", "tests", "test", "spec", "e2e", "cypress": + return true + } + } + return false +} + +// detectLang maps a file extension to the short language identifier used for +// Finding.Language and for per-rule language gating. Empty when unknown. +func detectLang(path string) string { + ext := strings.ToLower(extension(path)) + switch ext { + case ".go": + return "go" + case ".js", ".jsx", ".mjs", ".cjs": + return "js" + case ".ts", ".tsx": + return "ts" + case ".py": + return "python" + case ".java": + return "java" + case ".kt", ".kts": + return "kotlin" + case ".rb": + return "ruby" + case ".php": + return "php" + case ".cs": + return "csharp" + default: + return "" + } +} + +// toSlash normalizes separators to "/" without importing path/filepath, so +// path handling is identical on every OS (paths in a diff are already "/"). +func toSlash(p string) string { return strings.ReplaceAll(p, "\\", "/") } + +// base returns the final "/"-separated segment of p. +func base(p string) string { + if i := strings.LastIndex(p, "/"); i >= 0 { + return p[i+1:] + } + return p +} + +// extension returns the dotted extension of p (including the leading "."), +// or "" when the final segment has none. +func extension(p string) string { + b := base(p) + if i := strings.LastIndex(b, "."); i > 0 { + return b[i:] + } + return "" +} + +func containsAny(s string, subs ...string) bool { + for _, sub := range subs { + if strings.Contains(s, sub) { + return true + } + } + return false +} diff --git a/internal/flaky/flaky_test.go b/internal/flaky/flaky_test.go new file mode 100644 index 0000000..ad63eba --- /dev/null +++ b/internal/flaky/flaky_test.go @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package flaky + +import ( + "strings" + "testing" + + "github.com/CommitBrief/commitbrief/internal/diff" + "github.com/CommitBrief/commitbrief/internal/git" + "github.com/CommitBrief/commitbrief/internal/i18n" + "github.com/CommitBrief/commitbrief/internal/render" +) + +func TestIsTestFile(t *testing.T) { + cases := []struct { + path string + want bool + }{ + {"internal/worker/job_test.go", true}, + {"tests/test_login.py", true}, + {"app/login_test.py", true}, + {"src/components/Button.test.tsx", true}, + {"e2e/login.spec.ts", true}, + {"spec/models/user_spec.rb", true}, + {"src/test/java/com/acme/JobTest.java", true}, + {"Service.Tests.cs", true}, + {"tests/Feature/LoginTest.php", true}, + {"__tests__/util.js", true}, + {"internal/worker/job.go", false}, + {"src/components/Button.tsx", false}, + {"app/models/user.rb", false}, + {"README.md", false}, + } + for _, c := range cases { + if got := isTestFile(c.path); got != c.want { + t.Errorf("isTestFile(%q) = %v, want %v", c.path, got, c.want) + } + } +} + +func TestDetectLang(t *testing.T) { + cases := map[string]string{ + "a_test.go": "go", + "a.test.ts": "ts", + "a.test.jsx": "js", + "test_a.py": "python", + "AThingTest.java": "java", + "user_spec.rb": "ruby", + "LoginTest.php": "php", + "Svc.Tests.cs": "csharp", + "Makefile": "", + } + for path, want := range cases { + if got := detectLang(path); got != want { + t.Errorf("detectLang(%q) = %q, want %q", path, got, want) + } + } +} + +// loadCatalog loads the English catalog so finding text resolves through the +// same path production uses; a missing key would surface as the raw key. +func loadCatalog(t *testing.T) *i18n.Catalog { + t.Helper() + cat, err := i18n.Load("en") + if err != nil { + t.Fatalf("i18n.Load: %v", err) + } + return cat +} + +func detect(t *testing.T, cat *i18n.Catalog, raw string) []render.Finding { + t.Helper() + parsed, err := diff.Parse(git.Diff{Content: raw}) + if err != nil { + t.Fatalf("diff.Parse: %v", err) + } + return New(cat).Detect(parsed) +} + +// assertResolved checks that the catalog text fields are populated (not the +// raw key) and that the finding satisfies the render.Finding invariants the +// JSON/markdown renderers rely on. +func assertResolved(t *testing.T, f render.Finding) { + t.Helper() + if !f.Severity.IsValid() { + t.Errorf("invalid severity %q", f.Severity) + } + if f.Title == "" || strings.HasPrefix(f.Title, "flaky.") { + t.Errorf("title not resolved: %q", f.Title) + } + if f.Description == "" || f.Suggestion == "" { + t.Errorf("missing description/suggestion: %+v", f) + } + if !strings.HasPrefix(f.Snippet, "+") { + t.Errorf("snippet should keep the diff prefix: %q", f.Snippet) + } +} + +func TestDetect_GoLineNumbersAndRules(t *testing.T) { + cat := loadCatalog(t) + // Cursor starts at NewStart=10. Context advances, deleted does not: + // 10 setup() (context) + // -- old() (deleted, no advance) + // 11 time.Sleep(..) (added) -> hard-sleep @ 11 + // 12 rand.Intn(..) (added) -> unseeded-random @ 12 + // 13 assert(x) (context) + raw := `diff --git a/worker/job_test.go b/worker/job_test.go +--- a/worker/job_test.go ++++ b/worker/job_test.go +@@ -10,4 +10,5 @@ func TestJob(t *testing.T) { + setup() +- old() ++ time.Sleep(2 * time.Second) ++ x := rand.Intn(100) + assert(x) +` + got := detect(t, cat, raw) + if len(got) != 2 { + t.Fatalf("len(findings) = %d, want 2: %+v", len(got), got) + } + for _, f := range got { + assertResolved(t, f) + if f.File != "worker/job_test.go" { + t.Errorf("file = %q", f.File) + } + if f.Language != "go" { + t.Errorf("language = %q, want go", f.Language) + } + } + sleepF, randF := got[0], got[1] + if sleepF.Line != 11 || sleepF.Severity != render.SeverityMedium { + t.Errorf("hard-sleep: line=%d sev=%s, want 11/medium", sleepF.Line, sleepF.Severity) + } + if !strings.Contains(strings.ToLower(sleepF.Title), "sleep") { + t.Errorf("hard-sleep title = %q", sleepF.Title) + } + if randF.Line != 12 || randF.Severity != render.SeverityLow { + t.Errorf("unseeded-random: line=%d sev=%s, want 12/low", randF.Line, randF.Severity) + } + if !strings.Contains(strings.ToLower(randF.Title), "random") { + t.Errorf("unseeded-random title = %q", randF.Title) + } +} + +func TestDetect_NonTestFileIgnored(t *testing.T) { + cat := loadCatalog(t) + raw := `diff --git a/worker/job.go b/worker/job.go +--- a/worker/job.go ++++ b/worker/job.go +@@ -1,1 +1,2 @@ + package worker ++ time.Sleep(1 * time.Second) +` + if got := detect(t, cat, raw); len(got) != 0 { + t.Fatalf("non-test file should yield no findings, got %d: %+v", len(got), got) + } +} + +func TestDetect_CypressFixedWaitPrecision(t *testing.T) { + cat := loadCatalog(t) + // cy.wait() is flaky; cy.wait('@alias') is the correct pattern + // and must NOT be flagged. + raw := `diff --git a/e2e/login.spec.ts b/e2e/login.spec.ts +--- a/e2e/login.spec.ts ++++ b/e2e/login.spec.ts +@@ -5,1 +5,3 @@ + it('logs in', () => { ++ cy.wait(2000) ++ cy.wait('@loginRequest') +` + got := detect(t, cat, raw) + if len(got) != 1 { + t.Fatalf("want exactly 1 finding (numeric cy.wait only), got %d: %+v", len(got), got) + } + if got[0].Line != 6 { + t.Errorf("cy.wait line = %d, want 6", got[0].Line) + } + assertResolved(t, got[0]) +} + +func TestDetect_DeletedAndBinaryFilesSkipped(t *testing.T) { + cat := loadCatalog(t) + raw := `diff --git a/foo_test.go b/foo_test.go +deleted file mode 100644 +--- a/foo_test.go ++++ /dev/null +@@ -1,2 +0,0 @@ +- time.Sleep(5) +- old() +` + if got := detect(t, cat, raw); len(got) != 0 { + t.Fatalf("deleted file should be skipped, got %d: %+v", len(got), got) + } +} diff --git a/internal/flaky/rules.go b/internal/flaky/rules.go new file mode 100644 index 0000000..909d639 --- /dev/null +++ b/internal/flaky/rules.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package flaky + +import ( + "regexp" + "strings" + + "github.com/CommitBrief/commitbrief/internal/render" +) + +// rule is one deterministic anti-pattern. pattern is matched against the raw +// text of an added line (the diff marker is already stripped by the parser). +// langs nil means "every language"; otherwise the rule only applies to the +// listed identifiers from detectLang. The *Key fields are i18n catalog keys +// (present in messages.en.yml and messages.tr.yml). +type rule struct { + id string + langs map[string]bool + pattern *regexp.Regexp + severity render.Severity + titleKey string + descKey string + sugKey string +} + +func (r rule) appliesTo(lang string) bool { + if r.langs == nil { + return true + } + return r.langs[lang] +} + +// alt joins regex fragments into a single anchored alternation. Keeping the +// fragments as a list documents each language token on its own line. +func alt(parts ...string) *regexp.Regexp { + return regexp.MustCompile(strings.Join(parts, "|")) +} + +// reHardSleep matches fixed sleeps/waits used for synchronization. Each +// fragment is a distinct language idiom; numeric-argument guards (\d) keep the +// generic sleep( forms from matching alias-style calls. +var reHardSleep = alt( + `\btime\.[Ss]leep\s*\(`, // Go, Python (time.sleep) + `\basyncio\.sleep\s*\(`, // Python async + `\bThread\.[Ss]leep\s*\(`, // Java / Kotlin / C# + `\bTask\.Delay\s*\(`, // C# + `\.waitForTimeout\s*\(`, // Playwright / Puppeteer + `\bcy\.wait\s*\(\s*\d`, // Cypress fixed numeric wait (not cy.wait('@alias')) + `\busleep\s*\(`, // PHP / C + `\bsleep\s*\(\s*\d`, // PHP / Ruby / Python (from-import) +) + +// reUnseededRandom matches unseeded random sources whose values differ run to +// run, making assertions non-deterministic. +var reUnseededRandom = alt( + `\bMath\.random\s*\(`, // JS / TS / Java + `\brandom\.(?:random|randint|randrange|choice|shuffle|uniform|sample)\s*\(`, // Python + `\brand\.(?:Intn|Int|Int31|Int63|Float32|Float64|Perm|Shuffle)\s*\(`, // Go math/rand global +) + +// rules is the registry, evaluated in order per added line. Additions are +// additive (ADR-0022 §3); keep severities conservative. +var rules = []rule{ + { + id: "hard-sleep", + pattern: reHardSleep, + severity: render.SeverityMedium, + titleKey: "flaky.hard_sleep.title", + descKey: "flaky.hard_sleep.description", + sugKey: "flaky.hard_sleep.suggestion", + }, + { + id: "unseeded-random", + pattern: reUnseededRandom, + severity: render.SeverityLow, + titleKey: "flaky.unseeded_random.title", + descKey: "flaky.unseeded_random.description", + sugKey: "flaky.unseeded_random.suggestion", + }, +} diff --git a/internal/i18n/messages.en.yml b/internal/i18n/messages.en.yml index 4604c02..1498ea1 100644 --- a/internal/i18n/messages.en.yml +++ b/internal/i18n/messages.en.yml @@ -200,3 +200,12 @@ summary.flag_conflict_review: "summary produces no findings; it can't be combine summary.no_changes: "No changes to summarize after filtering." summary.generating: "Writing summary…" summary.empty: "The provider returned an empty summary." + +# Deterministic flaky-test detector (ADR-0022). Finding text for the static +# internal/flaky pre-pass; localized like LLM findings (output language). +flaky.hard_sleep.title: "Test uses a hard-coded sleep for synchronization" +flaky.hard_sleep.description: "Fixed sleeps make a test timing-dependent: it passes on a fast machine and fails under CI load, producing flaky results that erode trust in the suite." +flaky.hard_sleep.suggestion: "Replace the fixed delay with a condition-based wait — poll until the expected state, or use your framework's Eventually/waitFor helper — so the test reacts to actual readiness instead of wall-clock time." +flaky.unseeded_random.title: "Test relies on unseeded randomness" +flaky.unseeded_random.description: "A random source without a fixed seed yields different values on every run, so assertions can pass or fail non-deterministically." +flaky.unseeded_random.suggestion: "Seed the generator with a constant in the test, or inject a deterministic fake, so the run is reproducible." diff --git a/internal/i18n/messages.tr.yml b/internal/i18n/messages.tr.yml index f5d3dc9..6a836d4 100644 --- a/internal/i18n/messages.tr.yml +++ b/internal/i18n/messages.tr.yml @@ -198,3 +198,12 @@ summary.flag_conflict_review: "summary bulgu üretmez; --suggest-commit, --fail- summary.no_changes: "Filtrelemeden sonra özetlenecek değişiklik yok." summary.generating: "Özet yazılıyor…" summary.empty: "Sağlayıcı boş bir özet döndürdü." + +# Deterministik flaky-test dedektörü (ADR-0022). Statik internal/flaky +# ön-süzgecinin bulgu metni; LLM bulguları gibi (çıktı dili) yerelleştirilir. +flaky.hard_sleep.title: "Test senkronizasyon için sabit bir bekleme (sleep) kullanıyor" +flaky.hard_sleep.description: "Sabit beklemeler testi zamana bağımlı kılar: hızlı makinede geçer, CI yükü altında başarısız olur; bu da süite olan güveni aşındıran kararsız (flaky) sonuçlar üretir." +flaky.hard_sleep.suggestion: "Sabit gecikmeyi koşula dayalı bir beklemeyle değiştir — beklenen duruma ulaşana kadar yokla ya da çerçevenin Eventually/waitFor yardımcısını kullan — ki test duvar saatine değil gerçek hazır oluşa tepki versin." +flaky.unseeded_random.title: "Test sabitlenmemiş (seed'siz) rastgeleliğe dayanıyor" +flaky.unseeded_random.description: "Sabit bir seed olmadan rastgele kaynak her çalıştırmada farklı değer üretir; bu yüzden doğrulamalar deterministik olmayan biçimde geçebilir ya da kalabilir." +flaky.unseeded_random.suggestion: "Üreteci testte sabit bir seed ile başlat ya da deterministik bir sahte üreteç enjekte et; böylece çalıştırma tekrar üretilebilir olur."