From 2b954f581a8ced1f6651459c438c7de84feae829 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:44:00 +0200 Subject: [PATCH 1/2] Refuse a required context and a check name that disagree (#71) The required set holds strings and the workflows declare strings, and nothing on the platform ties the two together. A job renamed in passing removes itself from the required set while the tab still looks green, and a context required under a name nothing reports blocks every merge without saying why on the pull request. Five issues in the plan carry that warning in different words and nothing acted on it. internal/contexts is the comparison. It is a function over two lists and reads nothing, so every rule is proved against a fixture written out in full. cmd/contexts is the entry point, and .github/workflows/contexts.yml is where the ruleset is fetched, because half of this is a live setting rather than a file and lab opens no network connection. The deliberate absences are a list beside the comparison rather than a paragraph in a document. Most of it is one fact repeated: the ruleset requires no status check at all today, so every name this tree declares is outside the set, and each is written down rather than the comparison switching itself off while the set is empty. That is what catches the rename now: an absence names a literal string, so a job renamed while the set is still empty leaves its entry pointing at nothing and the check goes red. Two of the refusals also run without a network, in the ordinary suite, so a rename reddens the machine of whoever made it rather than waiting for the pull request. What a green result here does not say is written where the comparison is defined: it cannot judge behaviour, and it reads the ruleset as the API answered on the day it ran. The means is Go, which is what every other check in this tree is written in and what record 0001 chose. The fetch is the one thing that is not Go, and it is held to a single gh call in the workflow because a ruleset is not in the checkout. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- .github/workflows/contexts.yml | 108 ++++++ cmd/contexts/main.go | 122 +++++++ cmd/contexts/main_test.go | 103 ++++++ internal/contexts/contexts.go | 318 ++++++++++++++++++ internal/contexts/contexts_test.go | 288 ++++++++++++++++ internal/contexts/workflows.go | 239 +++++++++++++ internal/contexts/workflows_test.go | 131 ++++++++ .../absences | 0 .../expected-refusals | 1 + .../near-neighbour | 1 + .../required | 1 + .../workflows/two-jobs.yml | 20 ++ .../absences | 2 + .../expected-refusals | 2 + .../near-neighbour | 1 + .../required | 0 .../workflows/two-jobs.yml | 20 ++ .../a-matrix-name-expands-per-entry/absences | 0 .../expected-refusals | 0 .../a-matrix-name-expands-per-entry/required | 2 + .../workflows/matrix.yml | 22 ++ .../a-name-this-reader-cannot-expand/absences | 0 .../expected-refusals | 1 + .../near-neighbour | 1 + .../a-name-this-reader-cannot-expand/required | 0 .../workflows/no-matrix.yml | 14 + .../absences | 0 .../expected-refusals | 1 + .../near-neighbour | 1 + .../required | 3 + .../workflows/two-jobs.yml | 20 ++ .../an-absence-that-is-required/absences | 1 + .../expected-refusals | 1 + .../near-neighbour | 1 + .../an-absence-that-is-required/required | 2 + .../workflows/two-jobs.yml | 20 ++ .../an-absence-that-names-nothing/absences | 1 + .../expected-refusals | 1 + .../near-neighbour | 1 + .../an-absence-that-names-nothing/required | 2 + .../workflows/two-jobs.yml | 20 ++ testdata/contexts/everything-agrees/absences | 0 .../everything-agrees/expected-refusals | 0 testdata/contexts/everything-agrees/required | 2 + .../everything-agrees/workflows/two-jobs.yml | 20 ++ .../absences | 2 + .../expected-refusals | 0 .../required | 0 .../workflows/two-jobs.yml | 20 ++ 49 files changed, 1516 insertions(+) create mode 100644 .github/workflows/contexts.yml create mode 100644 cmd/contexts/main.go create mode 100644 cmd/contexts/main_test.go create mode 100644 internal/contexts/contexts.go create mode 100644 internal/contexts/contexts_test.go create mode 100644 internal/contexts/workflows.go create mode 100644 internal/contexts/workflows_test.go create mode 100644 testdata/contexts/a-declared-name-outside-the-required-set/absences create mode 100644 testdata/contexts/a-declared-name-outside-the-required-set/expected-refusals create mode 100644 testdata/contexts/a-declared-name-outside-the-required-set/near-neighbour create mode 100644 testdata/contexts/a-declared-name-outside-the-required-set/required create mode 100644 testdata/contexts/a-declared-name-outside-the-required-set/workflows/two-jobs.yml create mode 100644 testdata/contexts/a-job-renamed-while-the-set-is-empty/absences create mode 100644 testdata/contexts/a-job-renamed-while-the-set-is-empty/expected-refusals create mode 100644 testdata/contexts/a-job-renamed-while-the-set-is-empty/near-neighbour create mode 100644 testdata/contexts/a-job-renamed-while-the-set-is-empty/required create mode 100644 testdata/contexts/a-job-renamed-while-the-set-is-empty/workflows/two-jobs.yml create mode 100644 testdata/contexts/a-matrix-name-expands-per-entry/absences create mode 100644 testdata/contexts/a-matrix-name-expands-per-entry/expected-refusals create mode 100644 testdata/contexts/a-matrix-name-expands-per-entry/required create mode 100644 testdata/contexts/a-matrix-name-expands-per-entry/workflows/matrix.yml create mode 100644 testdata/contexts/a-name-this-reader-cannot-expand/absences create mode 100644 testdata/contexts/a-name-this-reader-cannot-expand/expected-refusals create mode 100644 testdata/contexts/a-name-this-reader-cannot-expand/near-neighbour create mode 100644 testdata/contexts/a-name-this-reader-cannot-expand/required create mode 100644 testdata/contexts/a-name-this-reader-cannot-expand/workflows/no-matrix.yml create mode 100644 testdata/contexts/a-required-context-nothing-reports/absences create mode 100644 testdata/contexts/a-required-context-nothing-reports/expected-refusals create mode 100644 testdata/contexts/a-required-context-nothing-reports/near-neighbour create mode 100644 testdata/contexts/a-required-context-nothing-reports/required create mode 100644 testdata/contexts/a-required-context-nothing-reports/workflows/two-jobs.yml create mode 100644 testdata/contexts/an-absence-that-is-required/absences create mode 100644 testdata/contexts/an-absence-that-is-required/expected-refusals create mode 100644 testdata/contexts/an-absence-that-is-required/near-neighbour create mode 100644 testdata/contexts/an-absence-that-is-required/required create mode 100644 testdata/contexts/an-absence-that-is-required/workflows/two-jobs.yml create mode 100644 testdata/contexts/an-absence-that-names-nothing/absences create mode 100644 testdata/contexts/an-absence-that-names-nothing/expected-refusals create mode 100644 testdata/contexts/an-absence-that-names-nothing/near-neighbour create mode 100644 testdata/contexts/an-absence-that-names-nothing/required create mode 100644 testdata/contexts/an-absence-that-names-nothing/workflows/two-jobs.yml create mode 100644 testdata/contexts/everything-agrees/absences create mode 100644 testdata/contexts/everything-agrees/expected-refusals create mode 100644 testdata/contexts/everything-agrees/required create mode 100644 testdata/contexts/everything-agrees/workflows/two-jobs.yml create mode 100644 testdata/contexts/the-set-is-empty-and-every-name-is-written-down/absences create mode 100644 testdata/contexts/the-set-is-empty-and-every-name-is-written-down/expected-refusals create mode 100644 testdata/contexts/the-set-is-empty-and-every-name-is-written-down/required create mode 100644 testdata/contexts/the-set-is-empty-and-every-name-is-written-down/workflows/two-jobs.yml diff --git a/.github/workflows/contexts.yml b/.github/workflows/contexts.yml new file mode 100644 index 0000000..6b0ccca --- /dev/null +++ b/.github/workflows/contexts.yml @@ -0,0 +1,108 @@ +# The gate refers to check names, and this is what holds the two together. +# +# A required context is a string. A check name is a string in a workflow file. +# Nothing on the platform ties them, so a job renamed in passing removes itself +# from the required set while the tab still looks green, and a context required +# under a name nothing reports blocks every merge without saying why on the pull +# request. Both are failures of the same disagreement and this job refuses both. +# +# WHY THIS IS A WORKFLOW AND NOT A VERB OF THE RUNNER. Half the comparison is a +# live setting on the hosting platform rather than a file in the tree, so it can +# only be answered by asking the API. lab reads a checkout and opens no network +# connection, and that claim is repeated in every document here, so the fetch +# lives in the step below and the judgement lives in a command that is handed the +# answer on standard input. The token is granted on this one job and nowhere +# else, read-only, because reading a ruleset is all it is for. +# +# The check name below is what the required set refers to later, so it is written +# here rather than left to the job id, and changing it is a change to the gate +# rather than tidying. This job is the one place where that has a consequence it +# can see: its own name is in the deliberate-absence list in +# internal/contexts/contexts.go, so renaming it without moving that entry reddens +# this check and the ordinary suite together. +# +# Hardened the way every other workflow in this tree is, because the audit job in +# zizmor.yml refuses a new one that departs from it: permissions denied at the +# top and granted per job, every action pinned to a commit with its version in a +# comment, checkout without persisted credentials, and nothing expanded into a +# run block. +name: Required contexts + +on: + pull_request: + branches: ["**"] + push: + branches: [main] + +# Explicit deny-all at workflow level; the job below grants only what it needs. +permissions: {} + +# Cancel a superseded run on the same ref. Nothing here publishes, so a run that +# has been overtaken is safe to drop. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + contexts: + name: required contexts + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + # Read the tree, and read the ruleset. Both are reads and there is no + # third thing this job does. + contents: read + steps: + - name: Checkout Repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + # The toolchain comes from go.mod, so the version this job uses moves + # when the module says so and never separately. + go-version-file: go.mod + # This module has no dependencies and therefore no go.sum for a cache + # key to be built from. Asking for a cache anyway makes the step + # depend on a file that is not there. + cache: false + + - name: Read the contexts the ruleset requires + # The fetch is its own step so that a ruleset the run could not read is + # a different outcome from a ruleset that disagrees with the tree. A + # failed fetch here stops the job, and a job that had piped a failed + # fetch into the comparison would have compared the tree against an + # empty list and reported whatever that produced. + # + # An empty answer is not a failure. The ruleset on this board requires + # no status check today, and the comparison still has work to do in + # that state, which is why the count is printed rather than the run + # stopping. + # + # Every value reaches the script through the environment. Nothing an + # author controls is part of a command, and no expression is expanded + # inside a run block. + env: + GH_TOKEN: ${{ github.token }} + REPOSITORY: ${{ github.repository }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + run: | + set -euo pipefail + if ! gh api "repos/${REPOSITORY}/rules/branches/${DEFAULT_BRANCH}" \ + --jq '.[] | select(.type=="required_status_checks") + | .parameters.required_status_checks[].context' > required.txt; then + echo "::error::The ruleset on ${DEFAULT_BRANCH} could not be read, so this run could not judge whether the gate and the tree agree. That is not the same as them agreeing." + exit 1 + fi + echo "the ruleset on ${DEFAULT_BRANCH} requires $(wc -l < required.txt) context(s):" + cat required.txt + + - name: Compare them against the check names this tree declares + # Built from this commit rather than downloaded, so a change that breaks + # a refusal is caught by the pull request that made it. The exit codes + # are the contract in docs/decisions/0011-the-exit-codes.md: 0 refused + # nothing, 1 refused something, 2 could not do the job. This step keys on + # all three by letting the command's own code become the job's. + run: go run ./cmd/contexts < required.txt diff --git a/cmd/contexts/main.go b/cmd/contexts/main.go new file mode 100644 index 0000000..b532ca5 --- /dev/null +++ b/cmd/contexts/main.go @@ -0,0 +1,122 @@ +// Command contexts compares the contexts the ruleset requires on the default +// branch against the check names the workflows in this repository declare, and +// refuses when the two disagree. +// +// WHY THIS IS NOT A VERB OF THE RUNNER. lab reads a checkout and opens no +// network connection, which is a claim a downloader is asked to take on trust +// and which cmd/lab's own suite holds it to. Half of this comparison is a live +// setting on the hosting platform, so it can only be answered by asking the API, +// and putting that inside lab would widen what an operator has to believe about +// a binary they downloaded in exchange for a verb that is useless outside a +// workflow. Keeping it here costs a third entry point under cmd/, which record +// 0002 does not name, and that is the judgement in this file a reader should +// check rather than accept. It is the same one cmd/pullrequest already made for +// the same reason. +// +// THE API CALL IS NOT IN HERE EITHER. The required set arrives on standard +// input, one context per line, so this command reads a checkout and a list and +// nothing else. The workflow that runs it is where the token is, which keeps the +// credential in one visible place and lets every rule below be proved against a +// list a test wrote out in full. +package main + +import ( + "bufio" + "fmt" + "io" + "os" + "strings" + + "github.com/Flowfin/lab/internal/contexts" +) + +// The exit codes. Decision record 0011 is the contract, and this command returns +// the same three the runner does, for the same reasons, which is why they are +// written here with the record named rather than invented. The exit-code leg in +// internal/invariants reads every exit-code constant in the tree and refuses a +// code declared with two numbers, or a number declared under two codes, so keep +// the convention these three follow. +const ( + // exitClean means the run completed and refused nothing. It does not mean + // the gate is intact, only that the two lists agree, and the report says + // what it compared so the two are not confused. + exitClean = 0 + + // exitRefused means the run completed and refused something. It is the + // only code that carries refusals. + exitRefused = 1 + + // exitCannot means the check could not do its job: no required set on + // standard input, no workflow directory to read, a file in a shape the + // reader was not built for. It is deliberately not the code a refusal + // returns, because a gate that treats a broken invocation like a violation + // reports one as the other and nobody investigates either. + exitCannot = 2 +) + +func main() { + os.Exit(run(os.Stdin, os.Stdout, os.Stderr, contexts.WorkflowsDir, contexts.Absences)) +} + +// run is main with its edges passed in: where the workflows are read from, and +// which deliberate-absence list the comparison is made against. Both are +// parameters so that what the command prints and what it returns can be read by +// a test against a tree written out in full, rather than against whichever +// repository the suite happens to be running inside. +func run(in io.Reader, out, errOut io.Writer, workflowsDir string, absences []contexts.Absence) int { + required, err := readRequired(in) + if err != nil { + fmt.Fprintf(errOut, "contexts: %v\n", err) + return exitCannot + } + + declared, err := contexts.ReadWorkflows(workflowsDir) + if err != nil { + fmt.Fprintf(errOut, "contexts: %v\n", err) + return exitCannot + } + + verdict := contexts.Judge(declared, required, absences) + fmt.Fprint(out, verdict.Report(declared, required, absences)) + if len(verdict.Refusals) > 0 { + return exitRefused + } + return exitClean +} + +// readRequired reads the contexts the ruleset requires, one per line. +// +// AN EMPTY LIST IS A LIST AND NOT A FAILURE. The ruleset on this board requires +// no status check today, so a run that read nothing is the ordinary case rather +// than a broken invocation, and the comparison still has work to do: every check +// name the tree declares has to be written down as a deliberate absence, and an +// absence naming nothing is still refused. A command that treated no input as a +// reason to stop would switch the whole check off on exactly the board it was +// written for. +// +// What it will not accept is a line that is not a context name. A required +// context is a check-run name, and a line carrying a tab or a control character +// is a fetch that returned something other than the list this expects, which is +// a broken invocation rather than a gate that disagrees. +func readRequired(in io.Reader) ([]string, error) { + var required []string + scanner := bufio.NewScanner(in) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + if strings.ContainsAny(line, "\t") || strings.ContainsFunc(line, isControl) { + return nil, fmt.Errorf("the required set carries the line %q, which is not a check-run name, so what arrived on standard input is not the list this expects", line) + } + required = append(required, line) + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("cannot read the required set: %w", err) + } + return required, nil +} + +func isControl(r rune) bool { + return r < 0x20 || r == 0x7f +} diff --git a/cmd/contexts/main_test.go b/cmd/contexts/main_test.go new file mode 100644 index 0000000..eb2a814 --- /dev/null +++ b/cmd/contexts/main_test.go @@ -0,0 +1,103 @@ +package main + +import ( + "bytes" + "path/filepath" + "strings" + "testing" +) + +// fixtures is a case directory from the package's own harness, reused here so +// the command is proved against the same trees the rules are. +const fixtures = "../../testdata/contexts" + +// TestTheCommandReturnsTheCodeItsVerdictEarns pins the contract record 0011 +// sets. Anything keyed on one of these codes is reading that record whether or +// not anybody said so, and a gate that returned the same number for a +// disagreement and for a broken fetch would report one as the other. +func TestTheCommandReturnsTheCodeItsVerdictEarns(t *testing.T) { + for _, one := range []struct { + name string + required string + dir string + want int + }{ + { + name: "the two lists agree", + required: "the first check\nthe second check\n", + dir: filepath.Join(fixtures, "everything-agrees", "workflows"), + want: exitClean, + }, + { + name: "a required context nothing reports", + required: "the first check\nthe second check\na check nobody wrote\n", + dir: filepath.Join(fixtures, "a-required-context-nothing-reports", "workflows"), + want: exitRefused, + }, + { + name: "there is no workflow directory to read", + required: "the first check\n", + dir: filepath.Join(fixtures, "there-is-no-such-case", "workflows"), + want: exitCannot, + }, + { + name: "what arrived is not a list of check names", + required: "the first check\tand a column of something else\n", + dir: filepath.Join(fixtures, "everything-agrees", "workflows"), + want: exitCannot, + }, + } { + t.Run(one.name, func(t *testing.T) { + var out, errOut bytes.Buffer + got := run(strings.NewReader(one.required), &out, &errOut, one.dir, nil) + if got != one.want { + t.Errorf("returned %d, want %d\nstdout: %s\nstderr: %s", got, one.want, out.String(), errOut.String()) + } + }) + } +} + +// TestTheReportSaysWhatItCompared refuses a run whose whole output is a verdict. +// A comparison of two empty lists and a comparison of two full ones both produce +// no refusals, and the only thing that tells them apart is the count the run +// printed. +func TestTheReportSaysWhatItCompared(t *testing.T) { + var out, errOut bytes.Buffer + code := run( + strings.NewReader("the first check\nthe second check\n"), + &out, &errOut, + filepath.Join(fixtures, "everything-agrees", "workflows"), + nil, + ) + if code != exitClean { + t.Fatalf("returned %d\n%s", code, errOut.String()) + } + for _, want := range []string{ + "required by the ruleset: 2", + "declared by the workflows: 2", + "written down as deliberately absent:", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("the report does not say %q\n%s", want, out.String()) + } + } +} + +// TestAnEmptyRequiredSetIsCompared refuses a command that treats no required +// contexts as nothing to do. That is the state of this board today, and a check +// that switched itself off there would be a check that has never run. +func TestAnEmptyRequiredSetIsCompared(t *testing.T) { + var out, errOut bytes.Buffer + code := run( + strings.NewReader(""), + &out, &errOut, + filepath.Join(fixtures, "everything-agrees", "workflows"), + nil, + ) + if code != exitRefused { + t.Fatalf("returned %d, and with an empty required set every declared name is outside it\n%s%s", code, out.String(), errOut.String()) + } + if !strings.Contains(out.String(), "required by the ruleset: 0") { + t.Errorf("the report does not say the required set was empty\n%s", out.String()) + } +} diff --git a/internal/contexts/contexts.go b/internal/contexts/contexts.go new file mode 100644 index 0000000..de70fea --- /dev/null +++ b/internal/contexts/contexts.go @@ -0,0 +1,318 @@ +// Package contexts compares the two lists that have to agree for the merge +// gate on the default branch to mean what a reader thinks it means: the +// contexts the ruleset requires, and the check names the workflows in this +// repository declare. +// +// Five issues in the plan carry the same warning in different words. The check +// name is what the required set refers to, and a job renamed in passing removes +// itself from that set while the tab still looks green. Two other pieces of work +// come close and neither closes it. One assembles the required set from the +// names a completed run reported, which settles what the contexts are called on +// the day it runs. One walks the pull requests those contexts have to arrive on, +// which settles that they arrive on the day it walks. Both are true when they +// are done and neither notices the rename afterwards. +// +// THE JUDGEMENT IS A FUNCTION OVER VALUES AND IT READS NOTHING. Judge takes two +// lists and returns a verdict. Nothing here opens a file, runs a command or +// reads an environment variable, so every rule below is proved against a value a +// test writes out in full. Where the declared names come from is workflows.go's +// half, and the command under cmd/contexts is the only place that asks the API +// for the required set. +// +// # What a green verdict here does not say +// +// IT CANNOT JUDGE BEHAVIOUR. A job that reports the right name having verified +// nothing passes this comparison exactly like one that did its work. Nothing in +// a name says what ran under it, and the proof that a check bites is the fixture +// its own suite carries rather than the string it reports under. A green result +// here says the gate refers to checks that exist and that every check the tree +// declares is accounted for, and it says nothing whatever about what any of them +// found. +// +// IT READS THE RULESET AS THE API ANSWERED ON THE DAY IT RAN. The required set +// is a live setting rather than a tracked file, so a context added or removed by +// hand is caught on the next pull request rather than at the moment it changes. +// Between those two moments the gate and this comparison disagree and nothing +// says so. That is the ordinary failure of anything that describes a setting it +// does not own, and it is stated here because a green result is otherwise read +// as the gate being intact right now. +package contexts + +import ( + "fmt" + "sort" + "strings" +) + +// The properties this package can refuse. A property is the rule, named once +// here and named nowhere else, so a fixture declaring what it expects and a +// refusal the judgement produced are the same string or they are not equal. +const ( + // RequiredContextNothingReports refuses a context the ruleset requires + // that no workflow in this tree declares. It blocks every merge and gives + // no reason on the pull request, because a context that never arrives is + // indistinguishable from one that has not finished. The first response is + // to wait and the second is to drop the context from the required set, + // which repairs a blocked board by making the gate smaller. + RequiredContextNothingReports = "required-context-nothing-reports" + + // DeclaredNameOutsideTheRequiredSet refuses a check name the workflows + // declare that is neither in the required set nor written down as a + // deliberate absence. This is the quieter of the two directions: a gate + // somebody believes in and nothing enforces. It survives indefinitely, and + // a green board is what hides it. + DeclaredNameOutsideTheRequiredSet = "declared-name-outside-the-required-set" + + // AbsenceNamesNothingDeclared refuses a deliberate absence naming a check + // name no workflow in this tree declares. It is the direction that catches + // the rename while the required set is still empty: an absence is written + // against a literal string, so a job renamed in passing leaves its absence + // pointing at nothing and this refuses rather than passing in silence. + AbsenceNamesNothingDeclared = "absence-names-nothing-declared" + + // AbsenceIsRequired refuses a deliberate absence naming a context the + // ruleset requires. The two statements contradict each other, and the + // contradiction is what makes the absence list load-bearing rather than + // decorative: an entry that stayed behind after its context became + // required would silently exempt that context from the direction above. + AbsenceIsRequired = "absence-is-required" + + // NameCarriesAnExpression refuses a declared check name still carrying a + // workflow expression the reader could not resolve. A name reaching the + // comparison half-expanded would be compared against a string no gate can + // ever hold, so this refuses rather than reporting a drift that is really + // a reader that did not understand the file. + NameCarriesAnExpression = "name-carries-an-expression" +) + +// Absence is one check name this tree declares and deliberately keeps out of +// the required set, with the reason and with what retires it. +// +// THE LIST IS DATA AND NOT A JUDGEMENT. A comparison that cannot tell an +// intended absence from a drift produces a red board nobody can act on, and one +// that infers the intent produces a green board nobody can check. So every +// absence is written down as a literal string beside the reason for it, and an +// entry that stops matching the tree is a refusal rather than a silent pass. +type Absence struct { + // Name is the check name, exactly as the workflow declares it. + Name string + + // Why is the reason this name is outside the required set. + Why string + + // Until names the issue that retires this entry, or is empty where the + // absence is permanent. The difference is the whole value of the field: a + // permanent absence is a decision and a pending one is a debt, and a list + // that collapsed them would read as settled everywhere. + Until string +} + +// Absences is the deliberate-absence list, and it is here rather than in a +// document because this is where the comparison is defined and a list written +// somewhere else drifts against the thing that reads it. +// +// MOST OF THIS LIST IS ONE FACT REPEATED. The ruleset on the default branch +// requires no status check at all today, so every check name this tree declares +// is outside the required set, and each of them is written down rather than the +// whole comparison being switched off while the set is empty. What that buys is +// the rename: an absence names a literal string, so a job renamed while the set +// is still empty leaves its entry pointing at nothing and reddens this check. +// A comparison that special-cased the empty set would notice none of it. +// +// Issue #26 is where the set is assembled, and each pending entry below is +// retired by moving its name into the ruleset rather than by editing a document. +var Absences = []Absence{ + { + Name: "Scorecard analysis", + Why: "the supply-chain self-audit publishes from the default branch and has no pull-request trigger, so requiring it would require a context that never arrives on the thing being gated", + Until: "", + }, + {Name: "build (linux/amd64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "build (linux/arm64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "build (darwin/amd64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "build (darwin/arm64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "build (windows/amd64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "build (windows/arm64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "test (linux/amd64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "test (windows/amd64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "test (darwin/arm64)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "vet", Why: theSetIsEmpty, Until: "#26"}, + {Name: "format", Why: theSetIsEmpty, Until: "#26"}, + {Name: "CodeQL (go)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "DCO sign-off", Why: theSetIsEmpty, Until: "#26"}, + {Name: "dependency-review", Why: theSetIsEmpty, Until: "#26"}, + {Name: "headless and unelevated", Why: theSetIsEmpty, Until: "#26"}, + {Name: "invariants", Why: theSetIsEmpty, Until: "#26"}, + {Name: "prose format", Why: theSetIsEmpty, Until: "#26"}, + {Name: "pull request", Why: theSetIsEmpty, Until: "#26"}, + {Name: "records", Why: theSetIsEmpty, Until: "#26"}, + {Name: "required contexts", Why: theSetIsEmpty, Until: "#26"}, + {Name: "Reject Trojan Source Unicode", Why: theSetIsEmpty, Until: "#26"}, + {Name: "Audit workflows (zizmor)", Why: theSetIsEmpty, Until: "#26"}, +} + +// theSetIsEmpty is the reason every pending entry above carries, written once so +// twenty-two rows cannot drift into twenty-two slightly different sentences. +const theSetIsEmpty = "the ruleset on the default branch requires no status check at all today, so no name this tree declares can be in a set that has no members" + +// Declared is one check name a workflow in this tree declares. +type Declared struct { + // Name is the check name as it will be reported. + Name string + + // Workflow is the file it was read from, so a refusal names the file a + // reader has to open rather than only the string that was wrong. + Workflow string + + // FromJobID says the name was not written in the file and was taken from + // the job id, which is what the platform reports when a job carries no + // name. It is a note rather than a refusal: the fallback is the platform's + // own rule and a job relying on it is legal, but a reader asking where a + // gate's string comes from is entitled to know it came from an identifier + // rather than from a line somebody chose. + FromJobID bool +} + +// Refusal is one violation, named by its property so a fixture and a run compare +// the same strings. +type Refusal struct { + Property string + Subject string + Detail string +} + +func (r Refusal) String() string { + return fmt.Sprintf("%s: %s (%s)", r.Subject, r.Detail, r.Property) +} + +// Note is something worth printing that is not a violation. +type Note struct { + Subject string + Detail string +} + +// Verdict is what one comparison produced. +type Verdict struct { + Refusals []Refusal + Notes []Note +} + +// Properties is the set of property ids this verdict refused, sorted and +// deduplicated, which is what a fixture compares against. +func (v Verdict) Properties() []string { + seen := map[string]bool{} + var props []string + for _, refusal := range v.Refusals { + if !seen[refusal.Property] { + seen[refusal.Property] = true + props = append(props, refusal.Property) + } + } + sort.Strings(props) + return props +} + +// Judge compares what the ruleset requires against what the workflows declare, +// with the deliberate absences it is given. +// +// The absences are a parameter rather than a package-level read, so a fixture +// proves the rules against a list it wrote out in full and never against this +// repository's own. The list this repository runs with is Absences above, and +// the command passes it in. +func Judge(declared []Declared, required []string, absences []Absence) Verdict { + var verdict Verdict + + declaredByName := map[string]Declared{} + for _, entry := range declared { + if strings.Contains(entry.Name, "${{") { + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: NameCarriesAnExpression, + Subject: entry.Workflow, + Detail: fmt.Sprintf("the check name %q still carries a workflow expression, so what this job reports under was not resolved and nothing here can be compared against it", entry.Name), + }) + continue + } + declaredByName[entry.Name] = entry + if entry.FromJobID { + verdict.Notes = append(verdict.Notes, Note{ + Subject: entry.Workflow, + Detail: fmt.Sprintf("the check name %q comes from the job id, because the job carries no name of its own", entry.Name), + }) + } + } + + requiredSet := map[string]bool{} + for _, context := range required { + requiredSet[context] = true + } + + absent := map[string]bool{} + for _, absence := range absences { + absent[absence.Name] = true + if _, ok := declaredByName[absence.Name]; !ok { + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: AbsenceNamesNothingDeclared, + Subject: absence.Name, + Detail: fmt.Sprintf("this name is written down as deliberately outside the required set, and no workflow in this tree declares it. Either a job was renamed and this entry was left behind, or the entry never matched. The reason it carries is %q", absence.Why), + }) + } + if requiredSet[absence.Name] { + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: AbsenceIsRequired, + Subject: absence.Name, + Detail: fmt.Sprintf("the ruleset requires this context and this list says it is deliberately outside the required set, so the two disagree. The reason the list carries is %q", absence.Why), + }) + } + } + + for _, context := range sortedKeys(requiredSet) { + if _, ok := declaredByName[context]; !ok { + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: RequiredContextNothingReports, + Subject: context, + Detail: "the ruleset on the default branch requires this context and no workflow in this tree declares a check name matching it, so every merge waits for a tick that never arrives", + }) + } + } + + for _, name := range sortedKeys(declaredByName) { + if requiredSet[name] || absent[name] { + continue + } + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: DeclaredNameOutsideTheRequiredSet, + Subject: name, + Detail: fmt.Sprintf("%s declares this check name, the ruleset does not require it, and nothing writes it down as a deliberate absence, so it is a check that runs and holds no merge", declaredByName[name].Workflow), + }) + } + + return verdict +} + +// Report is what the run printed. It says what it examined before it says what +// it found, because a run that compared two empty lists and a run that compared +// two full ones both produce no refusals and are otherwise indistinguishable. +func (v Verdict) Report(declared []Declared, required []string, absences []Absence) string { + out := "compared the required contexts against the check names this tree declares\n" + out += fmt.Sprintf(" required by the ruleset: %d\n", len(required)) + out += fmt.Sprintf(" declared by the workflows: %d\n", len(declared)) + out += fmt.Sprintf(" written down as deliberately absent: %d\n", len(absences)) + + for _, note := range v.Notes { + out += fmt.Sprintf("note: %s: %s\n", note.Subject, note.Detail) + } + for _, refusal := range v.Refusals { + out += "refused: " + refusal.String() + "\n" + } + out += fmt.Sprintf("%d refusal(s), %d note(s)\n", len(v.Refusals), len(v.Notes)) + return out +} + +func sortedKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for key := range m { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} diff --git a/internal/contexts/contexts_test.go b/internal/contexts/contexts_test.go new file mode 100644 index 0000000..205af97 --- /dev/null +++ b/internal/contexts/contexts_test.go @@ -0,0 +1,288 @@ +// The harness these rules are proved with. +// +// A case is a directory under testdata/contexts/. It holds the workflow files +// the reader read, as files in the repository rather than as strings assembled +// at run time, so a reader can look at exactly the input the comparison saw. It +// holds the two lists the comparison is between, and it holds the whole set of +// refusals it should have produced. +// +// The layout of a case: +// +// testdata/contexts//workflows/ what the reader reads +// testdata/contexts//required one context per line, may be +// empty +// testdata/contexts//absences one deliberately absent check +// name per line, may be empty +// testdata/contexts//expected-refusals one property per line, may be +// empty +// testdata/contexts//near-neighbour the case that differs by the +// smallest legal change, required +// of a case that refuses +// +// It is the shape the record checks and the invariants are proved with rather +// than a third one, because a reader who has understood one harness here should +// not have to learn another. +package contexts + +import ( + "os" + "path/filepath" + "reflect" + "sort" + "strings" + "testing" +) + +// casesDir is where the cases live. Record 0002 puts the runner's own fixtures +// at the root of the tree rather than beside the package, so the path climbs out +// of internal/contexts. +const casesDir = "../../testdata/contexts" + +// workflowsInThisRepository is this repository's own workflow directory, read +// from inside the package directory. +const workflowsInThisRepository = "../../" + WorkflowsDir + +// expectation is what a case says the comparison should have produced. +type expectation struct { + required []string + absences []Absence + refusals []string + neighbour string +} + +// loadCases reads every case directory. It fails rather than skipping: a harness +// that quietly runs no cases is a green suite that proves nothing. +func loadCases(t *testing.T) map[string]expectation { + t.Helper() + + entries, err := os.ReadDir(casesDir) + if err != nil { + t.Fatalf("cannot read %s: %v", casesDir, err) + } + cases := map[string]expectation{} + for _, entry := range entries { + if !entry.IsDir() { + continue + } + dir := filepath.Join(casesDir, entry.Name()) + exp := expectation{ + required: lines(t, dir, "required"), + refusals: lines(t, dir, "expected-refusals"), + neighbour: readOptional(t, dir, "near-neighbour"), + } + for _, name := range lines(t, dir, "absences") { + exp.absences = append(exp.absences, Absence{ + Name: name, + Why: "this case says so", + Until: "", + }) + } + sort.Strings(exp.refusals) + cases[entry.Name()] = exp + } + if len(cases) == 0 { + t.Fatalf("no cases under %s", casesDir) + } + return cases +} + +func lines(t *testing.T, dir, name string) []string { + t.Helper() + data, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil { + if os.IsNotExist(err) { + return nil + } + t.Fatalf("cannot read %s: %v", filepath.Join(dir, name), err) + } + var out []string + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if line != "" { + out = append(out, line) + } + } + return out +} + +func readOptional(t *testing.T, dir, name string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil { + if os.IsNotExist(err) { + return "" + } + t.Fatalf("cannot read %s: %v", filepath.Join(dir, name), err) + } + return strings.TrimSpace(string(data)) +} + +// TestACaseRefusesExactlyWhatItDeclares is the leg that makes a fixture worth +// having. It compares the whole set of property ids, so a rule that refuses its +// own case and something else besides fails here rather than passing on the +// strength of the one line it was written for. +func TestACaseRefusesExactlyWhatItDeclares(t *testing.T) { + for name, exp := range loadCases(t) { + t.Run(name, func(t *testing.T) { + declared, err := ReadWorkflows(filepath.Join(casesDir, name, "workflows")) + if err != nil { + t.Fatalf("reading the workflows of %s: %v", name, err) + } + verdict := Judge(declared, exp.required, exp.absences) + got := verdict.Properties() + if got == nil { + got = []string{} + } + want := exp.refusals + if want == nil { + want = []string{} + } + if !reflect.DeepEqual(got, want) { + t.Errorf("case %s refused %v and declares %v\n%s", name, got, want, verdict.Report(declared, exp.required, exp.absences)) + } + }) + } +} + +// TestEveryPropertyHasACaseThatTripsIt refuses a property nothing proves. A +// property with no case behind it is a rule nobody has watched bite, and the +// whole reason the fixtures exist is that a rule which cannot be shown to refuse +// is indistinguishable from one that never runs. +func TestEveryPropertyHasACaseThatTripsIt(t *testing.T) { + all := []string{ + RequiredContextNothingReports, + DeclaredNameOutsideTheRequiredSet, + AbsenceNamesNothingDeclared, + AbsenceIsRequired, + NameCarriesAnExpression, + } + tripped := map[string]bool{} + for _, exp := range loadCases(t) { + for _, property := range exp.refusals { + tripped[property] = true + } + } + for _, property := range all { + if !tripped[property] { + t.Errorf("no case trips %s", property) + } + } + for property := range tripped { + found := false + for _, known := range all { + if property == known { + found = true + } + } + if !found { + t.Errorf("a case declares %s and this package has no such property, so the case proves nothing", property) + } + } +} + +// TestACaseThatRefusesNamesANearNeighbour is the leg that catches a rule +// refusing everything. A case proving a refusal names a case differing by the +// smallest legal change, and that case has to come out clean, so a rule that +// fires on anything at all fails here. +func TestACaseThatRefusesNamesANearNeighbour(t *testing.T) { + cases := loadCases(t) + for name, exp := range cases { + if len(exp.refusals) == 0 { + if exp.neighbour != "" { + t.Errorf("case %s refuses nothing and declares a near neighbour", name) + } + continue + } + if exp.neighbour == "" { + t.Errorf("case %s refuses %v and declares no near neighbour, so nothing here would notice a rule that refuses everything", name, exp.refusals) + continue + } + neighbour, ok := cases[exp.neighbour] + if !ok { + t.Errorf("case %s names %s as its near neighbour and there is no such case", name, exp.neighbour) + continue + } + if len(neighbour.refusals) != 0 { + t.Errorf("case %s names %s as its near neighbour, and that case expects %v rather than nothing", name, exp.neighbour, neighbour.refusals) + } + } +} + +// TestNoDeliberateAbsenceNamesSomethingThisTreeDoesNotDeclare is the rename +// refusal, run against this repository rather than against a fixture. +// +// It is here as well as in the workflow because it needs no network. Every entry +// in the absence list is a literal check name, so a job renamed in passing +// leaves its entry pointing at nothing, and this fails in an ordinary suite run +// on the machine of whoever made the rename rather than waiting for the pull +// request. What it cannot do is the other direction, which needs the required +// set and therefore the API, and that half is the workflow's. +func TestNoDeliberateAbsenceNamesSomethingThisTreeDoesNotDeclare(t *testing.T) { + declared, err := ReadWorkflows(workflowsInThisRepository) + if err != nil { + t.Fatalf("reading %s: %v", workflowsInThisRepository, err) + } + names := map[string]bool{} + for _, entry := range declared { + names[entry.Name] = true + } + for _, absence := range Absences { + if !names[absence.Name] { + t.Errorf("the absence list names %q and no workflow in this tree declares it, so either a job was renamed and the entry was left behind or the entry never matched", absence.Name) + } + } +} + +// TestEveryCheckNameThisTreeDeclaresIsWrittenDown is the same reading from the +// other side, and it holds only while the ruleset requires no status check. +// +// The command is what judges this against the live ruleset. This test exists +// because today the required set is empty, which makes the answer knowable +// without asking: every name has to be in the absence list, because there is no +// other place for it to be. When issue #26 assembles the set, a name moving into +// the ruleset is a name leaving this list, and this test is what makes that +// removal deliberate rather than silent. +func TestEveryCheckNameThisTreeDeclaresIsWrittenDown(t *testing.T) { + declared, err := ReadWorkflows(workflowsInThisRepository) + if err != nil { + t.Fatalf("reading %s: %v", workflowsInThisRepository, err) + } + written := map[string]bool{} + for _, absence := range Absences { + written[absence.Name] = true + } + for _, entry := range declared { + if !written[entry.Name] { + t.Errorf("%s declares the check name %q and nothing writes it down, so it is a check that runs and holds no merge", entry.Workflow, entry.Name) + } + } +} + +// TestAnAbsenceCarriesAReason refuses an entry that says only that it is absent. +// The list is read by somebody deciding whether an absence is still right, and +// an entry with no reason gives them nothing to decide with. +func TestAnAbsenceCarriesAReason(t *testing.T) { + for _, absence := range Absences { + if strings.TrimSpace(absence.Why) == "" { + t.Errorf("the absence %q carries no reason", absence.Name) + } + if absence.Until != "" && !strings.HasPrefix(absence.Until, "#") { + t.Errorf("the absence %q says it is retired by %q, which is not an issue reference", absence.Name, absence.Until) + } + } +} + +// TestAJobWithNoNameOfItsOwnIsNotedRatherThanRefused pins the one place this +// package answers with a note. The platform reports the job id where a job +// carries no name, so such a job is legal and its name is still a gate string, and +// a reader asking where that string came from is told. +func TestAJobWithNoNameOfItsOwnIsNotedRatherThanRefused(t *testing.T) { + declared := []Declared{{Name: "dependency-review", Workflow: "dependency-review.yml", FromJobID: true}} + verdict := Judge(declared, []string{"dependency-review"}, nil) + if len(verdict.Refusals) != 0 { + t.Fatalf("refused %v", verdict.Refusals) + } + if len(verdict.Notes) != 1 { + t.Fatalf("produced %d note(s), and the fallback to the job id is meant to be said out loud", len(verdict.Notes)) + } +} diff --git a/internal/contexts/workflows.go b/internal/contexts/workflows.go new file mode 100644 index 0000000..9fab96c --- /dev/null +++ b/internal/contexts/workflows.go @@ -0,0 +1,239 @@ +package contexts + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strings" +) + +// WorkflowsDir is where the platform reads workflows from, and therefore the +// only directory a declared check name can come from. +const WorkflowsDir = ".github/workflows" + +// ReadWorkflows reads every workflow file under the directory it is given and +// returns the check names they declare. +// +// WHY THIS READS THE FILES RATHER THAN A COMPLETED RUN. A run reports the names +// of the jobs that ran on it, which is a different set: a job filtered out by a +// condition reports nothing, and a workflow added on a branch reports nothing +// until it runs there. What the gate has to agree with is what the tree +// declares, because the tree is the thing a pull request changes and the rename +// this check exists to catch is a change to a file. +// +// WHAT IT UNDERSTANDS, AND WHERE IT STOPS. This module carries no dependencies +// and adding a YAML parser to read a dozen files of one known shape is a runtime +// cost paid for something narrower than the library. So it reads the shape the +// workflows in this tree are written in, by line and by indentation: a job is a +// key two spaces in under `jobs:`, its name is `name:` four spaces in, and a +// matrix is an `include:` list of flat key-and-value entries. It stops reading a +// job's own keys at `steps:`, because everything below that belongs to a step +// rather than to the job. +// +// Anything it does not understand is a refusal rather than a guess. A name it +// cannot finish expanding keeps its expression and Judge refuses it by name, so +// a workflow written in a shape this reader was not built for reddens the check +// instead of quietly declaring the wrong string. +func ReadWorkflows(dir string) ([]Declared, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("cannot read %s, which is where a declared check name can only come from: %w", dir, err) + } + + var declared []Declared + for _, entry := range entries { + if entry.IsDir() || !isWorkflowFile(entry.Name()) { + continue + } + path := filepath.Join(dir, entry.Name()) + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("cannot read %s: %w", path, err) + } + found, err := readWorkflow(entry.Name(), string(data)) + if err != nil { + return nil, err + } + declared = append(declared, found...) + } + + if len(declared) == 0 { + // A reader that returned nothing would make every deliberate absence + // dangle and every required context unreported, which is a wall of + // refusals describing a reader that found no files rather than a tree + // that is wrong. Say the real thing instead. + return nil, fmt.Errorf("%s declared no check name at all, so either there are no workflows there or this reader did not understand any of them", dir) + } + + sort.Slice(declared, func(i, j int) bool { + if declared[i].Workflow != declared[j].Workflow { + return declared[i].Workflow < declared[j].Workflow + } + return declared[i].Name < declared[j].Name + }) + return declared, nil +} + +func isWorkflowFile(name string) bool { + return strings.HasSuffix(name, ".yml") || strings.HasSuffix(name, ".yaml") +} + +// job is one job as this reader understood it. +type job struct { + id string + name string + named bool + matrix []map[string]string + inSteps bool +} + +// readWorkflow returns the check names one workflow file declares. +func readWorkflow(file, text string) ([]Declared, error) { + var jobs []*job + var current *job + + inJobs := false + includeIndent := -1 + + for _, raw := range strings.Split(text, "\n") { + line := strings.TrimRight(raw, "\r") + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + indent := len(line) - len(strings.TrimLeft(line, " ")) + + if indent == 0 { + inJobs = trimmed == "jobs:" + current = nil + includeIndent = -1 + continue + } + if !inJobs { + continue + } + + if indent == 2 && strings.HasSuffix(trimmed, ":") { + current = &job{id: strings.TrimSuffix(trimmed, ":")} + jobs = append(jobs, current) + includeIndent = -1 + continue + } + if current == nil { + continue + } + + // Inside an include list: entries are flat key-and-value pairs, a new + // entry starts at a dash, and the list ends at the first line that is + // not deeper than the `include:` key itself. + if includeIndent >= 0 { + if indent > includeIndent { + if err := readMatrixEntry(current, trimmed, file); err != nil { + return nil, err + } + continue + } + includeIndent = -1 + } + + if indent == 4 { + // Everything under steps: belongs to a step. A step carries a + // name of its own and reading one as the job's would declare a + // check name that no gate will ever see. + if trimmed == "steps:" { + current.inSteps = true + continue + } + if !current.inSteps && strings.HasPrefix(trimmed, "name:") { + current.name = unquote(strings.TrimSpace(strings.TrimPrefix(trimmed, "name:"))) + current.named = true + } + continue + } + if !current.inSteps && trimmed == "include:" { + includeIndent = indent + } + } + + var declared []Declared + for _, j := range jobs { + name := j.name + if !j.named { + // The platform reports the job id where a job carries no name of + // its own, so that is the string a required context would have to + // match. + name = j.id + } + expanded, err := expand(name, j.matrix, file) + if err != nil { + return nil, err + } + for _, one := range expanded { + declared = append(declared, Declared{Name: one, Workflow: file, FromJobID: !j.named}) + } + } + return declared, nil +} + +// readMatrixEntry reads one `key: value` line of an include list into the entry +// it belongs to. +func readMatrixEntry(j *job, trimmed, file string) error { + if strings.HasPrefix(trimmed, "- ") { + j.matrix = append(j.matrix, map[string]string{}) + trimmed = strings.TrimSpace(strings.TrimPrefix(trimmed, "- ")) + } + if len(j.matrix) == 0 { + return fmt.Errorf("%s: the job %q has a line in its include list before any entry started, which is a shape this reader was not built for", file, j.id) + } + key, value, found := strings.Cut(trimmed, ":") + if !found { + return fmt.Errorf("%s: the job %q has the include line %q, which carries no key and value, and this reader will not guess at it", file, j.id, trimmed) + } + j.matrix[len(j.matrix)-1][strings.TrimSpace(key)] = unquote(strings.TrimSpace(value)) + return nil +} + +// expand turns a job name carrying matrix expressions into one name per matrix +// entry. A name with no expression is itself, whether or not the job has a +// matrix, because a matrix job whose name mentions no matrix key reports one +// check name per entry under the same string and the gate sees one context. +func expand(name string, matrix []map[string]string, file string) ([]string, error) { + if !strings.Contains(name, "${{") { + return []string{name}, nil + } + if len(matrix) == 0 { + // The name is left as it stands, expression included, and Judge + // refuses it by name. Returning an error here would stop the whole + // run on one file, and a refusal naming the workflow is what a reader + // can act on. + return []string{name}, nil + } + var out []string + seen := map[string]bool{} + for _, entry := range matrix { + expanded := name + for key, value := range entry { + expanded = strings.ReplaceAll(expanded, "${{ matrix."+key+" }}", value) + expanded = strings.ReplaceAll(expanded, "${{matrix."+key+"}}", value) + } + if seen[expanded] { + continue + } + seen[expanded] = true + out = append(out, expanded) + } + if len(out) == 0 { + return nil, fmt.Errorf("%s: the name %q expanded to nothing", file, name) + } + return out, nil +} + +func unquote(value string) string { + if len(value) >= 2 { + if (value[0] == '"' && value[len(value)-1] == '"') || (value[0] == '\'' && value[len(value)-1] == '\'') { + return value[1 : len(value)-1] + } + } + return value +} diff --git a/internal/contexts/workflows_test.go b/internal/contexts/workflows_test.go new file mode 100644 index 0000000..9e23f86 --- /dev/null +++ b/internal/contexts/workflows_test.go @@ -0,0 +1,131 @@ +package contexts + +import ( + "reflect" + "testing" +) + +// TestTheReaderExpandsAMatrixName pins what a matrix job declares. Six entries +// under one name are six contexts a gate has to hold, and a reader that returned +// the unexpanded string would compare against a name no ruleset can ever carry. +func TestTheReaderExpandsAMatrixName(t *testing.T) { + const file = ` +jobs: + build: + name: build (${{ matrix.goos }}/${{ matrix.goarch }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - goos: linux + goarch: amd64 + - goos: windows + goarch: arm64 + steps: + - name: Build + run: echo build +` + declared, err := readWorkflow("build.yml", file) + if err != nil { + t.Fatalf("%v", err) + } + var got []string + for _, entry := range declared { + got = append(got, entry.Name) + } + want := []string{"build (linux/amd64)", "build (windows/arm64)"} + if !reflect.DeepEqual(got, want) { + t.Errorf("read %v, want %v", got, want) + } +} + +// TestTheReaderDoesNotReadAStepNameAsAJobName is the near-miss worth spending +// the effort on. Both keys are spelled `name:` and both sit under the same job, +// so a reader keyed on the word rather than on where it sits would declare a +// step's name as a check name, and that string would then be compared against a +// gate and found missing for a reason nobody could follow. +func TestTheReaderDoesNotReadAStepNameAsAJobName(t *testing.T) { + const file = ` +jobs: + records: + name: records + runs-on: ubuntu-latest + steps: + - name: Checkout Repository + uses: actions/checkout@0000000000000000000000000000000000000000 + - name: Walk the records + run: go run ./cmd/lab check . +` + declared, err := readWorkflow("records.yml", file) + if err != nil { + t.Fatalf("%v", err) + } + if len(declared) != 1 || declared[0].Name != "records" { + t.Errorf("read %+v, and the only check name this file declares is \"records\"", declared) + } +} + +// TestAJobWithNoNameDeclaresItsJobID pins the platform's own fallback, which is +// the rule a required context has to match against dependency-review.yml. +func TestAJobWithNoNameDeclaresItsJobID(t *testing.T) { + const file = ` +jobs: + dependency-review: + runs-on: ubuntu-latest + steps: + - name: Dependency review + uses: actions/dependency-review-action@0000000000000000000000000000000000000000 +` + declared, err := readWorkflow("dependency-review.yml", file) + if err != nil { + t.Fatalf("%v", err) + } + if len(declared) != 1 || declared[0].Name != "dependency-review" || !declared[0].FromJobID { + t.Errorf("read %+v, want the job id declared and marked as coming from the job id", declared) + } +} + +// TestAWorkflowNameIsNotAJobName pins that the string at the top of the file is +// the workflow's rather than a job's. A gate holds check names, and a reader +// that declared the workflow name would put a string in the comparison that no +// check run is ever called. +func TestAWorkflowNameIsNotAJobName(t *testing.T) { + const file = ` +name: Build and test + +on: + pull_request: + +jobs: + vet: + name: vet + runs-on: ubuntu-latest + steps: + - name: Vet + run: go vet ./... +` + declared, err := readWorkflow("build.yml", file) + if err != nil { + t.Fatalf("%v", err) + } + if len(declared) != 1 || declared[0].Name != "vet" { + t.Errorf("read %+v, want only the job name", declared) + } +} + +// TestThisRepositoryDeclaresTheNamesItsWorkflowsCarry reads the real directory +// rather than a string, so a workflow written in a shape this reader was not +// built for is caught here rather than as a wall of refusals on a pull request. +func TestThisRepositoryDeclaresTheNamesItsWorkflowsCarry(t *testing.T) { + declared, err := ReadWorkflows(workflowsInThisRepository) + if err != nil { + t.Fatalf("%v", err) + } + for _, entry := range declared { + if entry.Name == "" { + t.Errorf("%s declares an empty check name", entry.Workflow) + } + } + t.Logf("this tree declares %d check name(s)", len(declared)) +} diff --git a/testdata/contexts/a-declared-name-outside-the-required-set/absences b/testdata/contexts/a-declared-name-outside-the-required-set/absences new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-declared-name-outside-the-required-set/expected-refusals b/testdata/contexts/a-declared-name-outside-the-required-set/expected-refusals new file mode 100644 index 0000000..48576dc --- /dev/null +++ b/testdata/contexts/a-declared-name-outside-the-required-set/expected-refusals @@ -0,0 +1 @@ +declared-name-outside-the-required-set diff --git a/testdata/contexts/a-declared-name-outside-the-required-set/near-neighbour b/testdata/contexts/a-declared-name-outside-the-required-set/near-neighbour new file mode 100644 index 0000000..8255792 --- /dev/null +++ b/testdata/contexts/a-declared-name-outside-the-required-set/near-neighbour @@ -0,0 +1 @@ +everything-agrees diff --git a/testdata/contexts/a-declared-name-outside-the-required-set/required b/testdata/contexts/a-declared-name-outside-the-required-set/required new file mode 100644 index 0000000..34a4e91 --- /dev/null +++ b/testdata/contexts/a-declared-name-outside-the-required-set/required @@ -0,0 +1 @@ +the first check diff --git a/testdata/contexts/a-declared-name-outside-the-required-set/workflows/two-jobs.yml b/testdata/contexts/a-declared-name-outside-the-required-set/workflows/two-jobs.yml new file mode 100644 index 0000000..49fda54 --- /dev/null +++ b/testdata/contexts/a-declared-name-outside-the-required-set/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second diff --git a/testdata/contexts/a-job-renamed-while-the-set-is-empty/absences b/testdata/contexts/a-job-renamed-while-the-set-is-empty/absences new file mode 100644 index 0000000..f8407c5 --- /dev/null +++ b/testdata/contexts/a-job-renamed-while-the-set-is-empty/absences @@ -0,0 +1,2 @@ +the first check +the second check diff --git a/testdata/contexts/a-job-renamed-while-the-set-is-empty/expected-refusals b/testdata/contexts/a-job-renamed-while-the-set-is-empty/expected-refusals new file mode 100644 index 0000000..f32f74c --- /dev/null +++ b/testdata/contexts/a-job-renamed-while-the-set-is-empty/expected-refusals @@ -0,0 +1,2 @@ +absence-names-nothing-declared +declared-name-outside-the-required-set diff --git a/testdata/contexts/a-job-renamed-while-the-set-is-empty/near-neighbour b/testdata/contexts/a-job-renamed-while-the-set-is-empty/near-neighbour new file mode 100644 index 0000000..7c578a9 --- /dev/null +++ b/testdata/contexts/a-job-renamed-while-the-set-is-empty/near-neighbour @@ -0,0 +1 @@ +the-set-is-empty-and-every-name-is-written-down diff --git a/testdata/contexts/a-job-renamed-while-the-set-is-empty/required b/testdata/contexts/a-job-renamed-while-the-set-is-empty/required new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-job-renamed-while-the-set-is-empty/workflows/two-jobs.yml b/testdata/contexts/a-job-renamed-while-the-set-is-empty/workflows/two-jobs.yml new file mode 100644 index 0000000..5b60d1f --- /dev/null +++ b/testdata/contexts/a-job-renamed-while-the-set-is-empty/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check, tidied + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second diff --git a/testdata/contexts/a-matrix-name-expands-per-entry/absences b/testdata/contexts/a-matrix-name-expands-per-entry/absences new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-matrix-name-expands-per-entry/expected-refusals b/testdata/contexts/a-matrix-name-expands-per-entry/expected-refusals new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-matrix-name-expands-per-entry/required b/testdata/contexts/a-matrix-name-expands-per-entry/required new file mode 100644 index 0000000..d519c6c --- /dev/null +++ b/testdata/contexts/a-matrix-name-expands-per-entry/required @@ -0,0 +1,2 @@ +build (linux/amd64) +build (windows/arm64) diff --git a/testdata/contexts/a-matrix-name-expands-per-entry/workflows/matrix.yml b/testdata/contexts/a-matrix-name-expands-per-entry/workflows/matrix.yml new file mode 100644 index 0000000..4950396 --- /dev/null +++ b/testdata/contexts/a-matrix-name-expands-per-entry/workflows/matrix.yml @@ -0,0 +1,22 @@ +name: A matrix + +on: + pull_request: + +permissions: {} + +jobs: + build: + name: build (${{ matrix.goos }}/${{ matrix.goarch }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - goos: linux + goarch: amd64 + - goos: windows + goarch: arm64 + steps: + - name: Build + run: echo build diff --git a/testdata/contexts/a-name-this-reader-cannot-expand/absences b/testdata/contexts/a-name-this-reader-cannot-expand/absences new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-name-this-reader-cannot-expand/expected-refusals b/testdata/contexts/a-name-this-reader-cannot-expand/expected-refusals new file mode 100644 index 0000000..57eb762 --- /dev/null +++ b/testdata/contexts/a-name-this-reader-cannot-expand/expected-refusals @@ -0,0 +1 @@ +name-carries-an-expression diff --git a/testdata/contexts/a-name-this-reader-cannot-expand/near-neighbour b/testdata/contexts/a-name-this-reader-cannot-expand/near-neighbour new file mode 100644 index 0000000..c784b3b --- /dev/null +++ b/testdata/contexts/a-name-this-reader-cannot-expand/near-neighbour @@ -0,0 +1 @@ +a-matrix-name-expands-per-entry diff --git a/testdata/contexts/a-name-this-reader-cannot-expand/required b/testdata/contexts/a-name-this-reader-cannot-expand/required new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-name-this-reader-cannot-expand/workflows/no-matrix.yml b/testdata/contexts/a-name-this-reader-cannot-expand/workflows/no-matrix.yml new file mode 100644 index 0000000..1028f9e --- /dev/null +++ b/testdata/contexts/a-name-this-reader-cannot-expand/workflows/no-matrix.yml @@ -0,0 +1,14 @@ +name: A name with nothing behind it + +on: + pull_request: + +permissions: {} + +jobs: + build: + name: build (${{ matrix.goos }}) + runs-on: ubuntu-latest + steps: + - name: Build + run: echo build diff --git a/testdata/contexts/a-required-context-nothing-reports/absences b/testdata/contexts/a-required-context-nothing-reports/absences new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/a-required-context-nothing-reports/expected-refusals b/testdata/contexts/a-required-context-nothing-reports/expected-refusals new file mode 100644 index 0000000..f749eda --- /dev/null +++ b/testdata/contexts/a-required-context-nothing-reports/expected-refusals @@ -0,0 +1 @@ +required-context-nothing-reports diff --git a/testdata/contexts/a-required-context-nothing-reports/near-neighbour b/testdata/contexts/a-required-context-nothing-reports/near-neighbour new file mode 100644 index 0000000..8255792 --- /dev/null +++ b/testdata/contexts/a-required-context-nothing-reports/near-neighbour @@ -0,0 +1 @@ +everything-agrees diff --git a/testdata/contexts/a-required-context-nothing-reports/required b/testdata/contexts/a-required-context-nothing-reports/required new file mode 100644 index 0000000..a502568 --- /dev/null +++ b/testdata/contexts/a-required-context-nothing-reports/required @@ -0,0 +1,3 @@ +the first check +the second check +a check nobody wrote diff --git a/testdata/contexts/a-required-context-nothing-reports/workflows/two-jobs.yml b/testdata/contexts/a-required-context-nothing-reports/workflows/two-jobs.yml new file mode 100644 index 0000000..49fda54 --- /dev/null +++ b/testdata/contexts/a-required-context-nothing-reports/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second diff --git a/testdata/contexts/an-absence-that-is-required/absences b/testdata/contexts/an-absence-that-is-required/absences new file mode 100644 index 0000000..cfabd08 --- /dev/null +++ b/testdata/contexts/an-absence-that-is-required/absences @@ -0,0 +1 @@ +the second check diff --git a/testdata/contexts/an-absence-that-is-required/expected-refusals b/testdata/contexts/an-absence-that-is-required/expected-refusals new file mode 100644 index 0000000..d05a48a --- /dev/null +++ b/testdata/contexts/an-absence-that-is-required/expected-refusals @@ -0,0 +1 @@ +absence-is-required diff --git a/testdata/contexts/an-absence-that-is-required/near-neighbour b/testdata/contexts/an-absence-that-is-required/near-neighbour new file mode 100644 index 0000000..8255792 --- /dev/null +++ b/testdata/contexts/an-absence-that-is-required/near-neighbour @@ -0,0 +1 @@ +everything-agrees diff --git a/testdata/contexts/an-absence-that-is-required/required b/testdata/contexts/an-absence-that-is-required/required new file mode 100644 index 0000000..f8407c5 --- /dev/null +++ b/testdata/contexts/an-absence-that-is-required/required @@ -0,0 +1,2 @@ +the first check +the second check diff --git a/testdata/contexts/an-absence-that-is-required/workflows/two-jobs.yml b/testdata/contexts/an-absence-that-is-required/workflows/two-jobs.yml new file mode 100644 index 0000000..49fda54 --- /dev/null +++ b/testdata/contexts/an-absence-that-is-required/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second diff --git a/testdata/contexts/an-absence-that-names-nothing/absences b/testdata/contexts/an-absence-that-names-nothing/absences new file mode 100644 index 0000000..7b45295 --- /dev/null +++ b/testdata/contexts/an-absence-that-names-nothing/absences @@ -0,0 +1 @@ +a check nobody wrote diff --git a/testdata/contexts/an-absence-that-names-nothing/expected-refusals b/testdata/contexts/an-absence-that-names-nothing/expected-refusals new file mode 100644 index 0000000..b46ddf8 --- /dev/null +++ b/testdata/contexts/an-absence-that-names-nothing/expected-refusals @@ -0,0 +1 @@ +absence-names-nothing-declared diff --git a/testdata/contexts/an-absence-that-names-nothing/near-neighbour b/testdata/contexts/an-absence-that-names-nothing/near-neighbour new file mode 100644 index 0000000..8255792 --- /dev/null +++ b/testdata/contexts/an-absence-that-names-nothing/near-neighbour @@ -0,0 +1 @@ +everything-agrees diff --git a/testdata/contexts/an-absence-that-names-nothing/required b/testdata/contexts/an-absence-that-names-nothing/required new file mode 100644 index 0000000..f8407c5 --- /dev/null +++ b/testdata/contexts/an-absence-that-names-nothing/required @@ -0,0 +1,2 @@ +the first check +the second check diff --git a/testdata/contexts/an-absence-that-names-nothing/workflows/two-jobs.yml b/testdata/contexts/an-absence-that-names-nothing/workflows/two-jobs.yml new file mode 100644 index 0000000..49fda54 --- /dev/null +++ b/testdata/contexts/an-absence-that-names-nothing/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second diff --git a/testdata/contexts/everything-agrees/absences b/testdata/contexts/everything-agrees/absences new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/everything-agrees/expected-refusals b/testdata/contexts/everything-agrees/expected-refusals new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/everything-agrees/required b/testdata/contexts/everything-agrees/required new file mode 100644 index 0000000..f8407c5 --- /dev/null +++ b/testdata/contexts/everything-agrees/required @@ -0,0 +1,2 @@ +the first check +the second check diff --git a/testdata/contexts/everything-agrees/workflows/two-jobs.yml b/testdata/contexts/everything-agrees/workflows/two-jobs.yml new file mode 100644 index 0000000..49fda54 --- /dev/null +++ b/testdata/contexts/everything-agrees/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second diff --git a/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/absences b/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/absences new file mode 100644 index 0000000..f8407c5 --- /dev/null +++ b/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/absences @@ -0,0 +1,2 @@ +the first check +the second check diff --git a/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/expected-refusals b/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/expected-refusals new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/required b/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/required new file mode 100644 index 0000000..e69de29 diff --git a/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/workflows/two-jobs.yml b/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/workflows/two-jobs.yml new file mode 100644 index 0000000..49fda54 --- /dev/null +++ b/testdata/contexts/the-set-is-empty-and-every-name-is-written-down/workflows/two-jobs.yml @@ -0,0 +1,20 @@ +name: Two jobs + +on: + pull_request: + +permissions: {} + +jobs: + first: + name: the first check + runs-on: ubuntu-latest + steps: + - name: Do the first thing + run: echo first + second: + name: the second check + runs-on: ubuntu-latest + steps: + - name: Do the second thing + run: echo second From 5a445a50c9dcc29078ff1a5b50cce54f20effa8c Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:49:47 +0200 Subject: [PATCH 2/2] Count the check names no workflow file carries (#71) The first commit read check names out of the workflow files and nothing else, which is short by two. A code-scanning upload creates a check run of its own, named after the analysis rather than after the job that uploaded it, and it is a name a ruleset can require exactly like any other. Measured on the head commit of this branch rather than supposed: gh api "repos/Flowfin/lab/commits/$(git rev-parse HEAD)/check-runs" --paginate \ --jq '.check_runs[] | "\(.name)\t\(.app.slug)"' | sort -u Twenty-four names, of which twenty-two come from github-actions and two, CodeQL and zizmor, come from github-advanced-security. Without this the comparison would refuse either of those as a required context nothing reports, which is a false red on the change that assembles the required set, and a false red is the failure this check exists to avoid rather than to cause. The two names are written down beside the absences with the command that found them, and what that costs is stated there: the string they report under is decided by the analysis rather than by a line in this tree, so a change to it is not caught here. The report says which of the two claims a reader is holding. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- cmd/contexts/main.go | 33 +++++++++++----- cmd/contexts/main_test.go | 8 ++-- internal/contexts/contexts.go | 60 +++++++++++++++++++++++++++++- internal/contexts/contexts_test.go | 49 ++++++++++++++++++++---- 4 files changed, 126 insertions(+), 24 deletions(-) diff --git a/cmd/contexts/main.go b/cmd/contexts/main.go index b532ca5..fd8725c 100644 --- a/cmd/contexts/main.go +++ b/cmd/contexts/main.go @@ -55,29 +55,42 @@ const ( ) func main() { - os.Exit(run(os.Stdin, os.Stdout, os.Stderr, contexts.WorkflowsDir, contexts.Absences)) + os.Exit(run(os.Stdin, os.Stdout, os.Stderr, edges{ + workflowsDir: contexts.WorkflowsDir, + absences: contexts.Absences, + alsoReported: contexts.ReportedOutsideAWorkflowFile, + })) } -// run is main with its edges passed in: where the workflows are read from, and -// which deliberate-absence list the comparison is made against. Both are -// parameters so that what the command prints and what it returns can be read by -// a test against a tree written out in full, rather than against whichever -// repository the suite happens to be running inside. -func run(in io.Reader, out, errOut io.Writer, workflowsDir string, absences []contexts.Absence) int { +// edges is everything this command reaches for that is not its own logic: where +// the workflows are read from, which deliberate-absence list the comparison is +// made against, and which check names arrive from something other than a job. +// All three are parameters so that what the command prints and what it returns +// can be read by a test against a tree written out in full, rather than against +// whichever repository the suite happens to be running inside. +type edges struct { + workflowsDir string + absences []contexts.Absence + alsoReported []contexts.Declared +} + +// run is main with its edges passed in. +func run(in io.Reader, out, errOut io.Writer, e edges) int { required, err := readRequired(in) if err != nil { fmt.Fprintf(errOut, "contexts: %v\n", err) return exitCannot } - declared, err := contexts.ReadWorkflows(workflowsDir) + declared, err := contexts.ReadWorkflows(e.workflowsDir) if err != nil { fmt.Fprintf(errOut, "contexts: %v\n", err) return exitCannot } + declared = append(declared, e.alsoReported...) - verdict := contexts.Judge(declared, required, absences) - fmt.Fprint(out, verdict.Report(declared, required, absences)) + verdict := contexts.Judge(declared, required, e.absences) + fmt.Fprint(out, verdict.Report(declared, required, e.absences)) if len(verdict.Refusals) > 0 { return exitRefused } diff --git a/cmd/contexts/main_test.go b/cmd/contexts/main_test.go index eb2a814..e873a5b 100644 --- a/cmd/contexts/main_test.go +++ b/cmd/contexts/main_test.go @@ -49,7 +49,7 @@ func TestTheCommandReturnsTheCodeItsVerdictEarns(t *testing.T) { } { t.Run(one.name, func(t *testing.T) { var out, errOut bytes.Buffer - got := run(strings.NewReader(one.required), &out, &errOut, one.dir, nil) + got := run(strings.NewReader(one.required), &out, &errOut, edges{workflowsDir: one.dir}) if got != one.want { t.Errorf("returned %d, want %d\nstdout: %s\nstderr: %s", got, one.want, out.String(), errOut.String()) } @@ -66,8 +66,7 @@ func TestTheReportSaysWhatItCompared(t *testing.T) { code := run( strings.NewReader("the first check\nthe second check\n"), &out, &errOut, - filepath.Join(fixtures, "everything-agrees", "workflows"), - nil, + edges{workflowsDir: filepath.Join(fixtures, "everything-agrees", "workflows")}, ) if code != exitClean { t.Fatalf("returned %d\n%s", code, errOut.String()) @@ -91,8 +90,7 @@ func TestAnEmptyRequiredSetIsCompared(t *testing.T) { code := run( strings.NewReader(""), &out, &errOut, - filepath.Join(fixtures, "everything-agrees", "workflows"), - nil, + edges{workflowsDir: filepath.Join(fixtures, "everything-agrees", "workflows")}, ) if code != exitRefused { t.Fatalf("returned %d, and with an empty required set every declared name is outside it\n%s%s", code, out.String(), errOut.String()) diff --git a/internal/contexts/contexts.go b/internal/contexts/contexts.go index de70fea..8ec6353 100644 --- a/internal/contexts/contexts.go +++ b/internal/contexts/contexts.go @@ -139,6 +139,8 @@ var Absences = []Absence{ {Name: "vet", Why: theSetIsEmpty, Until: "#26"}, {Name: "format", Why: theSetIsEmpty, Until: "#26"}, {Name: "CodeQL (go)", Why: theSetIsEmpty, Until: "#26"}, + {Name: "CodeQL", Why: theSetIsEmpty, Until: "#26"}, + {Name: "zizmor", Why: theSetIsEmpty, Until: "#26"}, {Name: "DCO sign-off", Why: theSetIsEmpty, Until: "#26"}, {Name: "dependency-review", Why: theSetIsEmpty, Until: "#26"}, {Name: "headless and unelevated", Why: theSetIsEmpty, Until: "#26"}, @@ -152,9 +154,51 @@ var Absences = []Absence{ } // theSetIsEmpty is the reason every pending entry above carries, written once so -// twenty-two rows cannot drift into twenty-two slightly different sentences. +// two dozen rows cannot drift into two dozen slightly different sentences. const theSetIsEmpty = "the ruleset on the default branch requires no status check at all today, so no name this tree declares can be in a set that has no members" +// ReportedOutsideAWorkflowFile is every check name that arrives on a commit here +// and is written in no workflow file, so the reader of those files can never +// find it. +// +// NOT EVERY CHECK RUN COMES FROM A JOB. A code-scanning upload creates a check +// run of its own, named after the analysis rather than after the job that +// uploaded it, and it is a name a ruleset can require exactly like any other. A +// comparison that knew only about jobs would refuse such a context as one +// nothing reports, which is a false red on the change that assembles the +// required set, and a false red is the failure this comparison exists to avoid +// rather than to cause. +// +// Measured rather than assumed. Every check run on one commit, with the +// application that created it: +// +// gh api "repos/Flowfin/lab/commits/$(git rev-parse HEAD)/check-runs" --paginate \ +// --jq '.check_runs[] | "\(.name)\t\(.app.slug)"' | sort -u +// +// On 2026-08-12 that printed twenty-four names, of which twenty-two came from +// github-actions and the two below came from github-advanced-security. Re-run it +// before trusting this list. +// +// WHAT THIS LIST COSTS. These two names are held here rather than read out of a +// file, so the rename this whole check is about is not caught for them: the +// string a code-scanning upload reports under is decided by the analysis rather +// than by a line somebody can change in this tree, and nothing here reads it. +// For these two the comparison is a statement that the name is expected, and for +// every other name it is a statement about what the tree says. The two are +// different claims and the report says which one a reader is holding. +var ReportedOutsideAWorkflowFile = []Declared{ + { + Name: "CodeQL", + Workflow: "codeql.yml", + FromUpload: true, + }, + { + Name: "zizmor", + Workflow: "zizmor.yml", + FromUpload: true, + }, +} + // Declared is one check name a workflow in this tree declares. type Declared struct { // Name is the check name as it will be reported. @@ -171,6 +215,14 @@ type Declared struct { // gate's string comes from is entitled to know it came from an identifier // rather than from a line somebody chose. FromJobID bool + + // FromUpload says this name was not read out of a workflow file at all and + // comes from ReportedOutsideAWorkflowFile. Workflow then names the file + // whose run causes the check to appear rather than the file the string was + // read from, and a rename of the string is not something this comparison + // can see. It is a note for the same reason as the field above: what a + // reader is holding is different for these names and the report says so. + FromUpload bool } // Refusal is one violation, named by its property so a fixture and a run compare @@ -239,6 +291,12 @@ func Judge(declared []Declared, required []string, absences []Absence) Verdict { Detail: fmt.Sprintf("the check name %q comes from the job id, because the job carries no name of its own", entry.Name), }) } + if entry.FromUpload { + verdict.Notes = append(verdict.Notes, Note{ + Subject: entry.Workflow, + Detail: fmt.Sprintf("the check name %q is written in no workflow file and is expected here rather than read, so a change to the string it reports under is not caught by this comparison", entry.Name), + }) + } } requiredSet := map[string]bool{} diff --git a/internal/contexts/contexts_test.go b/internal/contexts/contexts_test.go index 205af97..aaeb1ec 100644 --- a/internal/contexts/contexts_test.go +++ b/internal/contexts/contexts_test.go @@ -208,6 +208,19 @@ func TestACaseThatRefusesNamesANearNeighbour(t *testing.T) { } } +// declarationsInThisRepository is every check name that arrives on a commit +// here: the ones the workflow files declare, and the ones a code-scanning upload +// reports under, which are written down in ReportedOutsideAWorkflowFile because +// no file in this tree carries them. +func declarationsInThisRepository(t *testing.T) []Declared { + t.Helper() + declared, err := ReadWorkflows(workflowsInThisRepository) + if err != nil { + t.Fatalf("reading %s: %v", workflowsInThisRepository, err) + } + return append(declared, ReportedOutsideAWorkflowFile...) +} + // TestNoDeliberateAbsenceNamesSomethingThisTreeDoesNotDeclare is the rename // refusal, run against this repository rather than against a fixture. // @@ -218,10 +231,7 @@ func TestACaseThatRefusesNamesANearNeighbour(t *testing.T) { // request. What it cannot do is the other direction, which needs the required // set and therefore the API, and that half is the workflow's. func TestNoDeliberateAbsenceNamesSomethingThisTreeDoesNotDeclare(t *testing.T) { - declared, err := ReadWorkflows(workflowsInThisRepository) - if err != nil { - t.Fatalf("reading %s: %v", workflowsInThisRepository, err) - } + declared := declarationsInThisRepository(t) names := map[string]bool{} for _, entry := range declared { names[entry.Name] = true @@ -243,10 +253,7 @@ func TestNoDeliberateAbsenceNamesSomethingThisTreeDoesNotDeclare(t *testing.T) { // the ruleset is a name leaving this list, and this test is what makes that // removal deliberate rather than silent. func TestEveryCheckNameThisTreeDeclaresIsWrittenDown(t *testing.T) { - declared, err := ReadWorkflows(workflowsInThisRepository) - if err != nil { - t.Fatalf("reading %s: %v", workflowsInThisRepository, err) - } + declared := declarationsInThisRepository(t) written := map[string]bool{} for _, absence := range Absences { written[absence.Name] = true @@ -286,3 +293,29 @@ func TestAJobWithNoNameOfItsOwnIsNotedRatherThanRefused(t *testing.T) { t.Fatalf("produced %d note(s), and the fallback to the job id is meant to be said out loud", len(verdict.Notes)) } } + +// TestANameReportedOutsideAWorkflowFileIsNotedRatherThanRefused pins the other +// place this package answers with a note. A code-scanning upload creates a check +// run named after the analysis rather than after the job that uploaded it, so +// the name is in no workflow file and a comparison that knew only about jobs +// would refuse a required context matching it as one nothing reports. That would +// be a false red on the change that assembles the required set. +func TestANameReportedOutsideAWorkflowFileIsNotedRatherThanRefused(t *testing.T) { + if len(ReportedOutsideAWorkflowFile) == 0 { + t.Fatal("nothing is written down as reported outside a workflow file, and the measurement in the list's comment says two names are") + } + var required []string + for _, entry := range ReportedOutsideAWorkflowFile { + if !entry.FromUpload { + t.Errorf("%q is in this list and is not marked as coming from an upload, so the report would describe it as read out of a file", entry.Name) + } + required = append(required, entry.Name) + } + verdict := Judge(ReportedOutsideAWorkflowFile, required, nil) + if len(verdict.Refusals) != 0 { + t.Errorf("refused %v, and a required context these names match is reported rather than missing", verdict.Refusals) + } + if len(verdict.Notes) != len(ReportedOutsideAWorkflowFile) { + t.Errorf("produced %d note(s) for %d name(s), and what a reader is holding for these is different from a name read out of a file", len(verdict.Notes), len(ReportedOutsideAWorkflowFile)) + } +}