From c53cce6b1cc464c007efa1259ce543c4ff4a4a63 Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Wed, 8 Jul 2026 08:14:21 -0500 Subject: [PATCH] fix(baseline): counts extension closes the duplicate-call ratchet hole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #35. The v1 fingerprint format has a known static-collision limitation (flaglint/spec's spec/fingerprint.md): two call sites sharing (callType, flagKey, file) share one fingerprint string. Before this, a brand-new *duplicate* of an already-baselined call was invisible to --fail-on-new — the fingerprint was already in the baseline's known set, so the tool reported "no new findings" even though a genuinely new call site had been added. Copy-paste is exactly how flag debt spreads, so this was a real gap. baseline.File gains an optional "counts" field: fingerprint -> occurrence count. Write always emits it (computed via the new CountFingerprints helper, from the same unduplicated per-usage fingerprint list it already received). Read parses it if present, leaving it nil if the field is missing or explicitly null — an older or hand-written baseline without "counts" still works, falling back to pure set semantics. baseline.Read's return type changes from a bare map[string]bool to a new Baseline struct (Fingerprints + Counts) to carry both pieces through to New. baseline.New's signature changes to take the current scan's own per-fingerprint counts (via the same CountFingerprints helper) instead of a flat fingerprint slice: a fingerprint now counts as "new" if it's either absent from the known set (existing behavior) or its current count exceeds its baselined one (new). A fingerprint present in the set but absent from Counts is treated as baselined at count 0, per spec — only reachable for a malformed/hand-edited baseline, since Write always emits a complete, consistent counts object. Also bundles in "schemaVersion" (a small, spec-required, additive field): "scan-result.v1" on the scan JSON envelope (reporter/json.go, alongside the existing generatedAt field) and "baseline.v1" on baseline.File — both additive, tolerated-if-absent per spec, so no version bump of our own is needed to start emitting them now. Verified: full test suite green, including a new CLI end-to-end test reproducing the exact ratchet-hole scenario (write baseline from a single call, add a second identically-shaped call to the same file so it shares the first's fingerprint, confirm --fail-on-new now correctly exits 1 where it previously exited 0). Manually confirmed both the written baseline.json and scan JSON output's shape/field values via the built binary. Signed-off-by: Krishan Kant Sharma --- internal/baseline/baseline.go | 135 +++++++++++++++++++++++------ internal/baseline/baseline_test.go | 133 ++++++++++++++++++++++++++-- internal/cli/cli_test.go | 48 ++++++++++ internal/cli/validate.go | 2 +- internal/reporter/json.go | 21 +++-- internal/reporter/reporter_test.go | 3 + 6 files changed, 302 insertions(+), 40 deletions(-) diff --git a/internal/baseline/baseline.go b/internal/baseline/baseline.go index 40c897b..11b0b1b 100644 --- a/internal/baseline/baseline.go +++ b/internal/baseline/baseline.go @@ -18,15 +18,31 @@ import ( // "1", not the number 1 — flaglint-js's own ADR 008 originally documented // this as a number and had to be corrected to match the shipped string // implementation; flaglint-go starts from the corrected, actual contract. +// +// Counts is the optional v1-additive multiset extension (spec/ +// fingerprint.md in flaglint/spec): fingerprint -> occurrence count. +// Mitigates the v1 fingerprint's known static-collision limitation (two +// call sites sharing (callType, flagKey, file) share one fingerprint +// string), which otherwise lets a brand-new *duplicate* of an +// already-baselined call slip past --fail-on-new undetected — writers +// SHOULD emit it (Write always does); readers MUST accept a baseline +// without it, falling back to pure set semantics (Read/New both do). type File struct { - Version string `json:"version"` - CreatedAt string `json:"createdAt"` - FlaglintVersion string `json:"flaglintVersion"` - Fingerprints []string `json:"fingerprints"` + Version string `json:"version"` + SchemaVersion string `json:"schemaVersion"` + CreatedAt string `json:"createdAt"` + FlaglintVersion string `json:"flaglintVersion"` + Fingerprints []string `json:"fingerprints"` + Counts map[string]int `json:"counts,omitempty"` } const currentVersion = "1" +// schemaVersion is the const value flaglint/spec's baseline.v1.schema.json +// expects for the "schemaVersion" field — a separate, additive field from +// "version" (the pre-existing "1" string), not a replacement for it. +const schemaVersion = "baseline.v1" + // Error carries the exit code a baseline failure should terminate the // process with — always 2 (invalid input) per the exit-code contract; // the type exists so callers can distinguish a baseline-specific failure @@ -41,27 +57,40 @@ func errf(format string, args ...any) *Error { return &Error{Message: fmt.Sprintf(format, args...)} } -// Read loads and validates a baseline file, returning the set of known -// fingerprints. Returns *Error for a missing file, invalid JSON, wrong -// version, or a missing/malformed fingerprints array — the caller should -// treat this as exit code 2. -func Read(path string) (map[string]bool, error) { +// Baseline is a parsed baseline file's contents, as needed for --fail- +// on-new comparison (New). Counts is nil when the file has no "counts" +// object at all (a baseline written before this feature, or a hand- +// crafted one) — New falls back to pure set semantics in that case, +// per spec. +type Baseline struct { + Fingerprints map[string]bool + Counts map[string]int +} + +// Read loads and validates a baseline file, returning its known +// fingerprint set and (if present) per-fingerprint counts. Returns +// *Error for a missing file, invalid JSON, wrong version, or a +// missing/malformed fingerprints array — the caller should treat this +// as exit code 2. An absent or malformed "counts" object is not an +// error: it's an optional, additive field, and its absence just means +// New falls back to set semantics. +func Read(path string) (Baseline, error) { raw, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { - return nil, errf("baseline file not found: %s", path) + return Baseline{}, errf("baseline file not found: %s", path) } - return nil, errf("failed to read baseline file %s: %v", path, err) + return Baseline{}, errf("failed to read baseline file %s: %v", path, err) } var obj map[string]json.RawMessage if err := json.Unmarshal(raw, &obj); err != nil { - return nil, errf("invalid JSON in baseline file: %s", path) + return Baseline{}, errf("invalid JSON in baseline file: %s", path) } var version string if err := json.Unmarshal(obj["version"], &version); err != nil || version != currentVersion { - return nil, errf(`unsupported baseline version in %s. Expected version "1"`, path) + return Baseline{}, errf(`unsupported baseline version in %s. Expected version "1"`, path) } // A literal JSON `null` for fingerprints must be rejected the same as a @@ -74,38 +103,63 @@ func Read(path string) (map[string]bool, error) { // so its equivalent check rejects null for free. rawFingerprints, ok := obj["fingerprints"] if !ok || strings.TrimSpace(string(rawFingerprints)) == "null" { - return nil, errf("baseline file is missing a valid 'fingerprints' array: %s", path) + return Baseline{}, errf("baseline file is missing a valid 'fingerprints' array: %s", path) } var fingerprints []string if json.Unmarshal(rawFingerprints, &fingerprints) != nil { - return nil, errf("baseline file is missing a valid 'fingerprints' array: %s", path) + return Baseline{}, errf("baseline file is missing a valid 'fingerprints' array: %s", path) } set := make(map[string]bool, len(fingerprints)) for _, fp := range fingerprints { set[fp] = true } - return set, nil + + // "counts" is optional and additive — a missing or explicitly-null + // object is simply absent (nil), not an error; only a *present but + // malformed* one (wrong JSON shape) is worth surfacing, since that + // suggests real corruption rather than an older/hand-written file. + var counts map[string]int + if rawCounts, ok := obj["counts"]; ok && strings.TrimSpace(string(rawCounts)) != "null" { + if err := json.Unmarshal(rawCounts, &counts); err != nil { + return Baseline{}, errf("baseline file has a malformed 'counts' object: %s", path) + } + } + + return Baseline{Fingerprints: set, Counts: counts}, nil +} + +// CountFingerprints returns how many times each fingerprint appears in +// fingerprints — the shared shape both Write (baselining today's counts) +// and a --fail-on-new caller (computing the current scan's own counts to +// compare against a baseline's) need. +func CountFingerprints(fingerprints []string) map[string]int { + counts := make(map[string]int, len(fingerprints)) + for _, fp := range fingerprints { + counts[fp]++ + } + return counts } // Write deduplicates and sorts fingerprints and writes them to path, +// along with each fingerprint's occurrence count (see File.Counts), // creating parent directories as needed. func Write(path string, fingerprints []string, flaglintVersion string) error { - seen := map[string]bool{} - deduped := make([]string, 0, len(fingerprints)) - for _, fp := range fingerprints { - if !seen[fp] { - seen[fp] = true - deduped = append(deduped, fp) - } + counts := CountFingerprints(fingerprints) + + deduped := make([]string, 0, len(counts)) + for fp := range counts { + deduped = append(deduped, fp) } sort.Strings(deduped) file := File{ Version: currentVersion, + SchemaVersion: schemaVersion, CreatedAt: time.Now().UTC().Format(time.RFC3339), FlaglintVersion: flaglintVersion, Fingerprints: deduped, + Counts: counts, } content, err := json.MarshalIndent(file, "", " ") @@ -122,13 +176,40 @@ func Write(path string, fingerprints []string, flaglintVersion string) error { return nil } -// New returns the fingerprints in current that are not present in known. -func New(current []string, known map[string]bool) []string { +// New returns every fingerprint that represents a genuinely new finding +// beyond known: either the fingerprint is entirely absent from known's +// set (existing, pre-counts set semantics), or — when known carries the +// optional counts extension — the current scan's occurrence count for +// that fingerprint exceeds the baselined one. The latter is what closes +// the "ratchet hole" a bare fingerprint set can't see on its own: a +// brand-new *duplicate* of an already-baselined call shares that call's +// fingerprint (the v1 format's known static-collision limitation — see +// spec/fingerprint.md) and would otherwise silently pass --fail-on-new. +// +// currentCounts is the current scan's own per-fingerprint occurrence +// count (CountFingerprints over every usage's fingerprint, unfiltered by +// known). A fingerprint present in known.Fingerprints but absent from +// known.Counts (only possible for a malformed/hand-edited baseline, since +// Write always emits a complete, consistent counts object) is treated as +// a baselined count of 0, per spec — the conservative, "don't silently +// pass debt through" fallback. When known.Counts is nil altogether (a +// baseline written before this feature, or a hand-crafted one), this +// falls back to pure set semantics — matching the spec's "readers MUST +// accept baselines without it" requirement. +func New(currentCounts map[string]int, known Baseline) []string { newFindings := []string{} - for _, fp := range current { - if !known[fp] { + for fp, count := range currentCounts { + if !known.Fingerprints[fp] { + newFindings = append(newFindings, fp) + continue + } + if known.Counts == nil { + continue // no counts extension in play — pure set semantics + } + if count > known.Counts[fp] { // absent from Counts -> 0, per spec newFindings = append(newFindings, fp) } } + sort.Strings(newFindings) return newFindings } diff --git a/internal/baseline/baseline_test.go b/internal/baseline/baseline_test.go index 043618f..942a9fc 100644 --- a/internal/baseline/baseline_test.go +++ b/internal/baseline/baseline_test.go @@ -15,12 +15,21 @@ func TestWriteRead_roundTrip(t *testing.T) { t.Fatalf("Write() error = %v", err) } - set, err := Read(path) + got, err := Read(path) if err != nil { t.Fatalf("Read() error = %v", err) } - if len(set) != 3 || !set["a"] || !set["b"] || !set["c"] { - t.Errorf("Read() = %v, want {a,b,c}", set) + if len(got.Fingerprints) != 3 || !got.Fingerprints["a"] || !got.Fingerprints["b"] || !got.Fingerprints["c"] { + t.Errorf("Read().Fingerprints = %v, want {a,b,c}", got.Fingerprints) + } + want := map[string]int{"a": 1, "b": 2, "c": 1} + if len(got.Counts) != len(want) { + t.Fatalf("Read().Counts = %v, want %v", got.Counts, want) + } + for k, v := range want { + if got.Counts[k] != v { + t.Errorf("Read().Counts[%q] = %d, want %d", k, got.Counts[k], v) + } } } @@ -50,6 +59,9 @@ func TestWrite_dedupedAndSorted(t *testing.T) { if f.Version != "1" { t.Errorf("Version = %q, want string \"1\"", f.Version) } + if f.SchemaVersion != "baseline.v1" { + t.Errorf("SchemaVersion = %q, want baseline.v1 (flaglint/spec's baseline.v1.schema.json)", f.SchemaVersion) + } } func TestWrite_versionIsJSONString(t *testing.T) { @@ -76,6 +88,31 @@ func TestWrite_createsParentDirectories(t *testing.T) { } } +func TestWrite_emitsCounts(t *testing.T) { + // A duplicate call (two occurrences sharing one v1 fingerprint) must + // be recorded as count 2, not silently collapsed to 1 the way the + // deduplicated fingerprints array itself is — this is the whole point + // of the counts extension (spec/fingerprint.md's "ratchet hole" fix). + path := filepath.Join(t.TempDir(), "baseline.json") + if err := Write(path, []string{"dup", "dup", "single"}, "1.0.0"); err != nil { + t.Fatalf("Write() error = %v", err) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var f File + if err := json.Unmarshal(raw, &f); err != nil { + t.Fatal(err) + } + if f.Counts["dup"] != 2 { + t.Errorf("Counts[dup] = %d, want 2", f.Counts["dup"]) + } + if f.Counts["single"] != 1 { + t.Errorf("Counts[single] = %d, want 1", f.Counts["single"]) + } +} + func TestRead_missingFile(t *testing.T) { _, err := Read(filepath.Join(t.TempDir(), "does-not-exist.json")) if err == nil { @@ -144,9 +181,40 @@ func TestRead_missingFingerprintsArray(t *testing.T) { } } +func TestRead_noCountsIsNilNotError(t *testing.T) { + // A baseline written before this feature (or hand-crafted) has no + // "counts" field at all — readers MUST accept it (spec), falling back + // to pure set semantics rather than erroring. + path := filepath.Join(t.TempDir(), "baseline.json") + if err := os.WriteFile(path, []byte(`{"version": "1", "fingerprints": ["a"]}`), 0o644); err != nil { + t.Fatal(err) + } + got, err := Read(path) + if err != nil { + t.Fatalf("Read() error = %v, want nil", err) + } + if got.Counts != nil { + t.Errorf("Counts = %v, want nil", got.Counts) + } + if !got.Fingerprints["a"] { + t.Error("Fingerprints[a] = false, want true") + } +} + +func TestRead_malformedCountsRejected(t *testing.T) { + path := filepath.Join(t.TempDir(), "baseline.json") + if err := os.WriteFile(path, []byte(`{"version": "1", "fingerprints": ["a"], "counts": "not-an-object"}`), 0o644); err != nil { + t.Fatal(err) + } + _, err := Read(path) + if err == nil { + t.Fatal("Read() error = nil, want error for malformed counts object") + } +} + func TestNew(t *testing.T) { - known := map[string]bool{"a": true, "b": true} - got := New([]string{"a", "b", "c", "d"}, known) + known := Baseline{Fingerprints: map[string]bool{"a": true, "b": true}} + got := New(CountFingerprints([]string{"a", "b", "c", "d"}), known) want := []string{"c", "d"} if len(got) != len(want) { t.Fatalf("New() = %v, want %v", got, want) @@ -159,7 +227,8 @@ func TestNew(t *testing.T) { } func TestNew_noneNewReturnsEmptyNotNil(t *testing.T) { - got := New([]string{"a"}, map[string]bool{"a": true}) + known := Baseline{Fingerprints: map[string]bool{"a": true}} + got := New(CountFingerprints([]string{"a"}), known) if got == nil { t.Error("New() = nil, want empty non-nil slice") } @@ -167,3 +236,55 @@ func TestNew_noneNewReturnsEmptyNotNil(t *testing.T) { t.Errorf("New() = %v, want empty", got) } } + +func TestNew_noCountsExtensionFallsBackToSetSemantics(t *testing.T) { + // known.Counts is nil (baseline predates this feature) — a duplicate + // occurrence of an already-known fingerprint must NOT be flagged as + // new; that's exactly what the counts extension exists to change, + // and its absence must mean the old, pre-feature behavior. + known := Baseline{Fingerprints: map[string]bool{"dup": true}} + got := New(CountFingerprints([]string{"dup", "dup"}), known) + if len(got) != 0 { + t.Errorf("New() = %v, want empty (no counts extension in play)", got) + } +} + +func TestNew_duplicateCallExceedingBaselinedCountIsNew(t *testing.T) { + // The "ratchet hole" fix itself: a fingerprint already in the baseline + // set, baselined at count 1, now appearing twice in the current scan — + // a genuinely new duplicate call site — must be flagged as new. + known := Baseline{ + Fingerprints: map[string]bool{"dup": true}, + Counts: map[string]int{"dup": 1}, + } + got := New(CountFingerprints([]string{"dup", "dup"}), known) + if len(got) != 1 || got[0] != "dup" { + t.Errorf("New() = %v, want [dup]", got) + } +} + +func TestNew_countNotExceedingBaselineIsNotNew(t *testing.T) { + known := Baseline{ + Fingerprints: map[string]bool{"dup": true}, + Counts: map[string]int{"dup": 2}, + } + got := New(CountFingerprints([]string{"dup", "dup"}), known) + if len(got) != 0 { + t.Errorf("New() = %v, want empty (current count does not exceed baselined count)", got) + } +} + +func TestNew_fingerprintAbsentFromCountsTreatedAsZero(t *testing.T) { + // A fingerprint present in the set but absent from counts (only + // possible for a malformed/hand-edited baseline, since Write always + // emits a complete, consistent counts object) must be treated as + // baselined at count 0, per spec — the conservative fallback. + known := Baseline{ + Fingerprints: map[string]bool{"a": true, "b": true}, + Counts: map[string]int{"a": 1}, // "b" deliberately absent + } + got := New(CountFingerprints([]string{"a", "b"}), known) + if len(got) != 1 || got[0] != "b" { + t.Errorf("New() = %v, want [b]", got) + } +} diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 81f1796..a570f5f 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -362,6 +362,54 @@ func RunMore() { } } +func TestCLI_baselineRatchetHole_duplicateCallExceedsBaselinedCount(t *testing.T) { + // The v1 fingerprint's known static-collision limitation + // (spec/fingerprint.md in flaglint/spec): two call sites sharing + // (callType, flagKey, file) share one fingerprint string. Before the + // baseline "counts" extension, a brand-new *duplicate* of an + // already-baselined call was invisible to --fail-on-new, since the + // fingerprint was already in the known set — copy-paste is exactly + // how flag debt spreads, so this was a real ratchet hole, not a + // theoretical one. This test proves it's closed. + dir := t.TempDir() + writeGoFile(t, filepath.Join(dir, "flags.go"), sampleService) + baselinePath := filepath.Join(dir, "baseline.json") + + if err := exec.Command(binPath, "audit", dir, "--write-baseline", baselinePath).Run(); err != nil { + t.Fatalf("audit --write-baseline failed: %v", err) + } + + // A second, identically-shaped call in the SAME file — same callType, + // flagKey, and file, so it shares checkout-v2's exact v1 fingerprint + // with the one already baselined at count 1. + writeGoFile(t, filepath.Join(dir, "flags.go"), `package svc + +import ( + "time" + + ld "github.com/launchdarkly/go-server-sdk/v7" +) + +func Run() { + client, _ := ld.MakeClient("sdk-key", 5*time.Second) + _, _ = client.BoolVariation("checkout-v2", nil, false) + _, _ = client.BoolVariation("checkout-v2", nil, false) +} +`) + + cmd := exec.Command(binPath, "validate", dir, "--baseline", baselinePath, "--fail-on-new") + var stderr strings.Builder + cmd.Stderr = &stderr + err := cmd.Run() + exitErr, ok := err.(*exec.ExitError) + if !ok { + t.Fatalf("expected *exec.ExitError for a new duplicate call exceeding its baselined count, got %v (stderr: %s)", err, stderr.String()) + } + if exitErr.ExitCode() != 1 { + t.Errorf("exit code = %d, want 1 (stderr: %s)", exitErr.ExitCode(), stderr.String()) + } +} + func TestCLI_validate_malformedBaselineExits2(t *testing.T) { dir := t.TempDir() writeGoFile(t, filepath.Join(dir, "flags.go"), sampleService) diff --git a/internal/cli/validate.go b/internal/cli/validate.go index 097b829..c8a3067 100644 --- a/internal/cli/validate.go +++ b/internal/cli/validate.go @@ -114,7 +114,7 @@ func checkBaseline(cmd *cobra.Command, result types.ScanResult, baselinePath str fingerprints = append(fingerprints, u.Fingerprint) } } - newFindings := baseline.New(fingerprints, known) + newFindings := baseline.New(baseline.CountFingerprints(fingerprints), known) if !failOnNew { if len(newFindings) > 0 { diff --git a/internal/reporter/json.go b/internal/reporter/json.go index d39d6a9..1b70c98 100644 --- a/internal/reporter/json.go +++ b/internal/reporter/json.go @@ -6,17 +6,26 @@ import ( "github.com/flaglint/flaglint-go/internal/types" ) -// jsonEnvelope wraps ScanResult with a top-level generatedAt field, matching -// flaglint-js's `{ generatedAt: result.scannedAt, ...result }` spread — -// types.ScanResult is embedded without its own JSON tag, so encoding/json -// inlines its fields at the top level alongside GeneratedAt. +// scanResultSchemaVersion is the const value flaglint/spec's +// scan-result.v1.schema.json requires for the "schemaVersion" field — +// additive as of spec v1.1 (writers MUST emit, readers MUST tolerate its +// absence in documents produced before spec v1.1, which is exactly why +// this is safe to start emitting now without a version bump of our own). +const scanResultSchemaVersion = "scan-result.v1" + +// jsonEnvelope wraps ScanResult with top-level generatedAt and +// schemaVersion fields, matching flaglint-js's `{ generatedAt: +// result.scannedAt, ...result }` spread — types.ScanResult is embedded +// without its own JSON tag, so encoding/json inlines its fields at the +// top level alongside GeneratedAt/SchemaVersion. type jsonEnvelope struct { - GeneratedAt string `json:"generatedAt"` + GeneratedAt string `json:"generatedAt"` + SchemaVersion string `json:"schemaVersion"` types.ScanResult } func formatJSON(result types.ScanResult) string { - envelope := jsonEnvelope{GeneratedAt: result.ScannedAt, ScanResult: result} + envelope := jsonEnvelope{GeneratedAt: result.ScannedAt, SchemaVersion: scanResultSchemaVersion, ScanResult: result} // Marshal cannot fail here: every field of ScanResult is a plain string, // int, bool, or slice/struct thereof — no channels, funcs, or cyclic // pointers that could produce an error. diff --git a/internal/reporter/reporter_test.go b/internal/reporter/reporter_test.go index 709b792..2a64b7c 100644 --- a/internal/reporter/reporter_test.go +++ b/internal/reporter/reporter_test.go @@ -38,6 +38,9 @@ func TestRender_json(t *testing.T) { if decoded["generatedAt"] != "2026-07-06T00:00:00Z" { t.Errorf("generatedAt = %v, want scannedAt value", decoded["generatedAt"]) } + if decoded["schemaVersion"] != "scan-result.v1" { + t.Errorf("schemaVersion = %v, want scan-result.v1 (flaglint/spec's scan-result.v1.schema.json)", decoded["schemaVersion"]) + } if decoded["scannedAt"] != "2026-07-06T00:00:00Z" { t.Errorf("scannedAt = %v, want present at top level (spread, not nested)", decoded["scannedAt"]) }