Files
J3vbandClaude Opus 5 d6c768cb90 feat(invariants): add server invariant rules and close five deadlock blind spots (#1383)
* feat(invariants): add the invariant-rule harness and the syncutil-locks rule

* fix(ws,service): route the last five raw mutexes through syncutil

The -tags deadlock CI pass only observes locks declared via syncutil, whose
Mutex/RWMutex are build-tag aliases. These five were declared as raw sync
types and were invisible to it, including the hub voice key-holder lock and
the permission and role caches.

TestServerInvariants now gates the tree against regressions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(invariants): walk the tree through os.Root to close a symlink TOCTOU

gosec G122: reading a filepath.WalkDir-supplied path is race-prone, since a
symlink swapped between the walk and the read escapes the intended tree.
os.Root confines every read to the root and cannot be traversed out of.

Walking the root's fs.FS also yields slash-separated paths already relative
to it, so the filepath.Rel and ToSlash conversion is no longer needed.

* fix(invariants): close syncutil-locks evasions, isolate per-rule tests, harden the gate

- I1: TestServerInvariants now asserts every registered Rule.Scope
  directory exists and holds at least one non-test .go file, so the
  gate cannot pass by scanning nothing.
- I2: split CheckSource into a thin wrapper over an unexported
  checkSourceWith(rules, ...), so TestSyncutilLocks tests the
  syncutil-locks rule in isolation instead of the whole registry.
- I3: broaden checkSyncutilLocks to a single SelectorExpr match (any
  sync.Mutex/sync.RWMutex reference bound via f.Imports, aliases
  included) instead of only *ast.Field/*ast.ValueSpec. Catches :=
  composite literals, untyped var specs, type aliases, and
  []sync.Mutex/map[K]sync.Mutex, none of which the old rule saw. A
  dot-import of "sync" is now its own violation, since it would
  otherwise let a bare Mutex evade the selector match entirely.
- M2: suppression now keys off the violation's own Rule id
  (allowed[v.Line][v.Rule]) rather than the running rule's ID, so a
  rule that ever emits a sub-id isn't silently unsuppressible.
- M3: Run sorts with sort.SliceStable, since an unreasoned allow
  comment and the violation it fails to suppress can share a
  file:line.
- M4/M5/M1-partial: add a build-tag-gated fixture test, document that
  allow comments must be same-line, and correct the skipDirs comment
  to describe both the generated-code and gitignored-runtime-dir
  cases it actually covers.

All ten original TestSyncutilLocks subtests pass unchanged; six new
subtests cover the evasions above.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(server): point at the syncutil-locks invariant gate

Server/CLAUDE.md told developers not to hand-roll around syncutil but
never said it's enforced. Note that Server/invariants/ checks it at
go test time and that exceptions are greppable via
grep -rn "invariant:allow" Server/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-16 17:05:06 +02:00

138 lines
3.9 KiB
Go

package invariants
import (
"go/token"
"io/fs"
"os"
"path/filepath"
"strings"
"testing"
)
// TestServerInvariants is the gate. It runs every registered rule over the
// real server tree and reports every violation at once.
func TestServerInvariants(t *testing.T) {
violations, err := Run("..")
if err != nil {
t.Fatalf("walking the server tree: %v", err)
}
for _, v := range violations {
t.Errorf("%s", v)
}
if len(violations) > 0 {
t.Logf("%d invariant violation(s). Each message names the fix; "+
"use //invariant:allow <rule> — <reason> only with a real reason.",
len(violations))
}
assertScopesCovered(t, "..")
}
// assertScopesCovered guards against the gate passing vacuously: zero
// violations is indistinguishable from zero files scanned (a moved package,
// or a Scope entry that no longer exists, would go green while enforcing
// nothing). It fails loudly, naming the offending Scope entry, if any
// registered Rule.Scope directory does not exist under root or holds no
// non-test .go file.
func assertScopesCovered(t *testing.T, root string) {
t.Helper()
for _, r := range Rules {
for _, scope := range r.Scope {
dir := filepath.Join(root, filepath.FromSlash(scope))
info, err := os.Stat(dir)
if err != nil || !info.IsDir() {
t.Fatalf("rule %q Scope entry %q does not resolve to a directory under %q: %v",
r.ID, scope, root, err)
continue
}
found := false
walkErr := filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
if !d.IsDir() && strings.HasSuffix(p, ".go") && !strings.HasSuffix(p, "_test.go") {
found = true
}
return nil
})
if walkErr != nil {
t.Fatalf("walking rule %q Scope entry %q: %v", r.ID, scope, walkErr)
}
if !found {
t.Fatalf("rule %q Scope entry %q contains no non-test .go file under %q; "+
"the gate would be enforcing nothing there", r.ID, scope, root)
}
}
}
}
// TestBuildTagGatedFilesAreStillChecked locks in the package doc's central
// anti-evasion guarantee: parser.ParseFile ignores build constraints, so a
// file gated behind e.g. -tags deadlock is checked exactly like any other --
// a raw mutex cannot be hidden from the rules by moving it behind a tag.
func TestBuildTagGatedFilesAreStillChecked(t *testing.T) {
src := `//go:build deadlock
package ws
import "sync"
type Hub struct{ mu sync.Mutex }
`
got := CheckSource(token.NewFileSet(), "ws/x.go", []byte(src))
if len(got) != 1 {
t.Fatalf("got %d violation(s), want 1: %v", len(got), got)
}
}
// TestRunExclusions builds a throwaway tree so the walker's exclusions are
// tested directly, rather than vacuously against a clean real tree.
func TestRunExclusions(t *testing.T) {
root := t.TempDir()
lock := []byte("package ws\nimport \"sync\"\ntype h struct{ mu sync.Mutex }\n")
write := func(rel string) {
full := filepath.Join(root, filepath.FromSlash(rel))
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(full, lock, 0o644); err != nil {
t.Fatal(err)
}
}
write("ws/real.go") // reported
write("ws/real_test.go") // excluded: _test.go
write("ws/testdata/x.go") // excluded: skipDirs
write("api/other.go") // excluded: out of scope
got, err := Run(root)
if err != nil {
t.Fatalf("Run: %v", err)
}
if len(got) != 1 {
t.Fatalf("got %d violation(s), want 1:\n%v", len(got), got)
}
if got[0].File != "ws/real.go" {
t.Errorf("File = %q, want %q", got[0].File, "ws/real.go")
}
}
func TestRuleScopeMatching(t *testing.T) {
r := Rule{ID: "x", Scope: []string{"ws", "service"}}
cases := map[string]bool{
"ws": true,
"ws/internal": true,
"service": true,
"api": false,
"": false,
"wsx": false,
}
for dir, want := range cases {
if got := r.inScope(dir); got != want {
t.Errorf("inScope(%q) = %v, want %v", dir, got, want)
}
}
}