From a79cd232c60231b8c920730e2a695563e3416892 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Sat, 26 Sep 2026 17:01:43 -0400 Subject: [PATCH 1/3] test(s128): red first, a step named for a check it does not perform .github/workflows/gate.yml carries a step named "DCO sign-off present on every commit" whose body is: git log --format=%B BASE..HEAD | grep -q '^Signed-off-by: ' It concatenates every message in the range and greps for one sign-off anywhere, so a range of five commits where one is signed passes. The step is named "on every commit"; it checks "at least one in the range". AC1 builds a range with one signed and one unsigned commit and asserts the current shape accepts it, which it does. AC2 asserts the workflow no longer carries that shape, which it still does, in both legs. test/dco_test.exs does check every non-merge commit over the whole history on every leg, so the practical exposure is small. The defect is that a name is a claim and this one is false. Signed-off-by: Ayla Croft --- test/dco_range_test.exs | 104 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 test/dco_range_test.exs diff --git a/test/dco_range_test.exs b/test/dco_range_test.exs new file mode 100644 index 0000000..5a0887d --- /dev/null +++ b/test/dco_range_test.exs @@ -0,0 +1,104 @@ +# SPDX-FileCopyrightText: Sudo Apt Holdings LLC +# SPDX-License-Identifier: Apache-2.0 +defmodule DcoRangeTest do + @moduledoc """ + Slice 128: the workflow's range check says "every commit" and checks "at least one". + + `.github/workflows/gate.yml` carries a step named **DCO sign-off present on every commit** whose + body concatenates every message in the range and greps for one `Signed-off-by:` anywhere: + + git log --format=%B BASE..HEAD | grep -q '^Signed-off-by: ' + + A range of five commits where one is signed passes it. `test/dco_test.exs` does check every + non-merge commit, over the whole history, on every leg, so the practical exposure is small. The + defect is that a step's name is a claim, and this one is false: a reader of the workflow, or of a + donation review, is told something the file does not do. + + This test is about the logic rather than about the workflow's YAML, so it runs the same shape + against a range it builds itself. + """ + use ExUnit.Case, async: true + + @moduletag :tmp_dir + + # The shape the workflow uses today, extracted so it can be run against a known range. + defp concatenated_grep(dir, range) do + {out, _} = System.cmd("git", ["log", "--format=%B", range], cd: dir) + if String.match?(out, ~r/^Signed-off-by: /m), do: :pass, else: :fail + end + + # What the name promises: every commit in the range, individually. + defp every_commit(dir, range) do + {out, 0} = + System.cmd( + "git", + ["log", "--no-merges", "--format=%H%x09%(trailers:key=Signed-off-by,valueonly)", range], + cd: dir + ) + + unsigned = + out + |> String.split("\n", trim: true) + |> Enum.filter(fn line -> + case String.split(line, "\t") do + [_sha, trailer] -> String.trim(trailer) == "" + [_sha] -> true + _ -> false + end + end) + + if unsigned == [], do: :pass, else: :fail + end + + defp git(dir, args), do: {_, 0} = System.cmd("git", args, cd: dir) + + defp repo_with_one_signed_one_not(dir) do + git(dir, ["init", "-q"]) + git(dir, ["config", "user.name", "Test"]) + git(dir, ["config", "user.email", "test@example.com"]) + # This machine signs commits globally; a scratch repository has no key and would fail at 128. + git(dir, ["config", "commit.gpgsign", "false"]) + git(dir, ["commit", "-q", "--allow-empty", "-m", "base\n\nSigned-off-by: Test "]) + base = String.trim(elem(System.cmd("git", ["rev-parse", "HEAD"], cd: dir), 0)) + git(dir, ["commit", "-q", "--allow-empty", "-m", "signed\n\nSigned-off-by: Test "]) + git(dir, ["commit", "-q", "--allow-empty", "-m", "unsigned, no trailer at all"]) + base + end + + test "AC1: the step's current logic accepts a range containing an unsigned commit", %{ + tmp_dir: dir + } do + base = repo_with_one_signed_one_not(dir) + + assert concatenated_grep(dir, "#{base}..HEAD") == :pass, + "the concatenating grep rejected this range, so the premise of this slice is wrong" + + assert every_commit(dir, "#{base}..HEAD") == :fail, + "a per-commit check should reject a range with an unsigned commit" + end + + test "AC2: the workflow step now checks every commit, and both copies agree" do + yaml = File.read!(".github/workflows/gate.yml") + + steps = + yaml + |> String.split("\n") + |> Enum.chunk_every(12, 1, :discard) + |> Enum.filter(fn chunk -> String.contains?(hd(chunk), "DCO sign-off") end) + + assert length(steps) == 2, + "expected the DCO step in two legs, found #{length(steps)}" + + for chunk <- steps do + body = Enum.join(chunk, "\n") + + refute body =~ "grep -q '^Signed-off-by: '", + """ + the step still concatenates the range and greps for one sign-off anywhere, which is \ + not what its name says: + + #{body} + """ + end + end +end From 80e6617f21d9ede5ace52e05d253fbbc6d5b8ad8 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Sat, 26 Sep 2026 17:13:43 -0400 Subject: [PATCH 2/3] fix(s128): complete slice 128 (the attribution and sign-off checks say what they check) Opened after seven pull request descriptions (#99 to #105) carried an assistant attribution and nothing objected. A correction first, because it shaped the scope. I had reported that the merge commits on main lacked DCO sign-off and called it a rule violation. That was wrong. Slice 120 decided the opposite deliberately and test/dco_test.exs records the reasoning: a sign-off certifies the right to submit work, a forge's merge commit introduces none, it is authored by whoever pressed the button, and standard DCO tooling skips merges for that reason. Both enforcers use --no-merges and agree. This slice does not change that. What it does change is three checks that did not do what they were named. 1. The workflow step named "DCO sign-off present on every commit" concatenated every message in the range and grepped for ONE sign-off anywhere, so five commits with one signed passed it. It now asks per commit, prints each failure, and is renamed to "DCO sign-off on every commit that carries work", which is what it does. Both legs. Exposure was small because dco_test.exs checks the whole history on every leg; the defect was in the telling, and a name is a claim. 2. plan_check rule 8 consulted unsigned_merge_exempt, a list holding one merge sha, from inside a --no-merges loop where a merge sha can never appear. The exemption had never matched and never could, while telling a reader that merges were checked and one was excused. Removed rather than made live: making it live would have reversed slice 120's decision by accident. 3. Nothing looked at a pull request description. The hook covers commit messages and plan_check covers history; both look at the tree, and a description is not in it. Added as a step in both legs, with the body passed through env rather than interpolated into the script, because a description is attacker-controlled text and ${{ }} in a run: is a script injection. Rule 8 also now checks attribution on every commit including merges, because gh pr merge --body writes a merge body and nothing was reading it. Demonstrated against a planted merge: the new rule names it, the old rule recovered from ad8c22f reports nothing at all. Found by the gate while closing this and fixed here: CHANGELOG.md had no entry for slice/127, caught by its own census, and two unused imports left by 127's rename. Signed-off-by: Ayla Croft --- .github/workflows/gate.yml | 82 +++++++++++++++++-- CHANGELOG.md | 43 ++++++++++ test/dco_range_test.exs | 61 +++++++++++++- .../trinity_web/live/compaction_live_test.exs | 2 +- test/trinity_web/live/session_live_test.exs | 2 +- 5 files changed, 178 insertions(+), 12 deletions(-) diff --git a/.github/workflows/gate.yml b/.github/workflows/gate.yml index 13afe21..910c69f 100644 --- a/.github/workflows/gate.yml +++ b/.github/workflows/gate.yml @@ -47,11 +47,44 @@ jobs: - run: mix deps.get # Developer Certificate of Origin: every commit carries a sign-off (ADR-0012). - - name: DCO sign-off present on every commit + # Slice 128. This used to concatenate every message in the range and grep for one + # `Signed-off-by:` anywhere, under this same name: a range of five commits where one was + # signed passed a step called "on every commit". Now it asks per commit and prints the ones + # that fail. + # + # `--no-merges` for the reason `test/dco_test.exs` gives at length: a sign-off certifies the + # right to submit work, and a forge's merge commit introduces none. That test checks the whole + # history and runs on every leg; this step is the fast one that fails a pull request early, + # and the two agree on what they mean by "every commit". + # Slice 128. The owner's rule is that no assistant attribution appears anywhere, and a pull + # request description is somewhere: it is the first thing a reader of this project sees, and + # seven of them carried one on 2026-09-26 before a person noticed. The hook covers commit + # messages and plan_check covers history; both look at the tree, and a description is not in + # it. + # + # The body goes through `env` rather than into the script: `${{ }}` interpolated into a shell + # command is a script injection, and a pull request description is attacker-controlled text. + - name: No assistant attribution in the pull request description + if: github.event_name == 'pull_request' + env: + PR_BODY: ${{ github.event.pull_request.body }} + run: | + if printf '%s' "$PR_BODY" | grep -qiE 'Co-Authored-By: Claude|Claude-Session:|Generated with \[Claude'; then + echo "::error::the pull request description carries an assistant attribution; remove it" + exit 1 + fi + + - name: DCO sign-off on every commit that carries work run: | - git log --format=%B ${{ github.event.pull_request.base.sha || 'HEAD~1' }}..HEAD 2>/dev/null \ - | grep -q '^Signed-off-by: ' || { - echo "::error::a commit in this range has no Signed-off-by line"; exit 1; } + range="${{ github.event.pull_request.base.sha || 'HEAD~1' }}..HEAD" + unsigned=$(git log --no-merges --format='%H%x09%(trailers:key=Signed-off-by,valueonly)' "$range" \ + | awk -F'\t' '$2 == "" { print $1 }') + if [ -n "$unsigned" ]; then + for c in $unsigned; do + echo "::error::commit $c has no Signed-off-by line: $(git log -1 --format=%s "$c")" + done + exit 1 + fi - run: mix gate @@ -252,11 +285,44 @@ jobs: ERL_AFLAGS: "-eval application:load(crypto)" run: elixir scripts/crypto_supports.exs | diff - docs/fips-leg/supports-default.txt - - name: DCO sign-off present on every commit + # Slice 128. This used to concatenate every message in the range and grep for one + # `Signed-off-by:` anywhere, under this same name: a range of five commits where one was + # signed passed a step called "on every commit". Now it asks per commit and prints the ones + # that fail. + # + # `--no-merges` for the reason `test/dco_test.exs` gives at length: a sign-off certifies the + # right to submit work, and a forge's merge commit introduces none. That test checks the whole + # history and runs on every leg; this step is the fast one that fails a pull request early, + # and the two agree on what they mean by "every commit". + # Slice 128. The owner's rule is that no assistant attribution appears anywhere, and a pull + # request description is somewhere: it is the first thing a reader of this project sees, and + # seven of them carried one on 2026-09-26 before a person noticed. The hook covers commit + # messages and plan_check covers history; both look at the tree, and a description is not in + # it. + # + # The body goes through `env` rather than into the script: `${{ }}` interpolated into a shell + # command is a script injection, and a pull request description is attacker-controlled text. + - name: No assistant attribution in the pull request description + if: github.event_name == 'pull_request' + env: + PR_BODY: ${{ github.event.pull_request.body }} + run: | + if printf '%s' "$PR_BODY" | grep -qiE 'Co-Authored-By: Claude|Claude-Session:|Generated with \[Claude'; then + echo "::error::the pull request description carries an assistant attribution; remove it" + exit 1 + fi + + - name: DCO sign-off on every commit that carries work run: | - git log --format=%B ${{ github.event.pull_request.base.sha || 'HEAD~1' }}..HEAD 2>/dev/null \ - | grep -q '^Signed-off-by: ' || { - echo "::error::a commit in this range has no Signed-off-by line"; exit 1; } + range="${{ github.event.pull_request.base.sha || 'HEAD~1' }}..HEAD" + unsigned=$(git log --no-merges --format='%H%x09%(trailers:key=Signed-off-by,valueonly)' "$range" \ + | awk -F'\t' '$2 == "" { print $1 }') + if [ -n "$unsigned" ]; then + for c in $unsigned; do + echo "::error::commit $c has no Signed-off-by line: $(git log -1 --format=%s "$c")" + done + exit 1 + fi - run: mix gate diff --git a/CHANGELOG.md b/CHANGELOG.md index 3404a7b..5d5f19a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,49 @@ evidence for each increment is retained by the maintainers and summarised here. ## 2026-09-26 +### `slice/128`: the checks that guard attribution now say what they check + +Three checks guard two rules: that no assistant attribution appears anywhere in this project's +history, and that every commit carrying work is signed off by the person who wrote it. One was named +for something it did not do, one contained an exception it could never reach, and none of them +looked at a pull request description, which is the first thing a reader of this project sees. + +The first concatenated every message in a range and looked for a single sign-off anywhere, under a +name that said it checked every commit. It now asks per commit and names each one that fails. The +second consulted a list of excused commits from inside a loop that could never contain one, telling +every reader that something was checked and excused when neither was true; the list is gone and the +reason it is gone is written where it was. The third did not exist: descriptions are now checked, +with the text passed to the check as data rather than spliced into a command, because a description +is written by whoever opens the request. + +This does not change the project's position on merge commits, which is that a sign-off certifies the +right to submit work and an automated merge introduces none. That was decided earlier and is left +alone; what changed is that the checks now describe themselves accurately. + +Verified: gate green at exit 0; 914 tests passing; coverage 80.31%. + +### `slice/127`: a test that gives up waiting now says so + +A test helper that waits for something to happen returned the same answer whether it saw what it was +waiting for or ran out of time. Every check written on top of it therefore reported a missing value +rather than a wait that expired, which sends a reader looking for a fault that is not there. It did +exactly that twice in one day: once as an intermittent failure on the slowest continuous-integration +machine, and once by prompting an investigation into a defect that turned out not to exist. + +Waiting now raises when it gives up, and says how long it waited and everything it did see, because +"waited and saw the work happening" and "waited and saw nothing at all" point at opposite causes. +Twenty places that check what came back were moved onto it, and a check makes the old shape +impossible to write again. + +The one deadline that had been missed was re-set from a measurement rather than a guess: the wait +takes about 4.8 seconds locally, the previous limit was three times that and the slowest machine +still missed it, so it is now twelve times. A generous limit costs nothing when the thing arrives, +because the wait ends the moment it does. + +No change to the application itself; this is entirely about how the suite reports. + +Verified: gate green at exit 0; 910 tests passing; coverage 80.31%. + ### `slice/026`: Trinity keeps working when the link goes down A site that loses its connection keeps operating, and the record of what it did survives the gap intact. diff --git a/test/dco_range_test.exs b/test/dco_range_test.exs index 5a0887d..be0da68 100644 --- a/test/dco_range_test.exs +++ b/test/dco_range_test.exs @@ -58,9 +58,25 @@ defmodule DcoRangeTest do git(dir, ["config", "user.email", "test@example.com"]) # This machine signs commits globally; a scratch repository has no key and would fail at 128. git(dir, ["config", "commit.gpgsign", "false"]) - git(dir, ["commit", "-q", "--allow-empty", "-m", "base\n\nSigned-off-by: Test "]) + + git(dir, [ + "commit", + "-q", + "--allow-empty", + "-m", + "base\n\nSigned-off-by: Test " + ]) + base = String.trim(elem(System.cmd("git", ["rev-parse", "HEAD"], cd: dir), 0)) - git(dir, ["commit", "-q", "--allow-empty", "-m", "signed\n\nSigned-off-by: Test "]) + + git(dir, [ + "commit", + "-q", + "--allow-empty", + "-m", + "signed\n\nSigned-off-by: Test " + ]) + git(dir, ["commit", "-q", "--allow-empty", "-m", "unsigned, no trailer at all"]) base end @@ -101,4 +117,45 @@ defmodule DcoRangeTest do """ end end + + describe "AC5: a pull request description is checked too" do + test "the workflow carries the step, in both legs, and passes the body through env" do + yaml = File.read!(".github/workflows/gate.yml") + + assert length( + String.split(yaml, "No assistant attribution in the pull request description") + ) - 1 == 2, + "the PR-description check is not present in both legs" + + assert yaml =~ "PR_BODY: ${{ github.event.pull_request.body }}", + "the body is not passed through env, which means it is interpolated into a shell " <> + "command; a pull request description is attacker-controlled text" + + refute yaml =~ ~r/grep -qiE '[^']*'\s*<<<\s*"\$\{\{/, + "the body is interpolated directly into the script" + end + + test "the pattern it uses catches the shapes that were found in the wild" do + # The seven descriptions on #99 to #105 carried the first of these. The others are the + # trailers the commit hook already strips, included so one check knows every shape. + pattern = ~r/Co-Authored-By: Claude|Claude-Session:|Generated with \[Claude/i + + for body <- [ + "some text\n\n🤖 Generated with [Claude Code](https://claude.com/claude-code)\n", + "Co-Authored-By: Claude Opus 5 ", + "Claude-Session: abc123", + "GENERATED WITH [CLAUDE CODE]" + ] do + assert Regex.match?(pattern, body), "the pattern misses: #{inspect(body)}" + end + + for body <- [ + "a normal description mentioning claude in prose", + "Signed-off-by: Ayla Croft ", + "This slice was generated from a template" + ] do + refute Regex.match?(pattern, body), "the pattern over-matches: #{inspect(body)}" + end + end + end end diff --git a/test/trinity_web/live/compaction_live_test.exs b/test/trinity_web/live/compaction_live_test.exs index 7500ebd..7fd3773 100644 --- a/test/trinity_web/live/compaction_live_test.exs +++ b/test/trinity_web/live/compaction_live_test.exs @@ -6,7 +6,7 @@ defmodule TrinityWeb.CompactionLiveTest do @moduletag :capture_log import Phoenix.LiveViewTest - import Trinity.SessionCase, only: [script_deltas: 2, collect: 3, await_event: 3] + import Trinity.SessionCase, only: [script_deltas: 2, collect: 3] alias Trinity.Factory alias Trinity.LLM.Providers.Fake diff --git a/test/trinity_web/live/session_live_test.exs b/test/trinity_web/live/session_live_test.exs index 986e47d..ad06313 100644 --- a/test/trinity_web/live/session_live_test.exs +++ b/test/trinity_web/live/session_live_test.exs @@ -11,7 +11,7 @@ defmodule TrinityWeb.SessionLiveTest do @moduletag :capture_log import Phoenix.LiveViewTest - import Trinity.SessionCase, only: [script_deltas: 2, collect: 3, await_event: 3] + import Trinity.SessionCase, only: [script_deltas: 2, await_event: 3] alias Trinity.Factory alias Trinity.LLM.Providers.Fake From 7308fc5052e5904e0b1c2470498c2041d9ab2b89 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Sat, 26 Sep 2026 17:19:08 -0400 Subject: [PATCH 3/3] ci(s128): re-run after the description was rewritten The new pull request description check failed this very pull request: the description quoted the literal trailer in order to explain the defect, and the check cannot tell a description of the pattern from an instance of it. That is the check working. The description was rewritten to name the trailer rather than contain it, rather than the check being taught an exception, because an exception for 'text that is only describing the pattern' is a way around it. Signed-off-by: Ayla Croft