ci(dco): enforce DCO sign-off as a required check - #133
Merged
Conversation
ADR-0006 chose a DCO over a CLA because it forecloses a rug-pull while keeping contribution friction low. Sign-off was practiced by every contributor and required by both CONTRIBUTING.md and the PR template, but no ruleset check enforced it. An unsigned commit would have merged. A repository script rather than the DCO GitHub App, so the gate stays inside the surface ADR-0007 keeps mechanical and self-contained. It imports only Node builtins and needs no bun install, so a broken dependency graph cannot take the sign-off gate down with it. Accept/reject semantics track the app's, since that is the contract contributors already know, with two deliberate differences. A sign-off's name and address must come from one identity; the app takes the name from either the author or the committer and the address from either, so a web-UI commit signed "Jane Doe <noreply@github.com>" passes there. And a bot still has to sign: app accounts are exempt from the identity match only, because Dependabot signs from support@github.com and cannot equal its own author address by construction, but presence is still checked. Every exemption is named in the output, so no commit is skipped silently. The squash-merge body setting moves from BLANK to COMMIT_MESSAGES in the same change. A pull-request check certifies the contributor, which is what the DCO is for, but a blank squash body discards every trailer at merge: main's own head f74c089 carries no sign-off while every commit proposed to it carried one. Provenance verified and then thrown away is not provenance. Per ADR-0016 the check was observed rejecting a real unsigned commit in a real repository before it counted as coverage, and the negative cases are permanent. The one that matters most for this class of check is that an empty commit range is an error rather than a pass: an unfetched or misspelled base ref makes git log return nothing and drives every count to zero, which renders identically to a clean run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
There was a problem hiding this comment.
Pull request overview
Adds DCO enforcement for pull-request commits, completing ADR-0006 action item 2.
Changes:
- Adds and tests a standalone DCO verifier.
- Adds a pull-request
dcoCI job and package script. - Documents enforcement and squash-merge provenance changes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/check-dco.ts |
Implements DCO validation and reporting. |
scripts/check-dco.test.ts |
Tests parsing, classification, and execution. |
package.json |
Adds the check:dco script. |
.github/workflows/ci.yml |
Adds the pull-request DCO job. |
CONTRIBUTING.md |
Documents enforcement and remediation. |
docs/adr/0006-license-apache-2-and-single-monorepo.md |
Records the completed action item. |
CHANGELOG.md |
Announces DCO enforcement. |
MANIFEST.md |
Lists the DCO gate. |
Suppressed comments (1)
scripts/check-dco.ts:316
- This remediation text contradicts the implemented and documented rule by saying the trailer must match the author only; committer matches are also accepted. Mention both identities so contributors are not instructed to rewrite an already valid committer sign-off.
`Every commit needs "Signed-off-by: Your Name <your@email>" matching its author.\n` +
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…s on failure Two defects found in review, both observed failing before the fix. The bot exemption only checked that some sign-off existed. A bot-authored commit carrying nothing but an unrelated person's Signed-off-by was exempted and then reported as "signed by the app account as <that person>" — a report asserting something it had never checked, which is the ADR-0016 failure the exemption exists inside. Only the address half needs exempting: Dependabot authors as dependabot[bot] with a noreply address and signs as dependabot[bot] <support@github.com>, so the name matches and only the address cannot. The trailer must now name the bot, and a commit whose signatures name nobody relevant falls through to the ordinary identity match so it fails with a precise message instead of being waved through. The failure diagnostic said "expected a sign-off by <author>" while the classifier accepts the author or the committer. Where those differ, the message omitted a valid way to fix the commit; it now names both, and still names one where they are the same. Verified against the real Dependabot commit on PR #99, which the tightened rule still exempts, so no false positive was introduced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
Open
3 tasks
A pull request runs this gate from its own merge checkout, so a change that edits the script or the workflow step invoking it can produce a green dco status over unsigned commits. Every gate in this repository shares the property, measured on #98: pull_request workflows execute the pull request's ci.yml, not main's. Recorded rather than papered over. Running the script from a trusted base revision leaves the invoking step equally under the pull request's control, so it would look like a control without being one — the failure ADR-0016 is about. Tracked repository-wide in #137, where the option that changes anything is a required review rather than a change to this file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
…ailer A four-lens deep review found one defect, and it fails in the expensive direction: a correctly signed commit was reported as unsigned, which on a required check blocks the merge. The name and address classes excluded < and > but not newline, so a line reading exactly "Signed-off-by:" let the lazy name group run down the message to the next < and absorb the valid trailer beneath it into one match named "Signed-off-by: Jane Doe". That falsified this parser's own documented invariant, which claimed an extra match could only ever add a candidate and never remove a valid one. Confining every class to a single physical line fixes it and caps the backtracking as a side effect: a 500k-character bracket-free line went from superlinear rescanning to 1.4ms. Observed failing in a real repository before the fix, and the four new cases were watched failing against the old regex. The review's other accepted findings, none of which changed behaviour: Failure messages now distinguish "could not examine" from "found unsigned commits". An unfetched base made git say "fatal: bad revision", which reads as a defect in the pull request rather than in the checkout; the script now says which it is. This is the same reporting rule ADR-0016 states, applied to the check's own failures rather than to its verdicts. The remediation block now shows the push, not just the rebase, and says to use --force-with-lease rather than --force, since the tool has just rewritten history and the obvious next command is the destructive one. It also explains in one line what sign-off certifies, for a contributor who has never met a DCO. CI invokes the check:dco alias like every sibling gate rather than holding a second reference to the script path. Verified to work with no node_modules, since the job deliberately runs no bun install. ADR-0006 gains a dco-signoff-required assertion in frontmatter, mirroring ADR-0007's clean-clone-builds. In a project about governance-as-code, a control recorded only in prose is invisible to anything querying the corpus. The merge exemption's comment no longer claims more than it can: an evil merge's own diff is not certified by its parents. Matching the DCO app's skip is a deliberate contract choice, not a property of merges. CONTRIBUTING warns browser editors before the wall rather than after, since the first-PR section steers newcomers toward exactly the docs edits made in the web UI, which cannot carry a trailer. The PR template's checklist item points at that section for commits already made. Also corrects a contradiction inside this same Unreleased block, where the ratification entry still said in the present tense that no ruleset check enforces the DCO. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
mbeacom
added a commit
that referenced
this pull request
Aug 14, 2026
… record ADR-0028 was oversized and mistiered. The repository's own precedent is `bfd9c84` (#133): a new `ci.yml` job plus a new `scripts/check-*.ts` with observed-failing fixtures, recorded as completed action item 2 on ADR-0006 and adding no ADR at all. This change is that same shape and should follow it. Roughly half of ADR-0028 also restated comments shipped in the same diff — the `exactly one` reasoning lives in the `findCommentViolations` docblock, the exclusions and `needs:` rationale in the job comments, the rung split in the reference-repo README. What is genuinely new — granting a CI job `pull-requests: write`, rejecting `pull_request_target`, and the reasons the two exclusions must stay — is now carried by ADR-0026 action item 9, in the style of ADR-0006 item 2. The property itself is declared as an `assertions` entry (`ci-comment-unique`) rather than prose, matching how ADR-0006 declares `dco-signoff-required`, and `scripts/check-ci-comment.ts` joins ADR-0026's `affects` so the decision still surfaces on any change to it. Item 8 now points at the runnable rung-2 artifact instead of asking a future reader to build one. The ARB queue returns to empty. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
mbeacom
added a commit
that referenced
this pull request
Aug 15, 2026
…oth rungs (#143) * ci(action): give the comment-posting Action an end-to-end signal on both rungs `self-dogfood` runs the CLI with `contents: read`, so it never constructed a GitHub client, resolved an identity, or posted a comment. The surface #107 lived in — comment identity and upsert — had no end-to-end coverage before shipping to every adopter pinned at the moving `v0` tag. That is how #107 survived two releases: every suite was green, and the job log it printed is exactly what healthy operation prints. Rung 1 — a continuous `action-dogfood` job runs `uses: ./packages/ci` twice against this repository's own pull requests and asserts, over the API, that exactly one comment leads with the marker and a Bot authored it. `exactly one`, not `at most one`: an empty comment list satisfies "at most one", and an empty list is what a revoked permission, a wrong PR number, and a silently-degraded Action all produce (ADR-0016). Gated on `clean-clone-builds` so it cannot pass over a stale committed bundle, serialized per pull request, and asserted between the two dispatches on a bounded retry as a read-your-writes barrier. Fork and Dependabot pull requests are excluded: their token is read-only whatever `permissions:` declares, so the Action correctly degrades (FR-014) and the assertion would fail a healthy Action. `pull_request_target` was rejected outright. Rung 2 — the reference-repository run ships as a runnable workflow rather than an instruction (ADR-0016 clause 4), covering two scenarios rung 1 structurally cannot: a fail-closed dispatch that must write nothing, and the FR-014 degrade under `pull-requests: read` that must stay green. Its evidence index is created empty and explicitly NOT YET OBSERVED — the comment path stays `implemented`, not `reference-verified`. `scripts/check-ci-comment.ts` imports only builtins, reports every marked comment it examined rather than only its verdict, and keeps permanent negative fixtures for the duplicate (#107), absent, empty, and human-authored shapes. Each was observed rejecting, as was the marker-drift test against a deliberately drifted copy. Closes ADR-0026 action item 9. Refs #135 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs(adr): fold the end-to-end signal into ADR-0026 rather than a new record ADR-0028 was oversized and mistiered. The repository's own precedent is `bfd9c84` (#133): a new `ci.yml` job plus a new `scripts/check-*.ts` with observed-failing fixtures, recorded as completed action item 2 on ADR-0006 and adding no ADR at all. This change is that same shape and should follow it. Roughly half of ADR-0028 also restated comments shipped in the same diff — the `exactly one` reasoning lives in the `findCommentViolations` docblock, the exclusions and `needs:` rationale in the job comments, the rung split in the reference-repo README. What is genuinely new — granting a CI job `pull-requests: write`, rejecting `pull_request_target`, and the reasons the two exclusions must stay — is now carried by ADR-0026 action item 9, in the style of ADR-0006 item 2. The property itself is declared as an `assertions` entry (`ci-comment-unique`) rather than prose, matching how ADR-0006 declares `dco-signoff-required`, and `scripts/check-ci-comment.ts` joins ADR-0026's `affects` so the decision still surfaces on any change to it. Item 8 now points at the runnable rung-2 artifact instead of asking a future reader to build one. The ARB queue returns to empty. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * ci(action): assert list completeness and comment-id stability Addresses review on #143. The reported pagination defect does not exist: `gh api --paginate` merges a top-level array across pages into one JSON document, verified against gh 2.96.0 over 12 pages of 1 — one array, parsed clean, zero `][` boundaries. The adjacent gap is real, though. A caller who loses `--paginate` sees page one only, and a duplicate sitting on page two then reads exactly like a healthy single comment. Both workflows now read the comment count GitHub reports for the issue and pass it as `--expect-total`, turning that blind pass into a named failure. The total is read BEFORE the list and compared with `<`, not `!=`: a comment arriving between the two calls makes the list one longer, which is benign, where the other ordering would make it one shorter and read as truncation on a healthy run. `--expect-id` replaces the un-asserted `edited` field, which is the reviewer's improvement rather than a defence of the original. An in-place update preserves the comment id and a create issues a new one, so a count of one can no longer be satisfied by a replacement. Unlike `updated_at` on a byte-identical PATCH, id stability is contractual, and it is a specific observed value rather than a count (ADR-0016 clause 3). Writing the id to a file rather than a stream keeps the human-readable report in the job log; redirecting stdout to capture it would have hidden exactly the "state what you examined" half of ADR-0016. Also from review, in the reference-repository artifact: - Third-party actions pinned to full commit SHAs, matching ci.yml. An artifact whose first claim is reproducibility had four mutable major tags in it. - `workflow_dispatch` removed. Every job is gated on `pull_request`, so a manual dispatch produced a green run with every job skipped — a pass that proved nothing, which is the shape this artifact exists to detect. - The read-your-writes barrier added before the second dispatch, so the rung-2 run cannot fail a correct Action on replica lag the way ci.yml already guards against. - Evidence-index row 2 restated as the id-stability assertion it can actually make, instead of an `updated_at` claim nothing verified. - README ordinal corrected: the degrade path is the fourth row, not the third. A guard was itself observed failing first: `--expect-total=` parsed as 0 through `Number('')`, silently disabling the check while looking applied. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * ci(action): make the completeness check immune to a mid-read deletion Found while reviewing the previous commit, not by the review round that prompted it. `--expect-total` was read once, before the list. That makes a comment CREATED mid-read benign (the list comes back longer) but a comment DELETED mid-read a false failure: the list is one shorter than the count and reads as truncation. The first assertion retries, so it would have self-healed there; the second does not retry, so it would have failed a healthy Action outright — on a job slated to become a required check, in front of external contributors. Both assertions now bracket the list read with two count reads and pass the smaller. Only a genuinely truncated list is short of every count observed around the read, so both directions of concurrent edit are benign and truncation is still caught. `expectTotal` is documented as a lower bound rather than an equality, which is what it always should have been. Simulated end-to-end through the real checker: steady state (4,4) PASS, comment added (4,5) PASS, comment deleted (5,4) PASS, truncated list (30,30) FAIL. The deletion row is the one that failed before this change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * ci(action): close the deep-review findings on the gate and the rung-2 artifact Three lenses (adversarial, operator, consumer) reviewed #143 at 9da873b and returned 20 findings, 8 major. The Action's public surface is deliberately unchanged. The gate was STRICTER than the Action it verifies. `classify` counted any marker-leading comment as ours, while `findOwnComment`'s app-installation branch requires a bot author as well. So any user could red a soon-to-be-required check with one line that renders invisibly, and the failure text blamed #107 for a defect that did not exist. Ownership is now bot-authored AND marker-leading, the same pair ADR-0026 already defines; a non-bot marker-leading comment is reported as an `impostor` and never counted toward `duplicate`. Verified: the fixture that previously failed now exits 0. A duplicate outlives its cause, because the Action never deletes. The gate now snapshots the ids it already owns BEFORE dispatching and reports provenance — "every one already existed before this run, so this run created none of them" rather than "the regression of #107" — and every duplicate message now names the remediation, including that hiding a comment does not remove it from the API. The rung-2 artifact could pass having read nothing. Its snapshots are pipelines ending in `sort`, which exits 0 on empty input, under GitHub's default `bash -e` WITHOUT pipefail: a failed `gh api` produced an empty snapshot, a passing step, and a before/after diff that printed "comment set unchanged" over two failed reads. Confirmed by execution. Every run block now sets `-euo pipefail` and asserts its snapshot is non-empty — the `degrade-read-only` job's assert also being the only thing that proves a `pull-requests: read` token can list comments at all. `incomplete` is now a retryable exit code (2) distinct from a definitive verdict (1). A lagging read replica raises it as readily as a lost `--paginate`, and the second assertion previously failed outright on it, reddening a required check on a healthy Action with a message blaming pagination. Retrying is scoped to exit 2 only: retrying a `duplicate` would be retrying until a gate passes. The `gh api` reads inside the retry loop are now soft. Under `bash -e` a bare failure killed the step on attempt 1, so the loop could not retry the transient class it exists for. The loop's closing line now names the last verdict observed instead of an OR across two causes with opposite responses. Also from review: the reference workflow gains a concurrency group (`cancel-in-progress: false`, matching release.yml) and the min-bracket count read that only ci.yml had; `--help` now prints usage instead of erroring; CONTRIBUTING documents the check, the bot comment, and the fork skip beside the DCO section; the reference README states the pinned commit must contain the assertion script and that the PR must come from a branch, not a fork; and site/src/content/docs/ci.mdx states the comment path's maturity rather than leaving adopters to infer parity with the reference-verified queue Action. Recorded honestly rather than closed badly: after a PR's first push the comment already exists, so a dispatch that writes NOTHING satisfies both assertions. The inverse of #107 is caught on every newly-opened PR's first run, not on later pushes within one PR; rung 2 covers that pair from a clean state. The two ways to close it at rung 1 — an Action output, or deleting the comment each run — were rejected for expanding a published contract to serve a test, and for notifying every subscriber on every push. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * ci(action): fix four artifact defects the rung-2 run exposed, and record the evidence The reference artifact executed for the first time on mbeacom/adrkit-t018-dogfood#16 against adrkit pinned at 71f46d6. All three scenarios passed, and running it found four defects that static review could not. `path: .adrkit` deleted the contents of a directory the reference repository tracks — `.adrkit/lint.json` and `.adrkit/queue.json`, consumed by its badge validation — logged verbatim as "Deleting the contents of '…/.adrkit'". Latent today, destructive the moment a step in that job reads repo state. Moved to `.adrkit-src`, the name that repository already reserves in .gitignore for exactly this purpose. `--paginate` applies `--jq` per page and concatenates, so `join(",")` INSIDE the filter emits one comma-joined line per page. Past 30 comments `--expect-ids` would receive a multi-line value that parses as NaN. Reproduced against the labels endpoint at per_page=5 — three lines versus one — and fixed in `action-dogfood` as well, which carried the same defect. `degrade-read-only` never echoed its observed outcome, unlike its sibling, so that evidence row had to be read from the REST API instead of a log line. The README omitted two preconditions that cost the operator real cycles: a token with the `workflow` scope (the artifact is delivered as a workflow file), and the requirement that no other workflow in the reference repository post the same marker comment. That last one is the subtle finding, and it is why attempt 1 is not the cited run. The reference repo's own adr.yml — pinned at the moving @main, not at the commit under test — created the comment four seconds before the snapshot step read it. A second writer does not fail the run: it satisfies the `absent` rule with a comment this workflow did not create, so `idempotence` passed having observed two updates and no create. A green rung-2 run is not self-evidently a complete one. Attempt 2 deleted the comment, confirmed zero, and re-ran this workflow alone: `created` then `updated`, id #5289855976 both times. The evidence index is filled from attempt 2 and keeps two values apart that are easy to conflate: the uploaded zip digest is not the snapshot member digest, and `steps.invalid.outcome=failure` is not what the REST API reports, because continue-on-error rewrites that step's conclusion to success. One design rationale became an observation. `updated_at` did not move on either in-place update — GitHub does not bump it for a PATCH whose body is byte-identical — so an artifact asserting `updated_at` advanced would have failed both attempts against a healthy Action. The gate asserts id stability instead, and that hedge is now evidenced rather than argued. ADR-0026 action item 8 is closed. The reviewer verdict is outstanding, so the comment path stays `implemented` and is not yet `reference-verified`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs(evidence): cite the corrected-artifact run as the rung-2 evidence The four fixes in b840e91 were made AFTER the run that produced the evidence, so what adrkit shipped was an artifact corrected after its own verification — unverified, which is precisely the state this index exists to make visible. Run 3 (31773788433, adrkit pinned at b840e91) is now the cited evidence. All three jobs green, and each fix confirmed rather than assumed: - `Deleting the contents of '…/.adrkit'` no longer appears; the only remaining deletion line is the ordinary workspace checkout, and `.adrkit/lint.json` and `.adrkit/queue.json` still exist in the reference repository afterwards. - `already ours before this run:` is empty, so the create was genuinely observed rather than inherited: `created` then `updated`, id #5289930628 both times. - `observed: steps.readonly.outcome=success` now appears, so that evidence row is fillable from a log line instead of the REST API. - fail-closed before/after members both sha256 037f3377…, artifact zip 28b63449… Runs 1 and 2 are retained and labelled rather than deleted: run 1 is the one that was green while proving less than it appeared to, and keeping it is what makes the "a green rung-2 run is not self-evidently a complete rung-2 run" caution concrete rather than abstract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs(evidence): measure the updated_at mechanism instead of inferring it The rung-2 report and this repository's records asserted that GitHub does not bump `updated_at` for a PATCH whose body is byte-identical. That was an inference from two runs where the body happened to be identical, stated as a mechanism — the shape ADR-0016 exists to catch, in a record about ADR-0016. A sibling observation on mbeacom/openleague#328 appeared to contradict it: `updated_at` advanced across two dispatches. It does not contradict it. Those runs were on different head commits (208663a2, c225e146), so the rendered body legitimately differed. Rather than reason about it, measured it. A probe comment on this pull request, created and then deleted: create updated_at = 05:47:25Z PATCH with identical body updated_at = 05:47:25Z (unchanged) PATCH with changed body updated_at = 05:47:33Z (advanced) So `updated_at` reports whether the body changed, not whether a write occurred. The claim was right and is now measured; the openleague case is consistent. This strengthens rather than weakens the id-stability choice: an artifact asserting `updated_at` advanced would fail against a healthy Action on every run that renders the same text twice, which is exactly what two dispatches in one job do. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * fix(evidence): correct a harmful reference instruction that caused a real duplicate The reference README told the operator to delete the marker comment BEFORE pushing. That is backwards, and the confirming run proved it: clearing the comment removes the accidental protection a pre-existing one provides — it makes a second writer update rather than create — so the verification workflow and the reference repository's own adr.yml both listed an empty set and both created, within the same second, producing consecutive ids #5289920445 and #5289920446 and a failed run. The guidance made the race likely instead of preventing it. The order is now push → settle → delete → `gh run rerun`, which excludes the race structurally rather than probabilistically, because a re-run does not re-trigger other workflows. Verified twice: after `gh run rerun`, the verification workflow advanced to attempt=2 while the other stayed at attempt=1, completed. Two things follow from that failure, and they point in opposite directions. It closes an ADR-0016 gap this index had recorded as open. Every rung-2 assertion had passed until now, which shows the gate accepting healthy behaviour and never shows it catching anything. The `duplicate` rule has now been observed rejecting a genuine duplicate produced by real infrastructure rather than by a fixture. And the message it printed was wrong about the cause. It asserted "a dispatch in this run created rather than updated — the regression of #107" when the Action had behaved correctly and logged exactly one `created`. `--expect-ids` separates "predates this run" from "created during it"; it cannot separate "created by this run's dispatch" from "created by a concurrent foreign writer". The message now names both causes and points at the Action's own log, which does distinguish them, with a test pinning that it no longer asserts a single cause. Also corrected in the index: it claimed all nine jobs concluded success, which became false the moment a run failed. The failing run is now listed as run 3, labelled as the most useful entry in the table. Also recorded, from the report: `dist/index.js` is byte-identical at both pins, so every run exercised the same Action through a changing harness and the behavioural evidence carries across the correction; and the survival of `.adrkit/lint.json` inside the runner workspace is evidenced indirectly via the badge-report validation on the same head SHA rather than claimed directly, since no step lists files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs: record the disposition step and the pre-emption instance Two additions from the reference operator's closing report, both amendments to existing documents rather than new records. The README documented how to run the artifact and never said what to do with the branch afterwards. "Close without merging" is the non-obvious half: the file triggers on pull_request, so merging it to a reference repository's default branch makes it run on every future pull request there, concurrently with whatever else writes the marker. That converts the documented race from an incidental hazard into a permanent CI failure mode — false #107 reports on unrelated changes, and fail-closed/degrade-read-only reddening whenever another write lands inside their byte-for-byte snapshot brackets. The branch is retained so the run, its logs, and its artifacts stay reachable for the evidence index to cite. ADR-0016 gains an instance, because it is the record's own shape one level up. The operator had already identified the race in an earlier report, suspected the delete-before-push ordering was wrong, and ran it as written anyway rather than quietly substituting a better one. That produced the first observation of the `duplicate` rule firing outside a fixture, exposed an instruction defect that would otherwise have shipped, and revealed that the failure message asserted one cause where two were possible. Had the ordering been silently corrected, all three would still be latent, and the correction would have been invisible to everyone including the reviewer. A silently corrected instruction is an unobserved failure. Clause 4 covers handing over a request to observe a failure without the failing case; this is the same error one level up — removing the conditions that would have produced one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs: withdraw the updated_at mechanism claim; it does not generalise Falsified by the openleague session. I asserted "GitHub does not bump `updated_at` for a PATCH whose body is byte-identical" and, worse, hardened it from inference to "measured" on the strength of a probe run in exactly one context. It does not hold in another. adrkit#143 probe, User via OAuth, byte-identical body -> unchanged adrkit-t018-dogfood#16, github-actions[bot], both runs -> unchanged openleague#328, github-actions[bot], 3 same-commit re-runs, SHA-256 identical body -> ADVANCED every time Two explanations ruled out rather than assumed. Both paths use the same endpoint: the Action calls octokit.rest.issues.updateComment, and the probe PATCHed /issues/comments/:id, so the issue-vs-review-comment hypothesis fails. Elapsed time makes no difference either — the probe was repeated at +5s and +50s with the same result. The only remaining observed difference is the actor type, and that is recorded as a hypothesis, not a finding, because recording it as a finding would repeat the exact error being corrected. I also had a fact wrong when I first dismissed the contradiction: I told that session its observation was "a second push, not a re-run". Run 31773282846 reports run_attempt 2 on an unchanged head_sha. It was a re-run, and I reconciled away a real contradiction instead of measuring it. The correction strengthens the gate rather than weakening it. `check-ci-comment` asserts id stability, and that never depended on which direction `updated_at` moves — only on the field being uncontractual. Two contexts disagreeing about the same operation evidences that better than either result alone: an artifact asserting `updated_at` advanced fails in the first two rows, one asserting it held fails in the third, and id stability holds in all three. Third correction to this one claim: stated as inference, upgraded to "measured" from a single context, now withdrawn. Recorded in place rather than quietly deleted, because the sequence is the useful part. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs: retract the actor explanation too, and say the mechanism is unknown The replacement for the withdrawn `updated_at` claim was also wrong, and contradicted by the table printed beside it: I named the actor as the remaining variable while the same table showed the unchanged and the advanced rows are both `github-actions[bot]`. A conclusion refuted by its own evidence, one commit after withdrawing a claim for being over-generalised. Measured rather than argued, to close the two candidates that could be closed: endpoint the Action calls octokit.rest.issues.updateComment; the probe PATCHed /issues/comments/:id -- the same endpoint elapsed time identical-body PATCH at +5s, +50s, +70s, +90s and +150s, three probe comments, six PATCHes, all created and deleted: never advanced What remains uncontrolled is that the unchanged rows update within one run or session and the advanced row updates across separate workflow runs. That is stated as uncontrolled, not offered as a third explanation. Two have been wrong already, and each read as reasonable when written. Also added, at the reference operator's suggestion, the framing a reader most needs and which "claim withdrawn" alone hides: the falsification made the gate STRONGER. Asserting id stability never depended on which direction updated_at moves, only on the field being uncontractual. Contexts that disagree evidence that better than agreement would have -- an artifact asserting it advanced fails two rows, one asserting it held fails the third, and id stability holds in all three. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs: isolate the authorship variable, and retract a refutation it invalidates Measured in one repository with everything else held constant -- same endpoint, same User credential, same byte-identical body -- varying only whether the editor authored the comment: editing its own comment 6 PATCHes, +5s..+310s never advanced editing another author's comment 3 PATCHes, +0s/+6s/+20s advanced every time Editing another author's comment always moves updated_at; editing your own with an identical body does not. That is controlled rather than inferred, which is more than either of the two withdrawn explanations had. It also invalidates a refutation, including one I had already accepted. The run-boundary variable was reported measured out by three byte-identical PATCHes in one session on openleague#328 -- but those were a User token editing a bot-authored comment, the regime that advances unconditionally. The test could not have produced another result, so the run boundary is NOT refuted. What is left unexplained is one row rather than a whole phenomenon: the Action editing its OWN comment with an identical body advanced updated_at across separate workflow runs on openleague#328, while the same operation within a single run on adrkit-t018-dogfood#16 did not. Endpoint, elapsed time and actor are closed; the run boundary survives for that row and is not claimed as the answer. Two process notes, recorded because they are the same failure as the subject matter. The first attempt at this test PATCHed the live governing-decisions comment on #143 with `gh api --jq .body` output, which appends a newline -- so it changed the body it meant to hold constant, and mutated a real artifact. Restored byte-exactly (verified by hash, first line intact, gate re-run green). The verification then compared a FILE hash against a BODY hash and refused to restore; the guard was right to stop, and the comparison was wrong. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * docs: confirm the authorship variable cross-repo, and record two hazards Three additions, all amendments to existing records. The authorship result reproduces in a second repository: on openleague, a self-authored comment held its updated_at across three byte-identical PATCHes while a bot-authored one advanced on all three, same session and credential. So "editing another author's comment always moves updated_at; editing your own with an identical body does not" is not a property of one repository. A probe hazard worth more than my own embarrassment: `gh api --jq '.body'` appends a trailing newline, so round-tripping it through `-F body=@file` PATCHes body + "\n" and silently changes the body a byte-identical test exists to hold constant. It mutated the live governing-decisions comment on #143 before being caught and restored byte-exactly. The other operator avoided it only by parsing JSON in Python rather than by noticing it, which is luck rather than method. Recorded where probe methodology lives, with the remedy: read and write the body as JSON, and hash what is stored rather than what a shell redirect produced. ADR-0016 gains a third failure kind, because it is the record's own drafting story recurring. The reference operator wrote down a confound -- "editing another author's comment may always bump" -- and one message later ran three measurements carrying that exact confound and reported the variable closed. Not a boundary error, and not blindness to data in view: the correct caveat had already been stated, in their own prior output, and was not carried forward. Neither closer reading of the evidence nor more scepticism about the conclusion would have caught it; only re-reading their own earlier message would. That is precisely the mechanism the record already describes in its own drafting, where its author asserted from memory rather than re-reading a thread about that failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> * ci(action): fix the retry loops, which could not retry the race they exist for Second review of the code delta, adversarial + operator. Both lenses landed on the same class of defect: the hardening applied to one step was not applied to its siblings, and the one guard that mattered most was aimed at the wrong rule. The retry loops were decorative for their stated purpose. The read-your-writes race they exist to absorb -- a comment created moments ago, not yet on the read replica -- produces `absent`, not `incomplete`. With `absent` permanently in the definitive set, the checker exited 1 and the workflow's `status -ne 2` branch killed the step on attempt 1. The loops could only ever retry the lost---paginate class, which retrying cannot fix. Reproduced before fixing: check-ci-comment empty.json --expect-total=1 -> exit 1 (wanted 2) `--just-wrote` now marks `absent` retryable for a caller that dispatched a write immediately before the read, and leaves it definitive otherwise -- a bare invocation has claimed no write, and treating a missing comment as transient there would restore the blind pass. A duplicate or a changed id stays fatal with the flag set, by test. Three more, all measured rather than argued: - Bare `gh api` reads in the second assertion, the snapshot step, and both reference-workflow loops. Under `set -euo pipefail` a single 502 or secondary rate limit killed the step on attempt 1 -- the transient class the loop exists for -- while its comment claimed it was bracketed "the same way as above". All are soft now. - `$(( before < after ? before : after ))` reads a non-numeric value as a variable name, which is 0 when unset. Measured: `before=null` yields 0 without `-u`, aborts with it. So a malformed count silently disabled the completeness cross-check at `--expect-total=0` while the log showed it applied. Counts are validated as `^[0-9]+$` and a bad read costs an attempt. - The `prior-ids` jq filter split on `\n` alone while the classifier splits on all three terminators, making it a fourth place deciding ownership differently. Measured: a CRLF body was `own` to the checker and invisible to the snapshot, which would misattribute a pre-existing duplicate to #107. From the operator lens: `cancel-in-progress` is now `false`, matching release.yml and the reference workflow. This job was the odd one out at `true` while the reference file argues the opposite in writing, for the same write to the same comment -- and both files were written here. Cancellation is a signal plus a grace period, so a cancelled dispatch can still be mid-create when its replacement creates its own, and that duplicate is non-retryable and needs a human to clear it. Retries gained exponential backoff to 75s, both loops now carry the same budget and a framed exhaustion message, and the job gained timeout-minutes: 10 so a hung request becomes a red check rather than six hours of Pending, which is the one state that notifies nobody. ADR-0026 records a third ruleset fact: `clean-clone-builds` must be made required in the same edit, because a job skipped for a failed `needs` reports success exactly like one skipped by an `if:` -- so a failed build would satisfy the comment gate without the Action ever running. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev> --------- Signed-off-by: Mark Beacom <m@beacom.dev> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #130.
ADR-0006 chose a DCO over a CLA because it "forecloses a rug-pull" while keeping contribution friction low. Sign-off was practiced by every contributor and required by both
CONTRIBUTING.mdand the PR template — and enforced by nothing. An unsigned commit would have merged.What landed
A
dcojob on every pull request, now a required status check on themainruleset, backed byscripts/check-dco.ts.A repository script rather than the DCO app, so the gate stays inside the surface ADR-0007 keeps mechanical and self-contained. It imports only Node builtins and runs with no
bun install, so a broken dependency graph cannot take the sign-off gate down with it.Accept/reject semantics track the app's — that is the contract contributors already know — with two deliberate differences:
Jane Doe <noreply@github.com>passes there. Pairing can only reject a trailer naming nobody who touched the commit, so it rejects nothing well-formed.dependabot[bot] <…+dependabot[bot]@users.noreply.github.com>and signs asdependabot[bot] <support@github.com>— the two cannot be equal by construction. Presence is still checked, which is strictly stronger.Every exemption is named in the job output, so no commit is ever skipped silently.
The squash-merge gap, which was the real hole
squash_merge_commit_messagewasBLANK. A pull-request check certifies the contributor, which is what the DCO is for — but a blank squash body discards every trailer at merge.main's own head (f74c089) carried no sign-off while every commit proposed to it carried one. Provenance that is verified and then thrown away at merge is not provenance.The setting is now
PR_TITLE+COMMIT_MESSAGES, so trailers land onmain.Observed failing (ADR-0016)
Not "the suite still passes". The check was watched rejecting real commits in a real repository:
the sign-off is missingexpected a sign-off by "Mark Beacom <m@beacom.dev>", but got "Someone Else <else@example.com>"git rebase --signoff origin/main— the fix the error message prints3 signed, 0 exempt, 0 unsigned35542a57)The negative cases are permanent in
scripts/check-dco.test.ts(35 tests). The one that matters most for this class of check: an empty commit range is an error, not a pass. An unfetched or misspelled base ref makesgit logreturn nothing and drives every count to zero, which renders identically to a clean run — the exact fail-quiet shape ADR-0016 exists to prevent.Two tightenings came out of watching tests fail rather than from review: backtracking let a greedy name capture absorb the first
<…>pair fromSigned-off-by: Jane <jane@x> <spoof@x>, and under themflag a\s*$terminator runs past the trailer's own line. Both have negative cases.CI verifies by explicit SHA pair, not
origin/$BASE_REF..HEAD— a ref resolves to whatever it points at now, which is the stale-read failure ADR-0016 records under "report what was examined".The gap was live, not theoretical
Open PR #98 has 4 commits that will fail this check — its 4 authored commits carry no trailer; its 4
Merge branch 'main'commits are correctly exempt. Fix on that branch:Verification
bun test— 2032 pass, 0 failtypecheck,lint,check:deps,check:freeze-hashes,check:doc-pins,check:changelog,check:dco— all greendcoobserved reporting green on this PR in 10s, so the newly-required check is not a merge deadlockdocs/adr/0006action item 2 is ticked with its evidence;CONTRIBUTING.md,CHANGELOG.md, andMANIFEST.mdupdated.