diff --git a/DELIVERY.md b/DELIVERY.md index 1868d98..deb2715 100644 --- a/DELIVERY.md +++ b/DELIVERY.md @@ -177,6 +177,104 @@ that hold when nobody is paying attention. approval snapshot with the versioning rule quoted back; and an unresolved review thread that correctly blocked a merge while every check was green. +### Read the exit code, not the output + +Judge a command by what it returned, not by what scrolled past. This sounds too obvious to write +down, which is exactly why it keeps happening. + +**Caught:** a lint run reported as clean that was not. The command was piped through `grep` into +`head`, so the shell reported `head`'s exit status, the `|| echo failed` branch never fired, and +silence read as success. CI found the real failure on the next push. + +Three shapes of the same mistake: + +- **A pipeline returns its last stage.** `cmd | grep x | head` tells you about `head`. Use + `set -o pipefail`, or capture `${PIPESTATUS[0]}`, or run the command on its own line and check `$?`. +- **A run that does nothing exits zero.** A test runner that discovers no test files, a linter given + no matching paths, a loop whose input was empty. Assert the count, not the status. +- **Console output is formatted for a human, and the formatting changes.** If you must read a + number out of a tool, read it from that tool's machine-readable output. + +**Caught, by the guard written for the second bullet above.** The anti-vacuity check asserted the +test count by grepping the console reporter for `Test Files 1`. It passed locally and failed a CI +run in which all 24 tests passed, because the runner colours that output and a laptop pipe does not, +so ANSI escapes sat between the words. Reading `numTotalTests` from the JSON reporter is the same +check with nothing to break. + +Worth sitting with: that was written in the same hour as this section, by someone who had just +finished describing the failure. Knowing the rule is not the same as following it, which is the +argument for gates over intentions. + +### A declared requirement is not an enforced one + +When a manifest says which runtime or platform it needs, something has to check that. Most package +managers do not. + +**Caught:** a test library declaring `node >=22` installed into a project on Node 20. Every test +passed, because npm treats `engines` as advisory. A second package in the same batch declared the +same requirement and failed loudly on import, and the loud one is the lucky case. The quiet one had +green CI on an unsupported runtime, and Node 20 had been end of life for three months and was the +base image of the published container. + +The general rule: **anything advisory will be ignored eventually, so read it yourself.** Cheap +version of this gate is a build step that diffs declared `engines` against the CI runtime. + +### Sanitise by allowlist, and expect a second door + +If untrusted text is rendered as markup, the first vector you close is not the only one. + +**Caught:** a page rendering `CHANGELOG.md`, a file edited by pull request, on a static site with no +runtime in front of it. Raw HTML was dropped, which looked complete. `[text](javascript:...)` is +ordinary markdown, so it survived that and rendered as a live href, as did `data:text/html`. Found +by the review bot after the author had already satisfied himself. + +Allowlist what is permitted rather than removing what is known bad, and resolve URLs rather than +matching their prefix. `JaVaScRiPt:` and a leading space are the same thing to `new URL`, and each +needs its own pattern otherwise. + +### Outside contributors test the process, not the code + +Opening a repository to contributors exercises paths no amount of solo work reaches, and most of +what it finds is in the process rather than the diff. + +**Caught, in one week of a repository being open:** + +- A contributor **refused a review instruction of mine** that would have made the host header + authoritative behind `AllowedHosts: "*"`. He was right and I was wrong, and only an outside + reviewer was positioned to say so. +- **Two issues filed for features that already existed**, spotted by a contributor who read the code + rather than the issue. Both were mine, from grepping for library API names instead of the + implementation. +- The **CLA gate blocked the dependency bot's own pull requests**, which nothing had ever exercised + because no bot had opened one against a protected branch before. +- **Assigning a non-collaborator is impossible in the web UI** and works through the REST API once + that person has commented, so an issue can sit unassigned looking like nobody wants it. + +None of these are code defects and none would have surfaced from another solo month. + +### Links and invites expire + +A URL that worked when it was written is not a URL that works. + +**Caught:** a chat invite in the README, the contributing guide, the issue template and twice on the +marketing site, set to expire four weeks out. All five would have gone dead on the same day, on the +one path a new contributor uses, with nothing to report it. Invite links default to expiring; the +non-expiring option is a deliberate setting. + +Worth a periodic link check in CI on the files that onboard people. Note that a bare `curl` is not +enough for this class: the expiring invite returned `200` right up until it did not. + +### Publishing is not releasing + +Pushing a package is one step. If the repository does not also record what shipped, the project +looks abandoned from the outside no matter how active it is. + +**Caught:** 67 versions on the package registry, four git tags, and a releases page showing a version +from many months earlier as *latest*, next to a changelog that was accurate and current. Nothing was +broken; the release job simply never tagged. The costs are real anyway: `git log v..HEAD` does +not resolve, so *what has landed since we shipped* cannot be answered from the repository, and a +contributor whose work merged has nothing that tells them it reached users. + --- ## Say what you actually measured @@ -205,6 +303,10 @@ skipped, say it was skipped. A green tick you did not verify is not evidence. | Cancelled CI job | a failure in `gh pr checks` | a superseded run. Identical durations across jobs is the tell; `gh api .../jobs` says `cancelled` | | Green review bot | reviewed and approved | may mean rate limited and read nothing. Open the comment | | Marketplace 404 | rejected | usually scan latency. Re-check after the next sync before concluding | +| A piped command succeeding | the command worked | the **last stage** worked. `cmd \| grep x \| head` reports `head` | +| Green tests on a new dependency | it is compatible | `engines` is advisory to npm. It may be running on a runtime it declares unsupported | +| A vulnerability alert count | current exposure | may predate the fix. Compare each advisory's patched version against what is actually resolved, and check the alert's `updated_at` against the merge | +| A `200` from a link check | the link is good | for an invite or a token URL, it is good **today**. Check the expiry, not the status | --- @@ -226,8 +328,15 @@ Done once, at the start, before the first feature. All of it is cheaper now than surprise a consumer. - [ ] An anti-vacuity check. A run that discovers no tests exits zero, so assert the count. - [ ] A secrets scan, with no path exemption on the rules that matter. -- [ ] A playground deployment, and a publish workflow that refuses to publish without it. +- [ ] A playground deployment, and a publish workflow that refuses to publish without it. Check the + `needs:` graph, not the intent: the publish job must depend on the job that observed a running + build, and it is easy to wire these the wrong way round and never notice. +- [ ] Release tagging. A tag and a release per published version, with the changelog section as the + body, or the repository cannot say what shipped and the releases page misrepresents the project. - [ ] Issue and pull request templates that ask for the failing case, not the intention. +- [ ] `set -o pipefail` in every multi-stage shell step, so a pipeline reports the failing stage + rather than its last one. +- [ ] A link check over the files that onboard people, including expiry for invites and tokens. --- @@ -244,12 +353,24 @@ rather than the same thing about all of them. It does: | **Carom** (.NET) | a public API gate, and it has eleven open issues inviting contributors | | **Talaan** (.NET) | a changelog, a public API gate | | **BaryoVM** (Go) | a changelog, an `apidiff` gate | +| **barakoCMS** (.NET + npm) | a public API gate, release tagging, and the publish ordering below | Two things that reading it alone would not have surfaced. **Carom is the most exposed**: it invites contributors into a library with no machine-checked public surface, so the first well-meaning pull request can break consumers with every check green. And **a missing changelog is not paperwork** on a published package: without one, a consumer deciding whether to upgrade has only a diff. +**barakoCMS publishes in the wrong order**, which is the one worth fixing first anywhere it appears. +Its release job graph is `test` → `publish` → `deploy-playground`: packages and public images go out, +and only then does anything get deployed and looked at. Phase 6 above says the opposite, and the +reason is asymmetry. A bad deploy is rolled back in a minute. A bad publish is permanent. Package +registries do not delete, they unlist, and anyone who already resolved the version keeps it. **Put +the irreversible step last.** + +Worth checking in any pipeline: draw the `needs:` graph and find where the irreversible job sits. If +anything that touches the outside world runs before the thing that observes a running build, the +ordering is wrong regardless of how thorough the tests are. + ## What this costs, and when to skip it The full process suits something that will be published, installed by strangers, or maintained by @@ -260,3 +381,27 @@ Three parts are never worth skipping, at any size, because each caught something 1. Run the tests in the configuration you ship. 2. Break a new test to prove it can fail. 3. Look at the running system before saying it works. + +--- + +## Keeping this document honest + +This file is a record of things that actually went wrong, not a standard copied from somewhere. That +only stays true if it is updated at the moment something is learned, which is also the moment it is +least convenient. + +**When something gets through, add the gate that would have stopped it, and cite what it caught.** +One short section: what the gate is, and the specific failure in a sentence or two. If you cannot +name what it caught, it does not go in. A gate with no incident behind it is ceremony, and the +opening of this document says to delete those rather than keep them for the look of the thing. + +Three prompts worth answering out loud at the end of a piece of work, because each one has produced +a section above: + +- **What did I believe that turned out not to be true?** Not what broke. What I was confident about. +- **What went green that should have gone red?** A silent pass is worth more attention than a + failure, because the failure announced itself. +- **Who or what caught this, and would it have been caught without them?** If the answer is a person + rather than a mechanism, the mechanism is missing. + +If a session ends with something learned and nothing added here, the learning is gone by next week.