From 1ad43d5cefb93192d1fe4a78a0cb9e85bf274160 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Nikolaus?= Date: Mon, 29 Jun 2026 04:56:11 +0000 Subject: [PATCH] feat(lint): reject `len()` in `when` patterns (E0508) `len()` inside `when` computes over the pattern's materialised value (e.g. `[]` for open lists), not the input's, so the result is always a fixed constant. Reject at load time with E0508 and guide users toward `list.MatchN` instead. - Add E0508 diagnostic code with help text - Reject `len` CallExpr in lintWhen walker - Remove `len` from permittedUniverseBuiltins - Remove now-unreachable evaluator test for len-inert-under-subsumption - Add unit test, fixture rule, and scrut integration test - Update AGENTS.md --- AGENTS.md | 15 ++++--- internal/config/lint.go | 28 ++++++++++-- internal/config/lint_diag_test.go | 45 ++++++++++++++++--- internal/diag/codes.go | 15 ++++++- internal/diag/codes_e0504_test.go | 4 +- internal/diag/codes_e0505_test.go | 4 +- internal/diag/codes_test.go | 3 +- internal/evaluator/evaluator_test.go | 45 ------------------- tests/diagnostics.md | 37 +++++++++++++++ .../len_in_when.cue | 18 ++++++++ 10 files changed, 147 insertions(+), 67 deletions(-) create mode 100644 tests/diagnostics_rules_broken_len/len_in_when.cue diff --git a/AGENTS.md b/AGENTS.md index e92a0b0..512fe65 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,10 +83,8 @@ and unify at schema time, but at match time they do not reflect input content: conditionally based on the input. - **Computed hidden count fields.** `_n: len(flags)` with `_n: >=2` — `flags: [...string]` materialises as `[]` at pattern level, so `_n` is `0` - regardless of input. Bare `len` lint-passes, but `len` over an input-derived - field stays inert this way: the gate is fixed at `len([]) == 0` and cannot - react to the input length, so a count constraint either matches every input - (`_n & 0`) or fails to load as a static conflict (`_n & >=1`). + regardless of input. **Rejected at load time (E0508).** Use `list.MatchN` + instead: `flags: list.MatchN(>=2, string)`. - **`close` over a `when` struct.** `when: close({tool_name: "Bash"})` closes an open struct pattern, so on extensible hook payloads the closed pattern never subsumes the input and the rule silently never matches. `close` is excluded @@ -112,12 +110,15 @@ Express these via: - **Unbound identifiers.** Any ident that resolves to none of a stdlib import binding, a locally-visible hidden sibling (`_foo`), a curated universe builtin, or a bare sibling top-level rule struct. +- **`len()` calls inside `when`.** `len` computes over the pattern's + materialised value (`[]` for open lists), not the input's. Use `list.MatchN` + instead. E0508. Imports, predeclared names (`string`, `int`, `number`, …), hidden local helpers, and bare references to sibling top-level rule structs all pass. The -curated universe builtins `and`, `or`, `matchN`, `matchIf`, and `len` also pass -bare in `when`. `close` and the arithmetic helpers (`div`, `mod`, `quo`, `rem`) -stay rejected. +curated universe builtins `and`, `or`, `matchN`, and `matchIf` also pass +bare in `when`. `close`, `len`, and the arithmetic helpers (`div`, `mod`, +`quo`, `rem`) stay rejected. ### Organizing rules into packages diff --git a/internal/config/lint.go b/internal/config/lint.go index 4923b43..7b95315 100644 --- a/internal/config/lint.go +++ b/internal/config/lint.go @@ -205,6 +205,10 @@ func lintWhen(ruleName string, ruleNames, helperDefNames map[string]struct{}, wh walk(node.High) return case *ast.CallExpr: + if id, ok := node.Fun.(*ast.Ident); ok && id.Name == "len" { + firstErr = lenInWhenDiag(ruleName, node) + return + } walk(node.Fun) for _, arg := range node.Args { walk(arg) @@ -292,14 +296,15 @@ func checkSelector(ruleName string, ruleNames, helperDefNames map[string]struct{ // The parser never binds universe builtins to an ast.Node and IsPredeclared() // recognises only type/range names, so absent this set they false-positive as // E0501. close is excluded because it closes an open struct pattern, silently -// breaking matches on extensible hook payloads; div/mod/quo/rem/error/self are -// excluded as arithmetic/inert helpers with no pattern meaning in `when`. +// breaking matches on extensible hook payloads; len is caught at the CallExpr +// level (E0508) since it computes over the pattern's materialised value, not +// the input's; div/mod/quo/rem/error/self are excluded as arithmetic/inert +// helpers with no pattern meaning in `when`. var permittedUniverseBuiltins = map[string]struct{}{ "and": {}, "or": {}, "matchN": {}, "matchIf": {}, - "len": {}, } // checkIdent classifies a bare identifier reference. Returns a *diag.DiagError @@ -417,6 +422,23 @@ func unboundDiag(ruleName string, id *ast.Ident, ruleNames, helperDefNames map[s return diag.NewDiagError(d, nil, nil) } +// lenInWhenDiag builds an E0508 DiagError for a `len()` call inside `when`. +func lenInWhenDiag(ruleName string, call *ast.CallExpr) error { + d := diag.Diagnostic{ + Code: diag.E0508.Code, + Severity: diag.SeverityError, + Title: "rule " + quote(ruleName) + + ": `len` in `when` computes over the pattern, not the input", + Primary: diag.Label{ + Pos: call.Pos(), + Len: 3, // "len" + Msg: "`len` in `when` of rule " + quote(ruleName), + }, + Help: diag.E0508.Help, + } + return diag.NewDiagError(d, nil, nil) +} + // quote wraps s in double quotes without escaping; identifier and rule names // never contain quote-sensitive characters, so strconv.Quote would only add // noise to the rendered diagnostic. diff --git a/internal/config/lint_diag_test.go b/internal/config/lint_diag_test.go index 299f564..5b636f4 100644 --- a/internal/config/lint_diag_test.go +++ b/internal/config/lint_diag_test.go @@ -292,7 +292,7 @@ clean_rule: { } // TestLoadRules_LintDiag_PermittedUniverseBuiltins_NoE05xx pins that the -// curated universe builtins (and, or, matchN, matchIf, len) may be used bare in +// curated universe builtins (and, or, matchN, matchIf) may be used bare in // `when` without tripping E0501. Each fixture exercises the builtin's real // validator arity in the srnnkls/cue fork. func TestLoadRules_LintDiag_PermittedUniverseBuiltins_NoE05xx(t *testing.T) { @@ -316,10 +316,6 @@ func TestLoadRules_LintDiag_PermittedUniverseBuiltins_NoE05xx(t *testing.T) { name: "matchIf_conditional", when: `when: {tool_name: matchIf({}, {}, {})}`, }, - { - name: "len_over_concrete_list", - when: `when: {tool_name: "Bash", _n: len(["a", "b"]) & 2}`, - }, } for _, tc := range cases { @@ -497,3 +493,42 @@ typo_rule: { t.Errorf("expected E0501 among recovered diagnostics; got codes %v", codes) } } + +// TestLoadRules_LintDiag_LenInWhen_EmitsE0508 pins that a `len()` call inside +// `when` surfaces as E0508 with the primary span anchored at `len`. +func TestLoadRules_LintDiag_LenInWhen_EmitsE0508(t *testing.T) { + const src = `package rules + +len_rule: { + when: { + tool_input: parsed: { + flags: [...] + _n: len(flags) + _n: >=2 + } + } + then: deny: { + rule_id: "n" + reason: "nope" + } +} +` + path := lintFixture(t, "len_when", src) + dir := filepath.Dir(path) + + _, err := config.LoadRules(dir) + if err == nil { + t.Fatal("expected len in when to be rejected, got nil error") + } + + de, ok := recoverDiag(t, err) + if !ok { + t.Fatalf("expected err to carry *diag.DiagError via errors.As; got: %v", err) + } + if de.D.Code != "E0508" { + t.Errorf("diagnostic Code = %q, want %q", de.D.Code, "E0508") + } + if got := tokenAtPos(t, de.D.Primary.Pos, 3); got != "len" { + t.Errorf("primary span should anchor at `len` keyword; got %q", got) + } +} diff --git a/internal/diag/codes.go b/internal/diag/codes.go index c15d5c8..b83efbb 100644 --- a/internal/diag/codes.go +++ b/internal/diag/codes.go @@ -194,9 +194,20 @@ different packages the merge is ambiguous; rename them to one shared package, or split the divergent files into their own directory.`, } +var E0508 = CodeInfo{ + Code: "E0508", + Help: "`len` inside `" + `when` + "` computes over the pattern's materialised value, not the input's." + ` + +` + "`_n: len(flags)`" + ` where ` + "`flags: [...string]`" + ` always yields ` + "`0`" + ` because the +pattern materialises the open list as ` + "`[]`" + `. A downstream constraint like +` + "`_n: >=2`" + ` either conflicts statically or is vacuously true — either way, +it cannot react to the input's actual list length. Use ` + "`list.MatchN`" + ` +instead: ` + "`flags: list.MatchN(>=2, string)`" + `.`, +} + // CodesInScopeV1 freezes the code count for this scope; bumping it requires // a deliberate design review to justify adding a new code. -const CodesInScopeV1 = 17 +const CodesInScopeV1 = 18 // codeRegistry maps each stable code string to its CodeInfo. // Built at package init so that duplicate codes fail loudly rather @@ -206,7 +217,7 @@ var codeRegistry = buildCodeRegistry( E0201, E0202, E0203, E0301, E0302, E0303, E0304, E0401, E0402, - E0501, E0502, E0503, E0504, E0505, + E0501, E0502, E0503, E0504, E0505, E0508, ) func buildCodeRegistry(entries ...CodeInfo) map[string]CodeInfo { diff --git a/internal/diag/codes_e0504_test.go b/internal/diag/codes_e0504_test.go index 85ea2c9..1ad006a 100644 --- a/internal/diag/codes_e0504_test.go +++ b/internal/diag/codes_e0504_test.go @@ -21,7 +21,7 @@ func TestE0504_Registered(t *testing.T) { // TestE0504_BumpsCodesInScope pins that registering E0504 advances the frozen // in-scope code count from 16 to 17 (E0505 is already counted). func TestE0504_BumpsCodesInScope(t *testing.T) { - if CodesInScopeV1 != 17 { - t.Errorf("CodesInScopeV1 = %d, want 17 after E0504 joins the registry", CodesInScopeV1) + if CodesInScopeV1 != 18 { + t.Errorf("CodesInScopeV1 = %d, want 18 after E0504 joins the registry", CodesInScopeV1) } } diff --git a/internal/diag/codes_e0505_test.go b/internal/diag/codes_e0505_test.go index 2483920..a7b0cb9 100644 --- a/internal/diag/codes_e0505_test.go +++ b/internal/diag/codes_e0505_test.go @@ -22,7 +22,7 @@ func TestDiag_E0505_Registered(t *testing.T) { // TestDiag_CodesInScopeV1_IncludesE0505 pins the frozen code count once both // E0505 (CRP-001) and E0504 (CRP-005) are registered: 15 -> 17. func TestDiag_CodesInScopeV1_IncludesE0505(t *testing.T) { - if CodesInScopeV1 != 17 { - t.Errorf("CodesInScopeV1 = %d, want 17 (E0504 + E0505 registered)", CodesInScopeV1) + if CodesInScopeV1 != 18 { + t.Errorf("CodesInScopeV1 = %d, want 18 (E0504 + E0505 registered)", CodesInScopeV1) } } diff --git a/internal/diag/codes_test.go b/internal/diag/codes_test.go index 00a1297..a055c94 100644 --- a/internal/diag/codes_test.go +++ b/internal/diag/codes_test.go @@ -41,6 +41,7 @@ func expectedCodes() []expectedCode { {"E0503", "scope/binding", func() diag.CodeInfo { return diag.E0503 }}, {"E0504", "scope/binding", func() diag.CodeInfo { return diag.E0504 }}, {"E0505", "scope/binding", func() diag.CodeInfo { return diag.E0505 }}, + {"E0508", "scope/binding", func() diag.CodeInfo { return diag.E0508 }}, } } @@ -184,7 +185,7 @@ func TestCodeInfoZeroValue(t *testing.T) { // Asserts that this scope adds no new error codes. func TestNoNewCodesInScope(t *testing.T) { - const frozen = 17 + const frozen = 18 if got := diag.CodesInScopeV1; got != frozen { t.Errorf("diag.CodesInScopeV1 = %d, want %d", got, frozen) } diff --git a/internal/evaluator/evaluator_test.go b/internal/evaluator/evaluator_test.go index 6f8a1d7..d6d89c6 100644 --- a/internal/evaluator/evaluator_test.go +++ b/internal/evaluator/evaluator_test.go @@ -5,7 +5,6 @@ import ( "errors" "os" "path/filepath" - "slices" "strings" "testing" @@ -1155,50 +1154,6 @@ func TestEvaluate_MatchIfBuiltinInWhen_Matches(t *testing.T) { }) } -// `when` is checked by static subsumption, never evaluated against the input, -// so `len` over an input-derived field materialises as `len([]) == 0` before -// any payload is seen. The gate value is fixed at compile time and cannot react -// to the input list length — this is the intended trap, not a bug to "fix". -func TestEvaluate_LenOverInputDerivedField_InertUnderSubsumption(t *testing.T) { - dir := t.TempDir() - mustWriteRule(t, dir, "gate_zero.cue", `{ - when: { - tool_input: parsed: flags: [...] - _n: len(tool_input.parsed.flags) - _gate: _n & 0 - } - then: deny: {rule_id: "gate-zero", reason: "len gate & 0"} - }`) - rules := loadRules(t, dir) - - ctx := cuecontext.New() - - matchedIDs := func(t *testing.T, src string) []string { - t.Helper() - input := mustCompile(t, ctx, src) - got, _, err := evaluator.Evaluate(rules, input) - if err != nil { - t.Fatalf("Evaluate: %v", err) - } - ids := make([]string, 0, len(got)) - for _, m := range got { - if m.Action != nil { - ids = append(ids, m.Action.RuleID) - } - } - return ids - } - - empty := matchedIDs(t, `{tool_input: parsed: flags: []}`) - three := matchedIDs(t, `{tool_input: parsed: flags: ["-r", "-f", "-x"]}`) - if !slices.Contains(empty, "gate-zero") { - t.Fatalf("gate-zero should match empty flags, matched: %v", empty) - } - if !slices.Contains(three, "gate-zero") { - t.Fatalf("gate-zero should match 3-element flags identically (len stays len([])==0), matched: %v", three) - } -} - // ----------------------------------------------------------------------------- // Three-lane signature (T6) — matches, diagnostics, error lanes are orthogonal. // Engine-level failures flow through the error lane only; diagnostics stay nil. diff --git a/tests/diagnostics.md b/tests/diagnostics.md index 11b6b7f..e394c30 100644 --- a/tests/diagnostics.md +++ b/tests/diagnostics.md @@ -28,6 +28,7 @@ Fixture rules live under `tests/diagnostics_rules*/`: Provenance footer. - `tests/diagnostics_rules_broken_scope/` — load-time E0501. - `tests/diagnostics_rules_broken_cross/` — load-time E0502. +- `tests/diagnostics_rules_broken_len/` — load-time E0508 (`len()` in `when`). Each block redirects stderr into stdout (`2>&1`) so scrut — which only captures stdout by default — sees the diagnostic stream. The @@ -391,6 +392,42 @@ underscore) at the file top level where both rules can see it. [1] ``` +## E0508 — `len()` in `when` (load-time) + +`tests/diagnostics_rules_broken_len/len_in_when.cue` uses `len(flags)` inside +`when` to compute over an input-derived field. The pattern materialises +`flags: [...string]` as `[]`, so `len(flags)` is always `0` — the count +constraint is either a static conflict or vacuously true. The loader rejects +the directory with an E0508 diagnostic and the CLI exits 1. + +```scrut +$ cat << 'EOF' | +> { +> "hook_event_name": "PreToolUse", +> "tool_name": "Bash", +> "tool_input": {"command": "ls"}, +> "session_id": "test", +> "cwd": "/tmp" +> } +> EOF +> fas eval --harness claude --config tests/diagnostics_rules_broken_len --global-config /tmp/fas-nonexistent-global 2>&1 +error[E0508]: rule "len_rule": `len` in `when` computes over the pattern, not the input + --> tests/diagnostics_rules_broken_len/len_in_when.cue:10:8 + | +10 | _n: len(flags) + | ^^^ `len` in `when` of rule "len_rule" + | + = help: `len` inside `when` computes over the pattern's materialised value, not the input's. + +`_n: len(flags)` where `flags: [...string]` always yields `0` because the +pattern materialises the open list as `[]`. A downstream constraint like +`_n: >=2` either conflicts statically or is vacuously true — either way, +it cannot react to the input's actual list length. Use `list.MatchN` +instead: `flags: list.MatchN(>=2, string)`. + +[1] +``` + ## `--explain=fired` emits only fired traces With three rules and a Read payload, only `disjunction` fires (its disjunction diff --git a/tests/diagnostics_rules_broken_len/len_in_when.cue b/tests/diagnostics_rules_broken_len/len_in_when.cue new file mode 100644 index 0000000..8ea0786 --- /dev/null +++ b/tests/diagnostics_rules_broken_len/len_in_when.cue @@ -0,0 +1,18 @@ +package rules + +// Uses `len()` inside `when` to compute over an input-derived field. +// The pattern materialises `flags` as `[]`, so `len(flags)` is always 0. +// Triggers E0508 at load time. +len_rule: { + when: { + tool_input: parsed: { + flags: [...] + _n: len(flags) + _n: >=2 + } + } + then: deny: { + rule_id: "len-in-when" + reason: "too many flags" + } +}