From 05f508dd6f44997edeec8e7a903423904edb5241 Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Thu, 24 Sep 2026 15:31:21 +0200 Subject: [PATCH 1/2] fix(dirty-set): correct lockfile detection in the fallback trigger isFallbackTrigger matched lockfiles by substring on the basename, which erred in both directions. It missed real lockfiles that carry ".lock" as an inner component, such as multitool.lock.json and .terraform.lock.hcl. That is the dangerous direction: the file belongs to no target's edges, so nothing is marked dirty, every target keeps its seed hash, and impacted targets are silently under-reported. In one monorepo tools/multitool.lock.json changed 33 times in the last 90 days, each time without a fallback. It also caught any file whose name merely contains "-lock." or "_lock." regardless of kind. A git merge driver (merge-pnpm-lock.sh), a PNG, an XML, three SQL migrations and three markdown documents each forced a full rehash of ~444k targets. A ".lock" extension now always triggers. The ambiguous inner-component and name-stem forms additionally require an extension a lockfile could plausibly use; an unrecognised extension stays conservative and still triggers. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/dirty_set.go | 34 +++++++++++++++++-- pkg/dirty_set_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/pkg/dirty_set.go b/pkg/dirty_set.go index e545f3a..e76753b 100644 --- a/pkg/dirty_set.go +++ b/pkg/dirty_set.go @@ -301,14 +301,42 @@ func propagateFrom(dirtyLabels, actuallyChanged map[string]bool, edges map[strin return result } +// nonLockfileExtensions are extensions a dependency lockfile never uses. +// Lockfile detection matches on name fragments, so without this a script, +// document or migration merely named after a lock forces a full rehash. +var nonLockfileExtensions = map[string]bool{ + ".md": true, ".rst": true, ".txt": true, + ".png": true, ".jpg": true, ".jpeg": true, ".svg": true, + ".sh": true, ".bash": true, + ".go": true, ".java": true, ".py": true, ".ts": true, ".js": true, + ".sql": true, ".xml": true, ".html": true, +} + +// looksLikeLockfile reports whether lowerBasename, already lowercased, names a +// dependency lockfile. Three shapes occur: a ".lock" extension (Cargo.lock, +// buf.lock), ".lock" as an inner component (multitool.lock.json, +// .terraform.lock.hcl), and a "-lock" or "_lock" name stem (pnpm-lock.yaml, +// package-lock.json). Only the first is unambiguous; the other two also match +// ordinary files that happen to be named after a lock, so they additionally +// require an extension a lockfile could plausibly use. +func looksLikeLockfile(lowerBasename string) bool { + if strings.HasSuffix(lowerBasename, ".lock") { + return true + } + if !strings.Contains(lowerBasename, ".lock.") && + !strings.Contains(lowerBasename, "-lock.") && + !strings.Contains(lowerBasename, "_lock.") { + return false + } + return !nonLockfileExtensions[filepath.Ext(lowerBasename)] +} + func isFallbackTrigger(basename string) bool { if strings.HasSuffix(basename, ".bzl") { return true } lowerBasename := strings.ToLower(basename) - if strings.HasSuffix(lowerBasename, ".lock") || - strings.Contains(lowerBasename, "-lock.") || - strings.Contains(lowerBasename, "_lock.") { + if looksLikeLockfile(lowerBasename) { return true } if basename == ".bazelrc" || strings.HasPrefix(basename, ".bazelrc.") || diff --git a/pkg/dirty_set_test.go b/pkg/dirty_set_test.go index 6d14df3..222a4d9 100644 --- a/pkg/dirty_set_test.go +++ b/pkg/dirty_set_test.go @@ -724,3 +724,80 @@ func TestPruneDirtySetReadsDependencyHashes(t *testing.T) { t.Error("//app:consumer should be pruned: //pkg:tool is unchanged per DependencyHashes") } } + +func TestIsFallbackTriggerLockfiles(t *testing.T) { + tests := []struct { + basename string + want bool + }{ + // Plain ".lock" extension. + {"Cargo.lock", true}, + {"yarn.lock", true}, + {"poetry.lock", true}, + {"uv.lock", true}, + {"buf.lock", true}, + {"MODULE.bazel.lock", true}, + + // ".lock" as an inner component. + {"multitool.lock.json", true}, + {".terraform.lock.hcl", true}, + + // "-lock" or "_lock" name stem. + {"pnpm-lock.yaml", true}, + {"package-lock.json", true}, + + // An unrecognised extension stays conservative and still triggers. + {"deps-lock.xyz", true}, + + // Ordinary files merely named after a lock must not trigger. + {"merge-pnpm-lock.sh", false}, + {"regen-pnpm-lock.sh", false}, + {"repin-with-lock.sh", false}, + {"2x-lock.png", false}, + {"db.carbon.leader-lock.xml", false}, + {"02_add_and_lock.md", false}, + {"v3__workflow_lock.sql", false}, + + // Nothing lock-like at all. + {"README.md", false}, + {"Main.java", false}, + } + + for _, tt := range tests { + if got := isFallbackTrigger(tt.basename); got != tt.want { + t.Errorf("isFallbackTrigger(%q) = %v, want %v", tt.basename, got, tt.want) + } + } +} + +func TestComputeDirtySetNestedLockExtensionFallsBack(t *testing.T) { + changedFiles := map[string]string{ + "tools/multitool.lock.json": "M", + } + + result := ComputeDirtySet(changedFiles, nil, nil, nil) + + if !result.NeedsFallback { + t.Fatal("expected fallback for multitool.lock.json change") + } + if result.FallbackCode != "unsafe_file_change" { + t.Errorf("FallbackCode = %q, want unsafe_file_change", result.FallbackCode) + } +} + +func TestComputeDirtySetLockNamedScriptDoesNotFallBack(t *testing.T) { + edges := map[string][]string{ + "//pkg:rule_a": {}, + } + allLabels := CollectAllLabels(edges, nil) + + changedFiles := map[string]string{ + "tools/git/merge-pnpm-lock.sh": "M", + } + + result := ComputeDirtySet(changedFiles, edges, allLabels, nil) + + if result.NeedsFallback { + t.Errorf("unexpected fallback for a shell script: %s", result.FallbackReason) + } +} From 1a08997be6c15c2c5fd673dac6e49d4a5e5481d6 Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Thu, 24 Sep 2026 15:42:58 +0200 Subject: [PATCH 2/2] fix(dirty-set): keep .txt and .xml out of the non-lockfile denylist pip writes requirements_lock.txt, so excluding ".txt" made a real lockfile stop triggering a fallback -- the exact under-reporting this change set out to remove. TestComputeDirtySetRepositoryMetadataFallbacks already covered it and caught the regression. Drop ".txt" and ".xml", and record the rule in a comment: the denylist holds executable code, images and prose only. An extension that could carry structured dependency data stays off it, because a wrong entry silently under-reports impacted targets while a missing one costs only a redundant rehash. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/dirty_set.go | 11 +++++++++-- pkg/dirty_set_test.go | 6 ++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/dirty_set.go b/pkg/dirty_set.go index e76753b..e5bd862 100644 --- a/pkg/dirty_set.go +++ b/pkg/dirty_set.go @@ -304,12 +304,19 @@ func propagateFrom(dirtyLabels, actuallyChanged map[string]bool, edges map[strin // nonLockfileExtensions are extensions a dependency lockfile never uses. // Lockfile detection matches on name fragments, so without this a script, // document or migration merely named after a lock forces a full rehash. +// +// Keep this list to executable code, images and prose. Anything that could +// hold structured dependency data must stay off it, however unlikely it +// looks: pip writes requirements_lock.txt, so ".txt" belongs to lockfiles, +// and ".xml" is omitted for the same reason. Wrongly listing an extension +// here silently under-reports impacted targets, which is far worse than the +// redundant rehash a missing entry causes. var nonLockfileExtensions = map[string]bool{ - ".md": true, ".rst": true, ".txt": true, + ".md": true, ".rst": true, ".html": true, ".png": true, ".jpg": true, ".jpeg": true, ".svg": true, ".sh": true, ".bash": true, ".go": true, ".java": true, ".py": true, ".ts": true, ".js": true, - ".sql": true, ".xml": true, ".html": true, + ".sql": true, } // looksLikeLockfile reports whether lowerBasename, already lowercased, names a diff --git a/pkg/dirty_set_test.go b/pkg/dirty_set_test.go index 222a4d9..d9cda5d 100644 --- a/pkg/dirty_set_test.go +++ b/pkg/dirty_set_test.go @@ -745,8 +745,11 @@ func TestIsFallbackTriggerLockfiles(t *testing.T) { // "-lock" or "_lock" name stem. {"pnpm-lock.yaml", true}, {"package-lock.json", true}, + {"requirements_lock.txt", true}, - // An unrecognised extension stays conservative and still triggers. + // Extensions that could hold dependency data stay conservative, as + // does an unrecognised one. + {"deps-lock.xml", true}, {"deps-lock.xyz", true}, // Ordinary files merely named after a lock must not trigger. @@ -754,7 +757,6 @@ func TestIsFallbackTriggerLockfiles(t *testing.T) { {"regen-pnpm-lock.sh", false}, {"repin-with-lock.sh", false}, {"2x-lock.png", false}, - {"db.carbon.leader-lock.xml", false}, {"02_add_and_lock.md", false}, {"v3__workflow_lock.sql", false},