From d6a0d37db901179ac2041b098577e47568d8481b Mon Sep 17 00:00:00 2001 From: Oleks Date: Thu, 13 Aug 2026 22:27:21 +0300 Subject: [PATCH] test(lint): pin block-style branch/path scoping for WP001 (oleks/pipetree#22) Every `branch:` in the corpus was flow-style `[main]` or scalar `main`; no fixture used the block form branch: - main so the parser's handling of it was correct but unpinned. That matters now because oleks/emmett 278a567 deleted the hand-rolled xonsh check and cut the pre-push WPR slot over to `pipetree lint` -- and block-style scoping is the exact input that defeated that predecessor (oleks/emmett#385), where it read as an unscoped trigger and hard-failed correctly-scoped configs. pipetree is now the sole guard for the class, so a silent regression here would land straight in everyone's pre-push hook. Adds two `_clean` fixtures (block `branch:`, and block `path:` with two entries so a first-item-wins parse cannot pass by accident), both wired into TestWP001_CleanCases. The verdict assertion alone is NOT sufficient, which is the reason for the separate TestWP001_BlockStyleScopingParsesAsScoped: a parser that drops the list value still leaves the `branch` KEY in the condition, so WP001 sees a non-`event` key and stays quiet whether or not the scoping survived. Silence cannot tell "understood" from "lost". Verified by mutation rather than by assertion alone -- making scalarOrList reject non-flow sequences (simulating oleks/emmett#385) leaves TestWP001_Fires, _EventListFires, _CleanCases and _RemediationCarriesClass2Caveat all PASSING and fails only the new parse test. The mutation was reverted; the diff here touches no parser code. verdicts.json regenerated with the real woodpecker-cli via `-update`, not hand-written: the oracle corpus is globbed, so new fixtures would otherwise drift it out of sync. Woodpecker's own parser independently reports both as valid with a when-block and one step, and no existing entry changed. --- internal/analyze/lint_test.go | 53 +++++++++++++++++++ internal/analyze/testdata/README.md | 14 +++++ .../testdata/wp001_blockbranch_clean.yaml | 24 +++++++++ .../testdata/wp001_blockpath_clean.yaml | 19 +++++++ internal/oracle/testdata/verdicts.json | 12 +++++ 5 files changed, 122 insertions(+) create mode 100644 internal/analyze/testdata/wp001_blockbranch_clean.yaml create mode 100644 internal/analyze/testdata/wp001_blockpath_clean.yaml diff --git a/internal/analyze/lint_test.go b/internal/analyze/lint_test.go index 09d0b73..6cc51cf 100644 --- a/internal/analyze/lint_test.go +++ b/internal/analyze/lint_test.go @@ -208,6 +208,8 @@ func TestWP001_CleanCases(t *testing.T) { "testdata/wp001_clean.yaml", "testdata/wp001_eventlist_clean.yaml", "testdata/wp001_steplevel_clean.yaml", + "testdata/wp001_blockbranch_clean.yaml", + "testdata/wp001_blockpath_clean.yaml", } { if got := findingsFor(lintFixture(t, file), "WP001"); len(got) != 0 { t.Errorf("%s: expected no WP001 finding, got %+v", file, got) @@ -215,6 +217,57 @@ func TestWP001_CleanCases(t *testing.T) { } } +// TestWP001_BlockStyleScopingParsesAsScoped pins the PARSE, not just the +// verdict (oleks/pipetree#22). WP001 staying quiet is necessary but not +// sufficient: a parser that dropped `branch:` entirely would also produce a +// bare push entry, which WP001 would then be right to flag - so silence alone +// cannot distinguish "scoping understood" from "scoping lost". Assert the +// constraint actually landed in the model. +// +// This is the exact shape that defeated the xonsh predecessor this linter +// replaced (oleks/emmett#385): its key regex demanded a same-line value, so a +// block-style `branch:` vanished from the entry and a correctly-scoped config +// hard-failed the push. That check is now deleted (oleks/emmett 278a567), +// leaving pipetree as the only thing standing between this bug class and +// everyone's pre-push hook. +func TestWP001_BlockStyleScopingParsesAsScoped(t *testing.T) { + for _, tc := range []struct { + file string + field string + want string + }{ + {"testdata/wp001_blockbranch_clean.yaml", "branch", "main"}, + {"testdata/wp001_blockpath_clean.yaml", "path", "src/**"}, + } { + pl, err := parse.File(tc.file) + if err != nil { + t.Fatalf("parse.File(%s): %v", tc.file, err) + } + + var pushConds []model.Condition + for _, c := range pl.When { + if c.Fields["event"] == "push" { + pushConds = append(pushConds, c) + } + } + if len(pushConds) != 1 { + t.Fatalf("%s: expected exactly 1 push condition, got %d (%+v)", + tc.file, len(pushConds), pl.When) + } + + got, ok := pushConds[0].Fields[tc.field] + if !ok { + t.Errorf("%s: block-style %q was dropped from the push condition; "+ + "fields were %+v", tc.file, tc.field, pushConds[0].Fields) + continue + } + if !strings.Contains(got, tc.want) { + t.Errorf("%s: push condition %q = %q, want it to contain %q", + tc.file, tc.field, got, tc.want) + } + } +} + // TestWP001_RemediationCarriesClass2Caveat is the constitution-V check in // test form (FR-002): the redundancy advice must never ship without the // warning about what that advice can create. diff --git a/internal/analyze/testdata/README.md b/internal/analyze/testdata/README.md index 3712a66..baf2623 100644 --- a/internal/analyze/testdata/README.md +++ b/internal/analyze/testdata/README.md @@ -18,6 +18,20 @@ naming a different rule, and one anchored to a line that has no finding. All four assert the fail-open contract — the finding survives and still blocks. +The `wp001_block*_clean.yaml` pair pins BLOCK-style scoping — a `branch:` or +`path:` whose list is indented under it rather than written inline as +`[main]` (oleks/pipetree#22). Both spellings are the same YAML, so both must +get the same verdict; they are separated here because the check pipetree +replaced got exactly this wrong (oleks/emmett#385), and since that check was +deleted (oleks/emmett 278a567) pipetree is the only remaining guard. + +Note these two are pinned by `TestWP001_BlockStyleScopingParsesAsScoped`, not +only by the `_clean` verdict. Rule silence alone cannot distinguish "scoping +understood" from "scoping lost": a parser that drops the value still leaves +the `branch` KEY in the condition, so WP001 sees a non-`event` key and stays +quiet either way. Verified by mutation — a deliberately style-sensitive +parser leaves every other WP001 test passing and fails only that one. + Class-2 fixtures are a *pair of files*, because the rules are pure functions of `(model, protection data)`: a config directory plus a JSON snapshot of what the Gitea API returns for diff --git a/internal/analyze/testdata/wp001_blockbranch_clean.yaml b/internal/analyze/testdata/wp001_blockbranch_clean.yaml new file mode 100644 index 0000000..39e91af --- /dev/null +++ b/internal/analyze/testdata/wp001_blockbranch_clean.yaml @@ -0,0 +1,24 @@ +# Same scoping as wp001_clean.yaml, written as a BLOCK-style list instead of +# the flow-style `branch: [main]`. The two spellings are the same YAML, so +# they must get the same verdict: quiet. +# +# This is the shape that broke the xonsh predecessor (oleks/emmett#385). Its +# key regex required a value on the SAME line as the key, so a `branch:` whose +# value lives on the following lines never entered the entry's key set, the +# entry read as bare, and a correctly-scoped config got a hard-failing false +# positive. pipetree is now the sole guard for this class (the xonsh check was +# deleted in oleks/emmett 278a567), so the style is pinned here deliberately. +# +# Modelled on the real config that surfaced it: ~/projects/gitea-mcp. +when: + - event: pull_request + - event: push + branch: + - main + - event: tag + +steps: + - name: unit + image: alpine + commands: + - echo running unit tests diff --git a/internal/analyze/testdata/wp001_blockpath_clean.yaml b/internal/analyze/testdata/wp001_blockpath_clean.yaml new file mode 100644 index 0000000..f980bea --- /dev/null +++ b/internal/analyze/testdata/wp001_blockpath_clean.yaml @@ -0,0 +1,19 @@ +# Block-style `path:` scoping — the other key WP001 accepts as narrowing, and +# the other half of oleks/emmett#385's blind spot. Multi-item on purpose: a +# single-element block list can be parsed correctly by accident (first item +# wins), a two-element one cannot. +# +# Must stay quiet: the push entry is scoped, so it is not the same check +# running twice on the same commit. +when: + - event: push + path: + - src/** + - internal/** + - event: pull_request + +steps: + - name: unit + image: alpine + commands: + - echo running unit tests diff --git a/internal/oracle/testdata/verdicts.json b/internal/oracle/testdata/verdicts.json index 4a35544..36eb1b2 100644 --- a/internal/oracle/testdata/verdicts.json +++ b/internal/oracle/testdata/verdicts.json @@ -29,6 +29,18 @@ "steps": 1, "has_when": true }, + { + "file": "../analyze/testdata/wp001_blockbranch_clean.yaml", + "valid": true, + "steps": 1, + "has_when": true + }, + { + "file": "../analyze/testdata/wp001_blockpath_clean.yaml", + "valid": true, + "steps": 1, + "has_when": true + }, { "file": "../analyze/testdata/wp001_clean.yaml", "valid": true, -- 2.54.0